[Qemu-devel] [PATCH for-2.5] qga: Better mapping of SEEK_* in guest-file-seek

[Qemu-devel] [PATCH for-2.5] qga: Better mapping of SEEK_* in guest-file-seek

[Eric Blake]

Exposing OS-specific SEEK_ constants in our qapi was a mistake
(if the host has SEEK_CUR as 1, but the guest has it as 2, then
the semantics are unclear what should happen); if we had a time
machine, we would instead expose only a symbolic enum.  It's too
late to change the fact that we have an integer in qapi, but we
can at least document what mapping we want to enforce for all
qga clients (and luckily, it happens to be the mapping that both
Linux and Windows use), then fix the code to match that mapping.
It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qga/commands-posix.c | 19 ++++++++++++++++++-
 qga/commands-win32.c | 20 +++++++++++++++++++-
 qga/qapi-schema.json |  4 ++--
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ebd473..ed2b8e8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -523,17 +523,34 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
 }

 struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
-                                          int64_t whence, Error **errp)
+                                          int64_t whence_code, Error **errp)
 {
     GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
     GuestFileSeek *seek_data = NULL;
     FILE *fh;
     int ret;
+    int whence;

     if (!gfh) {
         return NULL;
     }

+    /* We stupidly exposed 'whence':'int' in our qapi */
+    switch (whence_code) {
+    case 0:
+        whence = SEEK_SET;
+        break;
+    case 1:
+        whence = SEEK_CUR;
+        break;
+    case 2:
+        whence = SEEK_END;
+        break;
+    default:
+        error_setg(errp, "invalid whence code %"PRId64, whence_code);
+        return NULL;
+    }
+
     fh = gfh->fh;
     ret = fseek(fh, offset, whence);
     if (ret == -1) {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 41f6dd9..b88786a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -382,7 +382,7 @@ done:
 }

 GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
-                                   int64_t whence, Error **errp)
+                                   int64_t whence_code, Error **errp)
 {
     GuestFileHandle *gfh;
     GuestFileSeek *seek_data;
@@ -390,11 +390,29 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
     LARGE_INTEGER new_pos, off_pos;
     off_pos.QuadPart = offset;
     BOOL res;
+    int whence;
+
     gfh = guest_file_handle_find(handle, errp);
     if (!gfh) {
         return NULL;
     }

+    /* We stupidly exposed 'whence':'int' in our qapi */
+    switch (whence_code) {
+    case 0:
+        whence = SEEK_SET;
+        break;
+    case 1:
+        whence = SEEK_CUR;
+        break;
+    case 2:
+        whence = SEEK_END;
+        break;
+    default:
+        error_setg(errp, "invalid whence code %"PRId64, whence_code);
+        return NULL;
+    }
+
     fh = gfh->fh;
     res = SetFilePointerEx(fh, off_pos, &new_pos, whence);
     if (!res) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 78362e0..01c9ee4 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -318,13 +318,13 @@
 #
 # Seek to a position in the file, as with fseek(), and return the
 # current file position afterward. Also encapsulates ftell()'s
-# functionality, just Set offset=0, whence=SEEK_CUR.
+# functionality, with offset=0 and whence=1.
 #
 # @handle: filehandle returned by guest-file-open
 #
 # @offset: bytes to skip over in the file stream
 #
-# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
+# @whence: 0 for SEEK_SET, 1 for SEEK_CUR, or 2 for SEEK_END
 #
 # Returns: @GuestFileSeek on success.
 #
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5] qga: Better mapping of SEEK_* in guest-file-seek

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On 11/24/2015 11:57 AM, Eric Blake wrote:
> Exposing OS-specific SEEK_ constants in our qapi was a mistake
> (if the host has SEEK_CUR as 1, but the guest has it as 2, then
> the semantics are unclear what should happen); if we had a time
> machine, we would instead expose only a symbolic enum.  It's too
> late to change the fact that we have an integer in qapi, but we
> can at least document what mapping we want to enforce for all
> qga clients (and luckily, it happens to be the mapping that both
> Linux and Windows use), then fix the code to match that mapping.
> It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qga/commands-posix.c | 19 ++++++++++++++++++-
>  qga/commands-win32.c | 20 +++++++++++++++++++-
>  qga/qapi-schema.json |  4 ++--
>  3 files changed, 39 insertions(+), 4 deletions(-)

Hmm, we probably ought to squash in:

diff --git i/tests/test-qga.c w/tests/test-qga.c
index 6473846..642dcb5 100644
--- i/tests/test-qga.c
+++ w/tests/test-qga.c
@@ -457,7 +457,7 @@ static void test_qga_file_ops(gconstpointer fix)
     cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
                           " 'arguments': { 'handle': %" PRId64 ", "
                           " 'offset': %d, 'whence': %d } }",
-                          id, 6, SEEK_SET);
+                          id, 6, 0);
     ret = qmp_fd(fixture->fd, cmd);
     qmp_assert_no_error(ret);
     val = qdict_get_qdict(ret, "return");


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5] qga: Better mapping of SEEK_* in guest-file-seek

[Marc-André Lureau]

Hi

On Tue, Nov 24, 2015 at 8:09 PM, Eric Blake <eblake@redhat.com> wrote:
> On 11/24/2015 11:57 AM, Eric Blake wrote:
>> Exposing OS-specific SEEK_ constants in our qapi was a mistake
>> (if the host has SEEK_CUR as 1, but the guest has it as 2, then
>> the semantics are unclear what should happen); if we had a time
>> machine, we would instead expose only a symbolic enum.  It's too
>> late to change the fact that we have an integer in qapi, but we
>> can at least document what mapping we want to enforce for all
>> qga clients (and luckily, it happens to be the mapping that both
>> Linux and Windows use), then fix the code to match that mapping.
>> It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  qga/commands-posix.c | 19 ++++++++++++++++++-
>>  qga/commands-win32.c | 20 +++++++++++++++++++-
>>  qga/qapi-schema.json |  4 ++--
>>  3 files changed, 39 insertions(+), 4 deletions(-)
>
> Hmm, we probably ought to squash in:
>
> diff --git i/tests/test-qga.c w/tests/test-qga.c
> index 6473846..642dcb5 100644
> --- i/tests/test-qga.c
> +++ w/tests/test-qga.c
> @@ -457,7 +457,7 @@ static void test_qga_file_ops(gconstpointer fix)
>      cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
>                            " 'arguments': { 'handle': %" PRId64 ", "
>                            " 'offset': %d, 'whence': %d } }",
> -                          id, 6, SEEK_SET);
> +                          id, 6, 0);
>      ret = qmp_fd(fixture->fd, cmd);
>      qmp_assert_no_error(ret);
>      val = qdict_get_qdict(ret, "return");
>
>

With that fix,

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH for-2.5] qga: Better mapping of SEEK_* in guest-file-seek

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/24/2015 11:57 AM, Eric Blake wrote:
>> Exposing OS-specific SEEK_ constants in our qapi was a mistake
>> (if the host has SEEK_CUR as 1, but the guest has it as 2, then
>> the semantics are unclear what should happen); if we had a time
>> machine, we would instead expose only a symbolic enum.  It's too
>> late to change the fact that we have an integer in qapi, but we
>> can at least document what mapping we want to enforce for all
>> qga clients (and luckily, it happens to be the mapping that both
>> Linux and Windows use), then fix the code to match that mapping.
>> It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.
>> 
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  qga/commands-posix.c | 19 ++++++++++++++++++-
>>  qga/commands-win32.c | 20 +++++++++++++++++++-
>>  qga/qapi-schema.json |  4 ++--
>>  3 files changed, 39 insertions(+), 4 deletions(-)
>
> Hmm, we probably ought to squash in:
>
> diff --git i/tests/test-qga.c w/tests/test-qga.c
> index 6473846..642dcb5 100644
> --- i/tests/test-qga.c
> +++ w/tests/test-qga.c
> @@ -457,7 +457,7 @@ static void test_qga_file_ops(gconstpointer fix)
>      cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
>                            " 'arguments': { 'handle': %" PRId64 ", "
>                            " 'offset': %d, 'whence': %d } }",
> -                          id, 6, SEEK_SET);
> +                          id, 6, 0);
>      ret = qmp_fd(fixture->fd, cmd);
>      qmp_assert_no_error(ret);
>      val = qdict_get_qdict(ret, "return");

Loss in readability, I'm afraid.

I think defining and using the obvious enum QGA_SEEK would help.

Re: [Qemu-devel] [PATCH for-2.5] qga: Better mapping of SEEK_* in guest-file-seek

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On 11/25/2015 01:11 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/24/2015 11:57 AM, Eric Blake wrote:
>>> Exposing OS-specific SEEK_ constants in our qapi was a mistake
>>> (if the host has SEEK_CUR as 1, but the guest has it as 2, then
>>> the semantics are unclear what should happen); if we had a time
>>> machine, we would instead expose only a symbolic enum.  It's too
>>> late to change the fact that we have an integer in qapi, but we
>>> can at least document what mapping we want to enforce for all
>>> qga clients (and luckily, it happens to be the mapping that both
>>> Linux and Windows use), then fix the code to match that mapping.
>>> It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  qga/commands-posix.c | 19 ++++++++++++++++++-
>>>  qga/commands-win32.c | 20 +++++++++++++++++++-
>>>  qga/qapi-schema.json |  4 ++--
>>>  3 files changed, 39 insertions(+), 4 deletions(-)
>>
>> Hmm, we probably ought to squash in:
>>
>> diff --git i/tests/test-qga.c w/tests/test-qga.c
>> index 6473846..642dcb5 100644
>> --- i/tests/test-qga.c
>> +++ w/tests/test-qga.c
>> @@ -457,7 +457,7 @@ static void test_qga_file_ops(gconstpointer fix)
>>      cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
>>                            " 'arguments': { 'handle': %" PRId64 ", "
>>                            " 'offset': %d, 'whence': %d } }",
>> -                          id, 6, SEEK_SET);
>> +                          id, 6, 0);
>>      ret = qmp_fd(fixture->fd, cmd);
>>      qmp_assert_no_error(ret);
>>      val = qdict_get_qdict(ret, "return");
> 
> Loss in readability, I'm afraid.
> 
> I think defining and using the obvious enum QGA_SEEK would help.

Okay, I'll do that for v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]