* Re: [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek
2016-02-09 21:27 [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek Eric Blake
@ 2016-02-09 22:08 ` Michael Roth
2016-02-25 2:06 ` Michael Roth
2020-03-20 13:49 ` Peter Maydell
2 siblings, 0 replies; 5+ messages in thread
From: Michael Roth @ 2016-02-09 22:08 UTC (permalink / raw)
To: Eric Blake, qemu-devel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek
2016-02-09 21:27 [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek Eric Blake
2016-02-09 22:08 ` Michael Roth
@ 2016-02-25 2:06 ` Michael Roth
2020-03-20 13:49 ` Peter Maydell
2 siblings, 0 replies; 5+ messages in thread
From: Michael Roth @ 2016-02-25 2:06 UTC (permalink / raw)
To: Eric Blake, qemu-devel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek
2016-02-09 21:27 [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek Eric Blake
2016-02-09 22:08 ` Michael Roth
2016-02-25 2:06 ` Michael Roth
@ 2020-03-20 13:49 ` Peter Maydell
2020-03-20 14:57 ` Eric Blake
2 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2020-03-20 13:49 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Developers, Michael Roth
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek
2020-03-20 13:49 ` Peter Maydell
@ 2020-03-20 14:57 ` Eric Blake
0 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2020-03-20 14:57 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Michael Roth
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 5+ messages in thread