Working on this now.

Created attachment 20483 [details] [review]
Patch agains HEAD that implements gnome_vfs_async_seek ()

Adding PATCH keyword.

It resulted to be pretty easy to do, because gnome_vfs code is clean
and well structured.

Can anyone review this please?

Created attachment 20707 [details] [review]
Updated patch

The previous patch was not working because it was missing a case in
_get_job_completed().

This version has been tested in downman with both ftp and http and
works well.

Please, review.

I started to look at that patch, but need to spend more time on it. A
few first glance comments:
* you forgot to add "seek" in job_debug_types[], this probably breaks
the JOB_DEBUG_TYPE macro since you modified the corresponding enum
* you should handle GNOME_VFS_OP_SEEK in gnome_vfs_op_destroy and
_gnome_vfs_job_destroy_notify_result since all the other operation
types are there
Will look at it more in details at a later point :)

> * you forgot to add "seek" in job_debug_types[], this probably breaks
> the JOB_DEBUG_TYPE macro since you modified the corresponding enum

Yes, I that didn't got in the patch, I forgot, sorry.

> * you should handle GNOME_VFS_OP_SEEK in gnome_vfs_op_destroy
> and _gnome_vfs_job_destroy_notify_result since all the other operation
> types are there

They are both being handled, just that the SEEK operation does not
need to do anything special there. I have revised it and I don't see
nothing that needs to be destroyed/unref/freed.

> Will look at it more in details at a later point :)
Great

>They are both being handled, just that the SEEK operation does not
>need to do anything special there. I have revised it and I don't see
>nothing that needs to be destroyed/unref/freed.

I'm not implying you should be doing something, just that it should be
there for consistency, for example there currently is:

	case GNOME_VFS_OP_XFER:
		/* the XFER result is allocated on the stack */
		break;
or
	case GNOME_VFS_OP_READ:
	case GNOME_VFS_OP_WRITE:
	case GNOME_VFS_OP_CLOSE:
	case GNOME_VFS_OP_READ_WRITE_DONE:
		break;

I agree, this is totally unimportant ;)

The "case GNOME_VFS_OP_SEEK" is there, look at the patch attachment in
bugzilla. Perhaps it didn't applied cleanly?

Hmm yeah, I looked at the patch without applying it during lunch break
and managed to miss those two chunks, sorry about that

I looked at it some more, and the 
 	case GNOME_VFS_OP_CREATE_AS_CHANNEL:
 	case GNOME_VFS_OP_CREATE_SYMBOLIC_LINK:
+	case GNOME_VFS_OP_SEEK:
 		/* if job got cancelled, no close expected */
 		return job->cancelled || job->failed;
and
+	if (result != GNOME_VFS_OK) {
+		/* if the seek failed, just drop the job */
+		job->failed = TRUE;
+	}

part doesn't feel right. I don't think you want to remove the job from
the job map when a seek fails, otherwise things like subsequent seeks
with the same handle will also fail. I'm quite new to this async code,
so forgive me if I'm wrong once again ;)

+	case GNOME_VFS_OP_SEEK:
 		/* if job got cancelled, no close expected */
 		return job->cancelled || job->failed;

Yes, you are right. This was only change in the second patch.
Obviously it was working for me because this usually returns FALSE.

I have modified the code to always return FALSE, but I still have one
doubt:

        case GNOME_VFS_OP_READ:
        case GNOME_VFS_OP_WRITE:
                g_assert_not_reached();
                return FALSE;

it looks that job_complete will not be called for read and write,
what's the reason? does this reason apply to seek?

and
+	if (result != GNOME_VFS_OK) {
+		/* if the seek failed, just drop the job */
+		job->failed = TRUE;
+	}

Yes, you are right again. I based the the code on "open". So this must
just be removed from the patch.

I will send an updated patch when you finish finding bugs :)

When the job is completing, op->type contains
GNOME_VFS_OP_READ_WRITE_DONE, it can no longer contain GNOME_VFS_OP_READ
or GNOME_VFS_OP_WRITE (see the end of execute_read and execute_write)

Btw, you can attach an updated patch, I have stopped looking at this
one for now and am reviewing the ftp seek one

Created attachment 20778 [details] [review]
updated patch

Changes in the last patch:

- added "seek" bug string
- always return FALSE on job_complete
- if seek does not succeed, don't mark the job failed, as the handle
must continue to be valid

Committed (I removed two unused fields from GnomeVFSSeekOpResult
(whence and offset). Thanks for the hard work :)

Umh, thanks for the excellent reviews :)