Re: [Qemu-devel] [RFC PATCH V3 2/5] qapi: add event helper functions

[Eric Blake <eblake@redhat.com>]

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

On 03/18/2014 11:16 PM, Wenchao Xia wrote:
> This file hold some functions that do not need to be generated.

s/hold/holds/

> 
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---
>  include/qapi/qmp-event.h |   25 ++++++++++++++++
>  qapi/Makefile.objs       |    1 +
>  qapi/qmp-event.c         |   71 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+), 0 deletions(-)
>  create mode 100644 include/qapi/qmp-event.h
>  create mode 100644 qapi/qmp-event.c
> 
> diff --git a/include/qapi/qmp-event.h b/include/qapi/qmp-event.h
> new file mode 100644
> index 0000000..fdf1a7f
> --- /dev/null
> +++ b/include/qapi/qmp-event.h
> @@ -0,0 +1,25 @@
> +/*
> + * QMP Event related
> + *
> + * Authors:
> + *  Wenchao Xia   <wenchaoqemu@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.

For the [L]GPL to work, someone must assert copyright.

> +++ b/qapi/qmp-event.c
> @@ -0,0 +1,71 @@
> +/*
> + * QMP Event related
> + *
> + * Authors:
> + *  Wenchao Xia   <wenchaoqemu@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.

Again, missing an actual use of the word "Copyright".

> +
> +typedef struct QMPEventFunctions {
> +    QMPEventFuncEmit emit;
> +} QMPEventFunctions;
> +
> +QMPEventFunctions qmp_event_functions;
> +
> +void qmp_event_set_func_emit(QMPEventFuncEmit emit)
> +{
> +    qmp_event_functions.emit = emit;
> +}
> +
> +QMPEventFuncEmit qmp_event_get_func_emit(void)
> +{
> +    return qmp_event_functions.emit;
> +}

Is this struct a bit overkill, or do you extend it to include other
fields later?


> +    err = qemu_gettimeofday(&tv);
> +    if (err < 0) {
> +        /* Put -1 to indicate failure of getting host time */
> +        tv.tv_sec = tv.tv_usec = -1;

Believe it or not, this is NOT portable.  Let's consider what happens
when tv_sec is int64_t and tv_usec is uint32_t.  Assignments happen
right to left, so tv_usec gets the unsigned value 0xffffffff, then since
all uint32_t values fit in int64_t, integer promotion says that the
value is 0-extended (not sign-extended), and tv_sec is NOT assigned -1.
 Solution: break this into two separate statements:

tv.tv_sec = -1;
tv.tv_usec = -1;

> +    }
> +
> +    obj = qobject_from_jsonf("{ 'seconds': %" PRId64 ", "
> +                                "'microseconds': %" PRId64 " }",
> +                                (int64_t) tv.tv_sec, (int64_t) tv.tv_usec);

Indentation is odd, but that's cosmetic.

-- 
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 --]