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

[Michael Roth]

Quoting Eric Blake (2015-11-25 11:37:15)
> 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.
> 
> In the future, we may wish to move our QGA_SEEK_* constants into
> qga/qapi-schema.json, along with updating the schema to take an
> alternate type (either the integer, or the string value of the
> enum name) - but that's too much risk during hard freeze.
> 
> Signed-off-by: Eric Blake <address@hidden>

Thanks, applied to qga tree:

  https://github.com/mdroth/qemu/commits/qga

> 
> ---
> v2: rebase to mdroth/qga, add QGA_SEEK_* constants instead of magic
> numbers
> ---
>  qga/commands-posix.c   | 19 ++++++++++++++++++-
>  qga/commands-win32.c   | 20 +++++++++++++++++++-
>  qga/guest-agent-core.h |  7 +++++++
>  qga/qapi-schema.json   |  4 ++--
>  tests/test-qga.c       |  5 +++--
>  5 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index cf1d7ec..c2ff970 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -553,17 +553,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 QGA_SEEK_SET:
> +        whence = SEEK_SET;
> +        break;
> +    case QGA_SEEK_CUR:
> +        whence = SEEK_CUR;
> +        break;
> +    case QGA_SEEK_END:
> +        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..0654fe4 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 QGA_SEEK_SET:
> +        whence = SEEK_SET;
> +        break;
> +    case QGA_SEEK_CUR:
> +        whence = SEEK_CUR;
> +        break;
> +    case QGA_SEEK_END:
> +        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/guest-agent-core.h b/qga/guest-agent-core.h
> index e92c6ab..238dc6b 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -15,6 +15,13 @@
> 
>  #define QGA_READ_COUNT_DEFAULT 4096
> 
> +/* Mapping of whence codes used by guest-file-seek. */
> +enum {
> +    QGA_SEEK_SET = 0,
> +    QGA_SEEK_CUR = 1,
> +    QGA_SEEK_END = 2,
> +};
> +
>  typedef struct GAState GAState;
>  typedef struct GACommandState GACommandState;
>  extern GAState *ga_state;
> 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.
>  #
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 3b99d9d..e6a84d1 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -13,6 +13,7 @@
> 
>  #include "libqtest.h"
>  #include "config-host.h"
> +#include "qga/guest-agent-core.h"
> 
>  typedef struct {
>      char *test_dir;
> @@ -457,7 +458,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, QGA_SEEK_SET);
>      ret = qmp_fd(fixture->fd, cmd);
>      qmp_assert_no_error(ret);
>      val = qdict_get_qdict(ret, "return");
> @@ -550,7 +551,7 @@ static void test_qga_file_write_read(gconstpointer fix)
>      cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
>                            " 'arguments': { 'handle': %" PRId64 ", "
>                            " 'offset': %d, 'whence': %d } }",
> -                          id, 0, SEEK_SET);
> +                          id, 0, QGA_SEEK_SET);
>      ret = qmp_fd(fixture->fd, cmd);
>      qmp_assert_no_error(ret);
>      val = qdict_get_qdict(ret, "return");
> -- 
> 2.4.3
>