| Message ID | 1455053236-22735-1-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Eric Blake (2016-02-09 15:27:16) > Magic constants are a pain to use, especially when we run the > risk that our choice of '1' for QGA_SEEK_CUR might differ from > the host or guest's choice of SEEK_CUR. Better is to use an > enum value, via a qapi alternate type for back-compatibility. > > With this, > {"command":"guest-file-seek", "arguments":{"handle":1, > "offset":0, "whence":"cur"}} > becomes a synonym for the older > {"command":"guest-file-seek", "arguments":{"handle":1, > "offset":0, "whence":1}} > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > --- > v2: rebase on top of qapi work > --- > v3: rebase to latest > v2 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03565.html > v1 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05730.html > > qga/guest-agent-core.h | 9 ++------- > qga/commands-posix.c | 19 ++++++------------- > qga/commands-win32.c | 19 ++++++------------- > qga/commands.c | 21 +++++++++++++++++++++ > tests/test-qga.c | 9 ++++----- > qga/qapi-schema.json | 33 +++++++++++++++++++++++++++++++-- > 6 files changed, 70 insertions(+), 40 deletions(-) > > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h > index 238dc6b..0a49516 100644 > --- a/qga/guest-agent-core.h > +++ b/qga/guest-agent-core.h > @@ -12,16 +12,10 @@ > */ > #include "qapi/qmp/dispatch.h" > #include "qemu-common.h" > +#include "qga-qmp-commands.h" > > #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; > @@ -44,6 +38,7 @@ void ga_set_frozen(GAState *s); > void ga_unset_frozen(GAState *s); > const char *ga_fsfreeze_hook(GAState *s); > int64_t ga_get_fd_handle(GAState *s, Error **errp); > +int ga_parse_whence(GuestFileWhence *whence, Error **errp); > > #ifndef _WIN32 > void reopen_fd_to_null(int fd); > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 9589b2d..9f51fae 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -550,31 +550,24 @@ 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_code, Error **errp) > + GuestFileWhence *whence_code, > + Error **errp) > { > GuestFileHandle *gfh = guest_file_handle_find(handle, errp); > GuestFileSeek *seek_data = NULL; > FILE *fh; > int ret; > int whence; > + Error *err = NULL; > > 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); > + whence = ga_parse_whence(whence_code, &err); > + if (err) { > + error_propagate(errp, err); > return NULL; > } > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index cf0757c..2799f5f 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -385,7 +385,8 @@ done: > } > > GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, > - int64_t whence_code, Error **errp) > + GuestFileWhence *whence_code, > + Error **errp) > { > GuestFileHandle *gfh; > GuestFileSeek *seek_data; > @@ -394,6 +395,7 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, > off_pos.QuadPart = offset; > BOOL res; > int whence; > + Error *err = NULL; > > gfh = guest_file_handle_find(handle, errp); > if (!gfh) { > @@ -401,18 +403,9 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, > } > > /* 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); > + whence = ga_parse_whence(whence_code, &err); > + if (err) { > + error_propagate(errp, err); > return NULL; > } > > diff --git a/qga/commands.c b/qga/commands.c > index 5b56786..e091ee1 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -473,3 +473,24 @@ done: > > return ge; > } > + > +/* Convert GuestFileWhence (either a raw integer or an enum value) into > + * the guest's SEEK_ constants. */ > +int ga_parse_whence(GuestFileWhence *whence, Error **errp) > +{ > + /* Exploit the fact that we picked values to match QGA_SEEK_*. */ > + if (whence->type == QTYPE_QSTRING) { > + whence->type = QTYPE_QINT; > + whence->u.value = whence->u.name; > + } > + switch (whence->u.value) { > + case QGA_SEEK_SET: > + return SEEK_SET; > + case QGA_SEEK_CUR: > + return SEEK_CUR; > + case QGA_SEEK_END: > + return SEEK_END; > + } > + error_setg(errp, "invalid whence code %"PRId64, whence->u.value); > + return -1; > +} > diff --git a/tests/test-qga.c b/tests/test-qga.c > index 993b007..4d7542e 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -7,7 +7,6 @@ > > #include "libqtest.h" > #include "config-host.h" > -#include "qga/guest-agent-core.h" > > typedef struct { > char *test_dir; > @@ -451,8 +450,8 @@ static void test_qga_file_ops(gconstpointer fix) > /* seek */ > cmd = g_strdup_printf("{'execute': 'guest-file-seek'," > " 'arguments': { 'handle': %" PRId64 ", " > - " 'offset': %d, 'whence': %d } }", > - id, 6, QGA_SEEK_SET); > + " 'offset': %d, 'whence': '%s' } }", > + id, 6, "set"); > ret = qmp_fd(fixture->fd, cmd); > qmp_assert_no_error(ret); > val = qdict_get_qdict(ret, "return"); > @@ -544,8 +543,8 @@ static void test_qga_file_write_read(gconstpointer fix) > /* seek to 0 */ > cmd = g_strdup_printf("{'execute': 'guest-file-seek'," > " 'arguments': { 'handle': %" PRId64 ", " > - " 'offset': %d, 'whence': %d } }", > - id, 0, QGA_SEEK_SET); > + " 'offset': %d, 'whence': '%s' } }", > + id, 0, "set"); > ret = qmp_fd(fixture->fd, cmd); > qmp_assert_no_error(ret); > val = qdict_get_qdict(ret, "return"); > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 01c9ee4..c21f308 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -314,6 +314,34 @@ > 'data': { 'position': 'int', 'eof': 'bool' } } > > ## > +# @QGASeek: > +# > +# Symbolic names for use in @guest-file-seek > +# > +# @set: Set to the specified offset (same effect as 'whence':0) > +# @cur: Add offset to the current location (same effect as 'whence':1) > +# @end: Add offset to the end of the file (same effect as 'whence':2) > +# > +# Since: 2.6 > +## > +{ 'enum': 'QGASeek', 'data': [ 'set', 'cur', 'end' ] } > + > +## > +# @GuestFileWhence: > +# > +# Controls the meaning of offset to @guest-file-seek. > +# > +# @value: Integral value (0 for set, 1 for cur, 2 for end), available > +# for historical reasons, and might differ from the host's or > +# guest's SEEK_* values (since: 0.15) > +# @name: Symbolic name, and preferred interface > +# > +# Since: 2.6 > +## > +{ 'alternate': 'GuestFileWhence', > + 'data': { 'value': 'int', 'name': 'QGASeek' } } > + > +## > # @guest-file-seek: > # > # Seek to a position in the file, as with fseek(), and return the > @@ -324,14 +352,15 @@ > # > # @offset: bytes to skip over in the file stream > # > -# @whence: 0 for SEEK_SET, 1 for SEEK_CUR, or 2 for SEEK_END > +# @whence: Symbolic or numeric code for interpreting offset > # > # Returns: @GuestFileSeek on success. > # > # Since: 0.15.0 > ## > { 'command': 'guest-file-seek', > - 'data': { 'handle': 'int', 'offset': 'int', 'whence': 'int' }, > + 'data': { 'handle': 'int', 'offset': 'int', > + 'whence': 'GuestFileWhence' }, > 'returns': 'GuestFileSeek' } > > ## > -- > 2.5.0 >
Quoting Eric Blake (2016-02-09 15:27:16) > Magic constants are a pain to use, especially when we run the > risk that our choice of '1' for QGA_SEEK_CUR might differ from > the host or guest's choice of SEEK_CUR. Better is to use an > enum value, via a qapi alternate type for back-compatibility. > > With this, > {"command":"guest-file-seek", "arguments":{"handle":1, > "offset":0, "whence":"cur"}} > becomes a synonym for the older > {"command":"guest-file-seek", "arguments":{"handle":1, > "offset":0, "whence":1}} > > Signed-off-by: Eric Blake <eblake@redhat.com> Thanks, applied to qga tree: https://github.com/mdroth/qemu/commits/qga > > --- > v2: rebase on top of qapi work > --- > v3: rebase to latest > v2 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03565.html > v1 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05730.html > > qga/guest-agent-core.h | 9 ++------- > qga/commands-posix.c | 19 ++++++------------- > qga/commands-win32.c | 19 ++++++------------- > qga/commands.c | 21 +++++++++++++++++++++ > tests/test-qga.c | 9 ++++----- > qga/qapi-schema.json | 33 +++++++++++++++++++++++++++++++-- > 6 files changed, 70 insertions(+), 40 deletions(-) > > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h > index 238dc6b..0a49516 100644 > --- a/qga/guest-agent-core.h > +++ b/qga/guest-agent-core.h > @@ -12,16 +12,10 @@ > */ > #include "qapi/qmp/dispatch.h" > #include "qemu-common.h" > +#include "qga-qmp-commands.h" > > #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; > @@ -44,6 +38,7 @@ void ga_set_frozen(GAState *s); > void ga_unset_frozen(GAState *s); > const char *ga_fsfreeze_hook(GAState *s); > int64_t ga_get_fd_handle(GAState *s, Error **errp); > +int ga_parse_whence(GuestFileWhence *whence, Error **errp); > > #ifndef _WIN32 > void reopen_fd_to_null(int fd); > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 9589b2d..9f51fae 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -550,31 +550,24 @@ 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_code, Error **errp) > + GuestFileWhence *whence_code, > + Error **errp) > { > GuestFileHandle *gfh = guest_file_handle_find(handle, errp); > GuestFileSeek *seek_data = NULL; > FILE *fh; > int ret; > int whence; > + Error *err = NULL; > > 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); > + whence = ga_parse_whence(whence_code, &err); > + if (err) { > + error_propagate(errp, err); > return NULL; > } > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index cf0757c..2799f5f 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -385,7 +385,8 @@ done: > } > > GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, > - int64_t whence_code, Error **errp) > + GuestFileWhence *whence_code, > + Error **errp) > { > GuestFileHandle *gfh; > GuestFileSeek *seek_data; > @@ -394,6 +395,7 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, > off_pos.QuadPart = offset; > BOOL res; > int whence; > + Error *err = NULL; > > gfh = guest_file_handle_find(handle, errp); > if (!gfh) { > @@ -401,18 +403,9 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, > } > > /* 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); > + whence = ga_parse_whence(whence_code, &err); > + if (err) { > + error_propagate(errp, err); > return NULL; > } > > diff --git a/qga/commands.c b/qga/commands.c > index 5b56786..e091ee1 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -473,3 +473,24 @@ done: > > return ge; > } > + > +/* Convert GuestFileWhence (either a raw integer or an enum value) into > + * the guest's SEEK_ constants. */ > +int ga_parse_whence(GuestFileWhence *whence, Error **errp) > +{ > + /* Exploit the fact that we picked values to match QGA_SEEK_*. */ > + if (whence->type == QTYPE_QSTRING) { > + whence->type = QTYPE_QINT; > + whence->u.value = whence->u.name; > + } > + switch (whence->u.value) { > + case QGA_SEEK_SET: > + return SEEK_SET; > + case QGA_SEEK_CUR: > + return SEEK_CUR; > + case QGA_SEEK_END: > + return SEEK_END; > + } > + error_setg(errp, "invalid whence code %"PRId64, whence->u.value); > + return -1; > +} > diff --git a/tests/test-qga.c b/tests/test-qga.c > index 993b007..4d7542e 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -7,7 +7,6 @@ > > #include "libqtest.h" > #include "config-host.h" > -#include "qga/guest-agent-core.h" > > typedef struct { > char *test_dir; > @@ -451,8 +450,8 @@ static void test_qga_file_ops(gconstpointer fix) > /* seek */ > cmd = g_strdup_printf("{'execute': 'guest-file-seek'," > " 'arguments': { 'handle': %" PRId64 ", " > - " 'offset': %d, 'whence': %d } }", > - id, 6, QGA_SEEK_SET); > + " 'offset': %d, 'whence': '%s' } }", > + id, 6, "set"); > ret = qmp_fd(fixture->fd, cmd); > qmp_assert_no_error(ret); > val = qdict_get_qdict(ret, "return"); > @@ -544,8 +543,8 @@ static void test_qga_file_write_read(gconstpointer fix) > /* seek to 0 */ > cmd = g_strdup_printf("{'execute': 'guest-file-seek'," > " 'arguments': { 'handle': %" PRId64 ", " > - " 'offset': %d, 'whence': %d } }", > - id, 0, QGA_SEEK_SET); > + " 'offset': %d, 'whence': '%s' } }", > + id, 0, "set"); > ret = qmp_fd(fixture->fd, cmd); > qmp_assert_no_error(ret); > val = qdict_get_qdict(ret, "return"); > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 01c9ee4..c21f308 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -314,6 +314,34 @@ > 'data': { 'position': 'int', 'eof': 'bool' } } > > ## > +# @QGASeek: > +# > +# Symbolic names for use in @guest-file-seek > +# > +# @set: Set to the specified offset (same effect as 'whence':0) > +# @cur: Add offset to the current location (same effect as 'whence':1) > +# @end: Add offset to the end of the file (same effect as 'whence':2) > +# > +# Since: 2.6 > +## > +{ 'enum': 'QGASeek', 'data': [ 'set', 'cur', 'end' ] } > + > +## > +# @GuestFileWhence: > +# > +# Controls the meaning of offset to @guest-file-seek. > +# > +# @value: Integral value (0 for set, 1 for cur, 2 for end), available > +# for historical reasons, and might differ from the host's or > +# guest's SEEK_* values (since: 0.15) > +# @name: Symbolic name, and preferred interface > +# > +# Since: 2.6 > +## > +{ 'alternate': 'GuestFileWhence', > + 'data': { 'value': 'int', 'name': 'QGASeek' } } > + > +## > # @guest-file-seek: > # > # Seek to a position in the file, as with fseek(), and return the > @@ -324,14 +352,15 @@ > # > # @offset: bytes to skip over in the file stream > # > -# @whence: 0 for SEEK_SET, 1 for SEEK_CUR, or 2 for SEEK_END > +# @whence: Symbolic or numeric code for interpreting offset > # > # Returns: @GuestFileSeek on success. > # > # Since: 0.15.0 > ## > { 'command': 'guest-file-seek', > - 'data': { 'handle': 'int', 'offset': 'int', 'whence': 'int' }, > + 'data': { 'handle': 'int', 'offset': 'int', > + 'whence': 'GuestFileWhence' }, > 'returns': 'GuestFileSeek' } > > ## > -- > 2.5.0 >
On Tue, 9 Feb 2016 at 21:27, Eric Blake <eblake@redhat.com> wrote: > > Magic constants are a pain to use, especially when we run the > risk that our choice of '1' for QGA_SEEK_CUR might differ from > the host or guest's choice of SEEK_CUR. Better is to use an > enum value, via a qapi alternate type for back-compatibility. > > With this, > {"command":"guest-file-seek", "arguments":{"handle":1, > "offset":0, "whence":"cur"}} > becomes a synonym for the older > {"command":"guest-file-seek", "arguments":{"handle":1, > "offset":0, "whence":1}} > > Signed-off-by: Eric Blake <eblake@redhat.com> Hi; dragging up this patch from 2016 to say that Coverity has just noticed that there's some C undefined behaviour in it (CID 1421990): > +/* Convert GuestFileWhence (either a raw integer or an enum value) into > + * the guest's SEEK_ constants. */ > +int ga_parse_whence(GuestFileWhence *whence, Error **errp) > +{ > + /* Exploit the fact that we picked values to match QGA_SEEK_*. */ > + if (whence->type == QTYPE_QSTRING) { > + whence->type = QTYPE_QINT; > + whence->u.value = whence->u.name; Here whence->u.value and whence->u.name are two different fields in a union generated by QAPI: typedef enum QGASeek { QGA_SEEK_SET, QGA_SEEK_CUR, QGA_SEEK_END, QGA_SEEK__MAX, } QGASeek; struct GuestFileWhence { QType type; union { /* union tag is @type */ int64_t value; QGASeek name; } u; }; So u.value and u.name overlap in storage. The C standard says that this assignment is only valid if the overlap is exact and the two objects have qualified or unqualified versions of a compatible type. In this case the enum type is likely not the same size as an int64_t, and so we have undefined behaviour. I guess to fix this we need to copy via a local variable (with a comment so nobody helpfully optimizes the local away again in future)... > + } > + switch (whence->u.value) { > + case QGA_SEEK_SET: > + return SEEK_SET; > + case QGA_SEEK_CUR: > + return SEEK_CUR; > + case QGA_SEEK_END: > + return SEEK_END; > + } > + error_setg(errp, "invalid whence code %"PRId64, whence->u.value); > + return -1; > +} thanks -- PMM
On 3/20/20 8:49 AM, Peter Maydell wrote: > On Tue, 9 Feb 2016 at 21:27, Eric Blake <eblake@redhat.com> wrote: >> >> Magic constants are a pain to use, especially when we run the >> risk that our choice of '1' for QGA_SEEK_CUR might differ from >> the host or guest's choice of SEEK_CUR. Better is to use an >> enum value, via a qapi alternate type for back-compatibility. >> > Hi; dragging up this patch from 2016 to say that Coverity > has just noticed that there's some C undefined behaviour > in it (CID 1421990): Wow, took us a long time to find that! > >> +/* Convert GuestFileWhence (either a raw integer or an enum value) into >> + * the guest's SEEK_ constants. */ >> +int ga_parse_whence(GuestFileWhence *whence, Error **errp) >> +{ >> + /* Exploit the fact that we picked values to match QGA_SEEK_*. */ >> + if (whence->type == QTYPE_QSTRING) { >> + whence->type = QTYPE_QINT; >> + whence->u.value = whence->u.name; > > Here whence->u.value and whence->u.name are two different > fields in a union generated by QAPI: > > typedef enum QGASeek { > QGA_SEEK_SET, > QGA_SEEK_CUR, > QGA_SEEK_END, > QGA_SEEK__MAX, > } QGASeek; > > struct GuestFileWhence { > QType type; > union { /* union tag is @type */ > int64_t value; > QGASeek name; > } u; > }; > > So u.value and u.name overlap in storage. The C standard > says that this assignment is only valid if the overlap is > exact and the two objects have qualified or unqualified > versions of a compatible type. In this case the enum > type is likely not the same size as an int64_t, and so > we have undefined behaviour. You are (well, Coverity is) absolutely right! Patch coming up.
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h index 238dc6b..0a49516 100644 --- a/qga/guest-agent-core.h +++ b/qga/guest-agent-core.h @@ -12,16 +12,10 @@ */ #include "qapi/qmp/dispatch.h" #include "qemu-common.h" +#include "qga-qmp-commands.h" #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; @@ -44,6 +38,7 @@ void ga_set_frozen(GAState *s); void ga_unset_frozen(GAState *s); const char *ga_fsfreeze_hook(GAState *s); int64_t ga_get_fd_handle(GAState *s, Error **errp); +int ga_parse_whence(GuestFileWhence *whence, Error **errp); #ifndef _WIN32 void reopen_fd_to_null(int fd); diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 9589b2d..9f51fae 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -550,31 +550,24 @@ 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_code, Error **errp) + GuestFileWhence *whence_code, + Error **errp) { GuestFileHandle *gfh = guest_file_handle_find(handle, errp); GuestFileSeek *seek_data = NULL; FILE *fh; int ret; int whence; + Error *err = NULL; 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); + whence = ga_parse_whence(whence_code, &err); + if (err) { + error_propagate(errp, err); return NULL; } diff --git a/qga/commands-win32.c b/qga/commands-win32.c index cf0757c..2799f5f 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -385,7 +385,8 @@ done: } GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, - int64_t whence_code, Error **errp) + GuestFileWhence *whence_code, + Error **errp) { GuestFileHandle *gfh; GuestFileSeek *seek_data; @@ -394,6 +395,7 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, off_pos.QuadPart = offset; BOOL res; int whence; + Error *err = NULL; gfh = guest_file_handle_find(handle, errp); if (!gfh) { @@ -401,18 +403,9 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset, } /* 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); + whence = ga_parse_whence(whence_code, &err); + if (err) { + error_propagate(errp, err); return NULL; } diff --git a/qga/commands.c b/qga/commands.c index 5b56786..e091ee1 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -473,3 +473,24 @@ done: return ge; } + +/* Convert GuestFileWhence (either a raw integer or an enum value) into + * the guest's SEEK_ constants. */ +int ga_parse_whence(GuestFileWhence *whence, Error **errp) +{ + /* Exploit the fact that we picked values to match QGA_SEEK_*. */ + if (whence->type == QTYPE_QSTRING) { + whence->type = QTYPE_QINT; + whence->u.value = whence->u.name; + } + switch (whence->u.value) { + case QGA_SEEK_SET: + return SEEK_SET; + case QGA_SEEK_CUR: + return SEEK_CUR; + case QGA_SEEK_END: + return SEEK_END; + } + error_setg(errp, "invalid whence code %"PRId64, whence->u.value); + return -1; +} diff --git a/tests/test-qga.c b/tests/test-qga.c index 993b007..4d7542e 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -7,7 +7,6 @@ #include "libqtest.h" #include "config-host.h" -#include "qga/guest-agent-core.h" typedef struct { char *test_dir; @@ -451,8 +450,8 @@ static void test_qga_file_ops(gconstpointer fix) /* seek */ cmd = g_strdup_printf("{'execute': 'guest-file-seek'," " 'arguments': { 'handle': %" PRId64 ", " - " 'offset': %d, 'whence': %d } }", - id, 6, QGA_SEEK_SET); + " 'offset': %d, 'whence': '%s' } }", + id, 6, "set"); ret = qmp_fd(fixture->fd, cmd); qmp_assert_no_error(ret); val = qdict_get_qdict(ret, "return"); @@ -544,8 +543,8 @@ static void test_qga_file_write_read(gconstpointer fix) /* seek to 0 */ cmd = g_strdup_printf("{'execute': 'guest-file-seek'," " 'arguments': { 'handle': %" PRId64 ", " - " 'offset': %d, 'whence': %d } }", - id, 0, QGA_SEEK_SET); + " 'offset': %d, 'whence': '%s' } }", + id, 0, "set"); ret = qmp_fd(fixture->fd, cmd); qmp_assert_no_error(ret); val = qdict_get_qdict(ret, "return"); diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 01c9ee4..c21f308 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -314,6 +314,34 @@ 'data': { 'position': 'int', 'eof': 'bool' } } ## +# @QGASeek: +# +# Symbolic names for use in @guest-file-seek +# +# @set: Set to the specified offset (same effect as 'whence':0) +# @cur: Add offset to the current location (same effect as 'whence':1) +# @end: Add offset to the end of the file (same effect as 'whence':2) +# +# Since: 2.6 +## +{ 'enum': 'QGASeek', 'data': [ 'set', 'cur', 'end' ] } + +## +# @GuestFileWhence: +# +# Controls the meaning of offset to @guest-file-seek. +# +# @value: Integral value (0 for set, 1 for cur, 2 for end), available +# for historical reasons, and might differ from the host's or +# guest's SEEK_* values (since: 0.15) +# @name: Symbolic name, and preferred interface +# +# Since: 2.6 +## +{ 'alternate': 'GuestFileWhence', + 'data': { 'value': 'int', 'name': 'QGASeek' } } + +## # @guest-file-seek: # # Seek to a position in the file, as with fseek(), and return the @@ -324,14 +352,15 @@ # # @offset: bytes to skip over in the file stream # -# @whence: 0 for SEEK_SET, 1 for SEEK_CUR, or 2 for SEEK_END +# @whence: Symbolic or numeric code for interpreting offset # # Returns: @GuestFileSeek on success. # # Since: 0.15.0 ## { 'command': 'guest-file-seek', - 'data': { 'handle': 'int', 'offset': 'int', 'whence': 'int' }, + 'data': { 'handle': 'int', 'offset': 'int', + 'whence': 'GuestFileWhence' }, 'returns': 'GuestFileSeek' } ##
Magic constants are a pain to use, especially when we run the risk that our choice of '1' for QGA_SEEK_CUR might differ from the host or guest's choice of SEEK_CUR. Better is to use an enum value, via a qapi alternate type for back-compatibility. With this, {"command":"guest-file-seek", "arguments":{"handle":1, "offset":0, "whence":"cur"}} becomes a synonym for the older {"command":"guest-file-seek", "arguments":{"handle":1, "offset":0, "whence":1}} Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: rebase on top of qapi work --- v3: rebase to latest v2 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03565.html v1 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05730.html qga/guest-agent-core.h | 9 ++------- qga/commands-posix.c | 19 ++++++------------- qga/commands-win32.c | 19 ++++++------------- qga/commands.c | 21 +++++++++++++++++++++ tests/test-qga.c | 9 ++++----- qga/qapi-schema.json | 33 +++++++++++++++++++++++++++++++-- 6 files changed, 70 insertions(+), 40 deletions(-)