Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Alex Bennée]

Nikos Filippakis  writes:

> Hello everyone! I am interested in getting to know the codebase a little 
> better
> so that I can eventually apply for a GSOC position.
> This is my first attempt at posting a patch to a mailing list, any feedback
> is greatly appreciated.

OK first things first this sort of meta comment belongs in the cover
letter. However for a single patch you may want to put such things below
the --- in the commit message as that will get stripped when the
maintainer eventually applies the patch. Otherwise your meta-comments
will end up in the version log ;-)

You'll see people use the --- area to keep version notes as patches go
through revisions.

>
> Signed-off-by: Nikos Filippakis 
> ---
>  net/net.c | 17 -
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index aebf753..79e9d7c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const 
> uint8_t *buf, int size)
>  static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
> int iovcnt, unsigned flags)
>  {
> -uint8_t buf[NET_BUFSIZE];
>  uint8_t *buffer;
>  size_t offset;
> +ssize_t ret;

With that said your comment needs to explain why you've just made the
change. I see NET_BUFSIZE is quite large so maybe this should be a
clean-up across the rest of the code-base, what's so special about this
function? Have you measured any difference in performance?

>
>  if (iovcnt == 1) {
>  buffer = iov[0].iov_base;
>  offset = iov[0].iov_len;
>  } else {
> -buffer = buf;
> -offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
> +buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
> +offset = iov_to_buf(iov, iovcnt, 0, buffer,
> +NET_BUFSIZE * sizeof(uint8_t));
>  }
>
>  if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
> -return nc->info->receive_raw(nc, buffer, offset);
> +ret = nc->info->receive_raw(nc, buffer, offset);
>  } else {
> -return nc->info->receive(nc, buffer, offset);
> +ret = nc->info->receive(nc, buffer, offset);
>  }
> +
> +if (iovcnt != 1) {
> +g_free(buffer);
> +}

This is a short function so you can get away with it but this sort of
logic can be confusing ("The iovec count was 1 therefor I should have
allocated a buffer" vs "I have an allocated buffer"). In general you
should know the various g_* functions tolerate NULLs well so maybe you
can structure the code differently (skipping the details ;-):

uint8_t *buffer, *dynbuf = NULL;

if (iovcnt == 1)
{
  buffer = ...
} else {
  buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
  ...
}
...

g_free(dynbuf)

> +
> +return ret;
>  }
>
>  ssize_t qemu_deliver_packet_iov(NetClientState *sender,


--
Alex Bennée

Re: [Qemu-devel] qemu AIO worker threads change causes Guest OS hangup

[Huaicheng Li (coperd)]

> On Mar 1, 2016, at 3:01 PM, Paolo Bonzini  wrote:
> 
> This is done
> because the worker threads only care about the queued request list, not
> about active or completed requests.

Do you think it would be useful to add an API for inserting one request back 
to the queued list? For example, In case of request failure, we can insert it 
back to the list for re-handling according to some rule before returning it 
directly 
to guest os. 

Best,
Huaicheng

Re: [Qemu-devel] qemu AIO worker threads change causes Guest OS hangup

[Huaicheng Li (coperd)]

> On Mar 1, 2016, at 3:34 PM, Stefan Hajnoczi  wrote:
> 
> Have you seen Linux Documentation/device-mapper/delay.txt?
> 
> You could set up a loopback block device and put the device-mapper delay 
> target on top to simulate latency.


I’m working on one idea to emulate the latency of SSD read/write, 
which is *dynamically* changing according to the status of emulated 
flash media. Thanks for the suggestion.

[Qemu-devel] [PATCH] kvm/irqchip: use bitmap utility for gsi tracking

[Wei Yang]

By using utilities in bitops and bitmap, this patch tries to make it more
friendly to audience. No functional change.

Signed-off-by: Wei Yang 
---
 kvm-all.c | 34 ++
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index bd9e764..ed3f4a2 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -90,7 +90,7 @@ struct KVMState
 #ifdef KVM_CAP_IRQ_ROUTING
 struct kvm_irq_routing *irq_routes;
 int nr_allocated_irq_routes;
-uint32_t *used_gsi_bitmap;
+unsigned long *used_gsi_bitmap;
 unsigned int gsi_count;
 QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
 #endif
@@ -951,12 +951,12 @@ typedef struct KVMMSIRoute {
 
 static void set_gsi(KVMState *s, unsigned int gsi)
 {
-s->used_gsi_bitmap[gsi / 32] |= 1U << (gsi % 32);
+set_bit(gsi, s->used_gsi_bitmap);
 }
 
 static void clear_gsi(KVMState *s, unsigned int gsi)
 {
-s->used_gsi_bitmap[gsi / 32] &= ~(1U << (gsi % 32));
+clear_bit(gsi, s->used_gsi_bitmap);
 }
 
 void kvm_init_irq_routing(KVMState *s)
@@ -965,17 +965,9 @@ void kvm_init_irq_routing(KVMState *s)
 
 gsi_count = kvm_check_extension(s, KVM_CAP_IRQ_ROUTING) - 1;
 if (gsi_count > 0) {
-unsigned int gsi_bits, i;
-
 /* Round up so we can search ints using ffs */
-gsi_bits = ALIGN(gsi_count, 32);
-s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);
+s->used_gsi_bitmap = bitmap_new(gsi_count);
 s->gsi_count = gsi_count;
-
-/* Mark any over-allocated bits as already in use */
-for (i = gsi_count; i < gsi_bits; i++) {
-set_gsi(s, i);
-}
 }
 
 s->irq_routes = g_malloc0(sizeof(*s->irq_routes));
@@ -1105,9 +1097,7 @@ static void kvm_flush_dynamic_msi_routes(KVMState *s)
 
 static int kvm_irqchip_get_virq(KVMState *s)
 {
-uint32_t *word = s->used_gsi_bitmap;
-int max_words = ALIGN(s->gsi_count, 32) / 32;
-int i, zeroes;
+int next_virq;
 
 /*
  * PIC and IOAPIC share the first 16 GSI numbers, thus the available
@@ -1120,16 +1110,12 @@ static int kvm_irqchip_get_virq(KVMState *s)
 }
 
 /* Return the lowest unused GSI in the bitmap */
-for (i = 0; i < max_words; i++) {
-zeroes = ctz32(~word[i]);
-if (zeroes == 32) {
-continue;
-}
-
-return zeroes + i * 32;
+next_virq = find_first_zero_bit(s->used_gsi_bitmap, s->gsi_count);
+if (next_virq >= s->gsi_count) {
+return -ENOSPC;
+} else {
+return next_virq;
 }
-return -ENOSPC;
-
 }
 
 static KVMMSIRoute *kvm_lookup_msi_route(KVMState *s, MSIMessage msg)
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 2/4] qapi-schema.json: Add power and keypad equal keys

[Programmingkid]

On Mar 5, 2016, at 7:03 PM, Eric Blake wrote:

> On 03/04/2016 10:17 PM, Programmingkid wrote:
>> Add the power and keypad equal keys. These keys are found on a real Macintosh
>> keyboard.
>> 
>> Signed-off-by: John Arbuckle 
>> ---
>> qapi-schema.json | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake 
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Thank you.

Re: [Qemu-devel] [PATCH v3 1/4] adb-keys.h: initial commit

[Programmingkid]

On Mar 5, 2016, at 7:02 PM, Eric Blake wrote:

> On 03/04/2016 10:15 PM, Programmingkid wrote:
>> This commit implements the adb-keys.h file. It holds information on adb 
>> keycode
>> values.
>> 
>> Signed-off-by: John Arbuckle 
>> ---
>> Changed name of file from MacKeys.h to adb-keys.h.
>> Changed name of constants from MAC_KEYS_ to ADB_KEYS_. 
>> 
>> include/hw/input/adb-keys.h | 160 
>> 
>> 1 file changed, 160 insertions(+)
>> create mode 100644 include/hw/input/adb-keys.h
> 
> Thanks; you're getting closer at decent submission style.  Your
> threading is still broken, though (the mail is missing an 'In-Reply-To:'
> header that refers to 
> from the 0/4 cover letter); you can see this in the list archives:
> https://lists.gnu.org/archive/html/qemu-devel/2016-03/threads.html#01180
> 
> Note how your messages each appear as top-level threads, while most
> other series are properly threaded.

I guess there is still room for improvement. :)

> 
>> 
>> diff --git a/include/hw/input/adb-keys.h b/include/hw/input/adb-keys.h
>> new file mode 100644
>> index 000..6e009ee
>> --- /dev/null
>> +++ b/include/hw/input/adb-keys.h
>> @@ -0,0 +1,160 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
> 
> Fabrice did not write this file.  You're probably better off replacing
> this line wholesale with a Copyright 2016 and claiming copyright
> yourself, since it is new material from you that is not substantially
> copied from some existing file.

Ok. 

> 
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
> 
> Any reason you chose the weaker BSD license instead of the
> project-default of GPLv2+?

I just used the text that was in vl.c. I thought all files in QEMU were using 
GPLv2. 

> 
>> +/*
>> + *  adb-keys.h
>> + *
>> + *  Provides a enum of all the Macintosh keycodes.
> 
> s/a enum/an enum/

Good catch.

> 
> Can you provide a URL to the source document that you used for coming up
> with this list, so that we can better double-check your work?  (Do it as
> a comment in this file, if you have one)

http://stackoverflow.com/questions/3202629/where-can-i-find-a-list-of-mac-virtual-key-codes

> 
>> + *  Note: keys like Power, volume related, and eject are handled at a lower
>> + *level and are not available to QEMU. That doesn't mean we can't
>> + *substitute one key for another. The function keys like F1 make a 
>> good
>> + *substitute for these keys. This can be done in the GTK, SDL, or 
>> Cocoa
>> + *code.
>> + */
>> +
>> +#ifndef __ADBKEYS__
>> +#define __ADBKEYS__
> 
> The name '__ADBKEYS__' is reserved for the compiler's use; you risk a
> future collision for no good reason.  It's fine to use 'ADB_KEYS_H' as
> your witness, better matching your filename and staying out of reserved
> namespace.

Ok.

Re: [Qemu-devel] [PATCH v3 1/4] adb-keys.h: initial commit

[Eric Blake]

On 03/04/2016 10:15 PM, Programmingkid wrote:
> This commit implements the adb-keys.h file. It holds information on adb 
> keycode
> values.
> 
> Signed-off-by: John Arbuckle 
> ---
> Changed name of file from MacKeys.h to adb-keys.h.
> Changed name of constants from MAC_KEYS_ to ADB_KEYS_. 
> 
>  include/hw/input/adb-keys.h | 160 
> 
>  1 file changed, 160 insertions(+)
>  create mode 100644 include/hw/input/adb-keys.h

Thanks; you're getting closer at decent submission style.  Your
threading is still broken, though (the mail is missing an 'In-Reply-To:'
header that refers to 
from the 0/4 cover letter); you can see this in the list archives:
https://lists.gnu.org/archive/html/qemu-devel/2016-03/threads.html#01180

Note how your messages each appear as top-level threads, while most
other series are properly threaded.

> 
> diff --git a/include/hw/input/adb-keys.h b/include/hw/input/adb-keys.h
> new file mode 100644
> index 000..6e009ee
> --- /dev/null
> +++ b/include/hw/input/adb-keys.h
> @@ -0,0 +1,160 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard

Fabrice did not write this file.  You're probably better off replacing
this line wholesale with a Copyright 2016 and claiming copyright
yourself, since it is new material from you that is not substantially
copied from some existing file.

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:

Any reason you chose the weaker BSD license instead of the
project-default of GPLv2+?

> +/*
> + *  adb-keys.h
> + *
> + *  Provides a enum of all the Macintosh keycodes.

s/a enum/an enum/

Can you provide a URL to the source document that you used for coming up
with this list, so that we can better double-check your work?  (Do it as
a comment in this file, if you have one)

> + *  Note: keys like Power, volume related, and eject are handled at a lower
> + *level and are not available to QEMU. That doesn't mean we can't
> + *substitute one key for another. The function keys like F1 make a 
> good
> + *substitute for these keys. This can be done in the GTK, SDL, or 
> Cocoa
> + *code.
> + */
> +
> +#ifndef __ADBKEYS__
> +#define __ADBKEYS__

The name '__ADBKEYS__' is reserved for the compiler's use; you risk a
future collision for no good reason.  It's fine to use 'ADB_KEYS_H' as
your witness, better matching your filename and staying out of reserved
namespace.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 2/4] qapi-schema.json: Add power and keypad equal keys

[Eric Blake]

On 03/04/2016 10:17 PM, Programmingkid wrote:
> Add the power and keypad equal keys. These keys are found on a real Macintosh
> keyboard.
> 
> Signed-off-by: John Arbuckle 
> ---
>  qapi-schema.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v13 18/18] qapi: Change visit_type_FOO() to no longer return partial objects

[Eric Blake]

Returning a partial object on error is an invitation for a careless
caller to leak memory.  As no one outside the testsuite was actually
relying on these semantics, it is cleaner to just document and
guarantee that ALL pointer-based visit_type_FOO() functions always
leave a safe value in *obj during an input visitor (either the new
object on success, or NULL if an error is encountered), so callers
can now unconditionally use qapi_free_FOO() to clean up regardless
of whether an error occurred.

The decision is done by enhancing qapi-visit-core to return true
for input visitors (the callbacks themselves do not need
modification); since we've documented that visit_end_* must be
called after any successful visit_start_*, that is a sufficient
point for knowing that something was allocated during start.

Note that we still leave *obj unchanged after a scalar-based
visit_type_FOO(); I did not feel like auditing all uses of
visit_type_Enum() to see if the callers would tolerate a specific
sentinel value (not to mention having to decide whether it would
be better to use 0 or ENUM__MAX as that sentinel).

Signed-off-by: Eric Blake 

---
v13: rebase to latest, documentation tweaks
v12: rebase to latest, don't modify callback signatures, use newer
approach for detecting input visitors, avoid 'allocated' boolean
[no v10, v11]
v9: fix bug in use of errp
v8: rebase to earlier changes
v7: rebase to earlier changes, enhance commit message, also fix
visit_type_str() and visit_type_any()
v6: rebase on top of earlier doc and formatting improvements, mention
that *obj can be uninitialized on entry to an input visitor, rework
semantics to keep valgrind happy on uninitialized input, break some
long lines
---
 include/qapi/visitor.h | 42 ++
 include/qapi/visitor-impl.h|  8 +---
 scripts/qapi-visit.py  | 25 +
 qapi/qapi-visit-core.c | 41 ++---
 tests/test-qmp-commands.c  | 13 ++---
 tests/test-qmp-input-strict.c  | 19 ---
 tests/test-qmp-input-visitor.c | 10 ++
 docs/qapi-code-gen.txt | 10 --
 8 files changed, 106 insertions(+), 62 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 8d4837d..5b7c86e 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -66,14 +66,16 @@
  * }' if an error is encountered on "value" (or to have the visitor
  * core auto-generate the nicer name).
  *
- * FIXME: At present, input visitors may allocate an incomplete *@obj
- * even when visit_type_FOO() reports an error.  Using an output
- * visitor with an incomplete object has undefined behavior; callers
- * must call qapi_free_FOO() (which uses the dealloc visitor, and
- * safely handles an incomplete object) to avoid a memory leak.
+ * If an error is detected during visit_type_FOO() with an input
+ * visitor, then *@obj will be NULL for pointer types, and left
+ * unchanged for scalar types.  Using an output visitor with an
+ * incomplete object has undefined behavior (other than a special case
+ * for visit_type_str() treating NULL like ""), while the dealloc
+ * visitor safely handles incomplete objects.
  *
- * Likewise, the QAPI object types (structs, unions, and alternates)
- * have a generated function in qapi-visit.h compatible with:
+ * Additionally, the QAPI object types (structs, unions, and
+ * alternates) have a generated function in qapi-visit.h compatible
+ * with:
  *
  * void visit_type_FOO_members(Visitor *v, FOO *obj, Error **errp);
  *
@@ -105,7 +107,6 @@
  *  v = ...obtain input visitor...
  *  visit_type_Foo(v, NULL, , );
  *  if (err) {
- *  qapi_free_Foo(f);
  *  ...handle error...
  *  } else {
  *  ...use f...
@@ -113,7 +114,6 @@
  *  v = ...obtain input visitor...
  *  visit_type_FooList(v, NULL, , );
  *  if (err) {
- *  qapi_free_FooList(l);
  *  ...handle error...
  *  } else {
  *  while (l) {
@@ -257,8 +257,14 @@ void visit_check_struct(Visitor *v, Error **errp);
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.  Destroying the visitor may
  * behave as if this was implicitly called.
+ *
+ * Returns true if this is an input visitor (that is, an allocation
+ * occurred during visit_start_struct() if obj was non-NULL).  The
+ * caller can use this, along with tracking whether a local error
+ * occurred in the meantime, to decide when to undo allocation before
+ * returning control from a visit_type_FOO() function.
  */
-void visit_end_struct(Visitor *v);
+bool visit_end_struct(Visitor *v);


 /* === Visiting lists */
@@ -314,8 +320,14 @@ GenericList *visit_next_list(Visitor *v, GenericList 
*tail, size_t size);
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.  Destroying the visitor may
  * behave as if this was implicitly 

[Qemu-devel] [PATCH v13 07/18] qapi: Document visitor interfaces, add assertions

[Eric Blake]

The visitor interface for mapping between QObject/QemuOpts/string
and QAPI is scandalously under-documented, making changes to visitor
core, individual visitors, and users of visitors difficult to
coordinate.  Among other questions: when is it safe to pass NULL,
vs. when a string must be provided; which visitors implement which
callbacks; the difference between concrete and virtual visits.

Correct this by retrofitting proper contracts, and document where some
of the interface warts remain (for example, we may want to modify
visit_end_* to require the same 'obj' as the visit_start counterpart,
so the dealloc visitor can be simplified).  Later patches in this
series will tackle some, but not all, of these warts.

Add assertions to (partially) enforce the contract.  Some of these
were only made possible by recent cleanup commits.

Signed-off-by: Eric Blake 

---
v13: minor wording tweaks for consistency
v12: major rework based on Markus' comments, drop R-b
[no v10, v11]
v9: no change
v8: rebase to 'name' motion
v7: retitle; more wording changes, add asserts to enforce the
wording, place later in series to rebase on fixes that would
otherwise trip the new assertions
v6: mention that input visitors blindly assign over *obj; wording
improvements
---
 include/qapi/visitor.h   | 433 +--
 include/qapi/visitor-impl.h  |  42 +++-
 include/qapi/dealloc-visitor.h   |   4 +
 include/qapi/opts-visitor.h  |   4 +
 include/qapi/qmp-input-visitor.h |   8 +
 include/qapi/string-input-visitor.h  |   4 +
 include/qapi/string-output-visitor.h |   4 +
 qapi/qapi-visit-core.c   |  19 +-
 8 files changed, 494 insertions(+), 24 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1000da2..093bc70 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -17,8 +17,185 @@
 #include "qemu/typedefs.h"
 #include "qapi/qmp/qobject.h"

+/*
+ * The QAPI schema defines both a set of C data types, and a QMP wire
+ * format.  A QAPI object is formed as a directed acyclic graph of
+ * QAPI values.  QAPI also generates visitor functions to walk these
+ * graphs.  This file represents the interface for doing work at each
+ * point of a QAPI graph; it can also be used for a virtual walk,
+ * where there is no actual QAPI C struct.
+ *
+ * There are three kinds of visitor classes: input visitors parse an
+ * external representation and allocate the corresponding QAPI graph
+ * (QMP, string, and QemuOpts), output visitors take a completed QAPI
+ * graph and generate an external representation (QMP and string), and
+ * the dealloc visitor can take a (partial) QAPI graph and recursively
+ * free its resources.  While the dealloc and QMP input/output
+ * visitors are general, the string and QemuOpts visitors have some
+ * implementation limitations; see the documentation for each visitor
+ * for more details on what it supports.  Also, see visitor-impl.h for
+ * the callback contracts implemented by each visitor, and
+ * docs/qapi-code-gen.txt for more about the QAPI code generator.
+ *
+ * All QAPI types have a corresponding function with a signature
+ * roughly compatible with this:
+ *
+ * void visit_type_FOO(Visitor *v, const char *name, void *obj, Error **errp);
+ *
+ * except that *@obj is typed correctly as a pointer or scalar,
+ * depending on the type of FOO.  The scalar visitors are declared
+ * here; the remaining visitors are generated in qapi-visit.h.
+ *
+ * The @name parameter of visit_type_FOO() describes the relation
+ * between this QAPI value and its parent container.  When visiting
+ * the root of a tree, @name is usually ignored (although some
+ * visitors require it to be NULL); when visiting a member of an
+ * object, @name is the key associated with the value; and when
+ * visiting a member of a list, @name is NULL.
+ *
+ * The visit_type_FOO() functions expect a non-NULL @obj argument;
+ * they allocate *@obj during input visits, leave it unchanged on
+ * output visits, and recursively free any resources during a dealloc
+ * visit.  Each function also has an @errp argument which may be NULL
+ * to ignore errors, or point to a NULL Error object on entry for
+ * reporting any errors (such as if a member @name is not present, or
+ * is present but not the specified type).
+ *
+ * FIXME: Clients must pass NULL for @name when visiting a member of a
+ * list, but this leads to poor error messages; it might be nicer to
+ * require a non-NULL name such as "key.0" for '{ "key": [ "value" ]
+ * }' if an error is encountered on "value" (or to have the visitor
+ * core auto-generate the nicer name).
+ *
+ * FIXME: At present, input visitors may allocate an incomplete *@obj
+ * even when visit_type_FOO() reports an error.  Using an output
+ * visitor with an incomplete object has undefined behavior; callers
+ * must call qapi_free_FOO() (which uses the dealloc visitor, and
+ * safely handles an 

[Qemu-devel] [PATCH v13 13/18] qapi: Split visit_end_struct() into pieces

[Eric Blake]

As mentioned in previous patches, we want to call visit_end_struct()
functions unconditionally, so that visitors can release resources
tied up since the matching visit_start_struct() without also having
to worry about error priority if more than one error occurs.

Even though error_propagate() can be safely used to ignore a second
error during cleanup caused by a first error, it is simpler if the
cleanup cannot set an error.  So, split out the error checking
portion (basically, input visitors checking for unvisited keys) into
a new function visit_check_struct(), which can be safely skipped if
any earlier errors are encountered, and leave the cleanup portion
(which never fails, but must be called unconditionally if
visit_start_struct() succeeded) in visit_end_struct().

Generated code has diffs resembling:

|@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v,
| goto out_obj;
| }
| visit_type_ACPIOSTInfo_members(v, obj, );
|-error_propagate(errp, err);
|-err = NULL;
|+if (err) {
|+goto out_obj;
|+}
|+visit_check_struct(v, );
| out_obj:
|-visit_end_struct(v, );
|+visit_end_struct(v);
| out:

Signed-off-by: Eric Blake 

---
v13: rebase to earlier changes
v12: rebase to earlier changes, fix bug in spapr_drc not calling
visit_end_struct, fold in docs, fix stray DO_UPCAST from sneaking
back in
[no v10, v11]
v9: rebase to earlier changes, drop Marc-Andre's R-b
v8: rebase to 'name' motion
v7: rebase to earlier changes
v6: new patch, revised version of RFC based on discussion of v5 7/46
---
 include/qapi/visitor.h | 20 +++-
 include/qapi/visitor-impl.h|  5 -
 scripts/qapi-event.py  |  5 -
 scripts/qapi-visit.py  | 15 +--
 qapi/qapi-visit-core.c | 11 +--
 hw/ppc/spapr_drc.c |  3 ++-
 hw/virtio/virtio-balloon.c | 15 ---
 qapi/opts-visitor.c| 17 +++--
 qapi/qapi-dealloc-visitor.c|  2 +-
 qapi/qmp-input-visitor.c   | 34 +++---
 qapi/qmp-output-visitor.c  |  2 +-
 qom/object.c   |  5 ++---
 qom/object_interfaces.c| 16 +++-
 tests/test-qmp-input-visitor.c |  3 ++-
 docs/qapi-code-gen.txt |  8 +---
 15 files changed, 103 insertions(+), 58 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 184647d..01f9d69 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -183,10 +183,11 @@
  *  }
  * outlist:
  *  visit_end_list(v);
+ *  if (!err) {
+ *  visit_check_struct(v, );
+ *  }
  * outobj:
- *  error_propagate(errp, err);
- *  err = NULL;
- *  visit_end_struct(v, );
+ *  visit_end_struct(v);
  *  error_propagate(errp, err);
  *  ...clean up v...
  * 
@@ -239,17 +240,26 @@ void visit_start_struct(Visitor *v, const char *name, 
void **obj,
 size_t size, Error **errp);

 /*
- * Complete an object visit started earlier.
+ * Prepare for completing an object visit.
  *
  * @errp must be NULL-initialized, and is set if an error is detected
  * (such as unparsed keys remaining in the input stream).
  *
+ * Should be called prior to visit_end_struct() if all other
+ * intermediate visit steps were successful, to allow the visitor one
+ * last chance to report errors.
+ */
+void visit_check_struct(Visitor *v, Error **errp);
+
+/*
+ * Complete an object visit started earlier.
+ *
  * Must be called after any successful use of visit_start_struct(),
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.  Destroying the visitor may
  * behave as if this was implicitly called.
  */
-void visit_end_struct(Visitor *v, Error **errp);
+void visit_end_struct(Visitor *v);


 /* === Visiting lists */
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 90bcaec..d44fcd1 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -39,8 +39,11 @@ struct Visitor
 void (*start_struct)(Visitor *v, const char *name, void **obj,
  size_t size, Error **errp);

+/* Optional; intended for input visitors.  */
+void (*check_struct)(Visitor *v, Error **errp);
+
 /* Must be set to visit structs.  */
-void (*end_struct)(Visitor *v, Error **errp);
+void (*end_struct)(Visitor *v);

 /* Must be set.  */
 void (*start_list)(Visitor *v, const char *name, Error **errp);
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 627e9a0..1ba6c02 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -69,7 +69,10 @@ def gen_event_send(name, arg_type):
 goto out;
 }
 visit_type_%(c_name)s_members(v, , );
-visit_end_struct(v, err ? NULL : );
+if (!err) {
+visit_check_struct(v, );
+}
+visit_end_struct(v);
 if (err) {
 goto out;
 }
diff --git a/scripts/qapi-visit.py 

[Qemu-devel] [PATCH v13 01/18] qapi-visit: Add visitor.type classification

[Eric Blake]

We have three classes of QAPI visitors: input, output, and dealloc.
Currently, all implementations of these visitors have one thing in
common based on their visitor type: the implementation used for the
visit_type_enum() callback.  But since we plan to add more such
common behavior, in relation to documenting and further refining
the semantics, it makes more sense to have the visitor
implementations advertise which class they belong to, so the common
qapi-visit-core code can use that information in multiple places.

For this patch, knowing the class of a visitor implementation lets
us make input_type_enum() and output_type_enum() become static
functions, by replacing the callback function Visitor.type_enum()
with the simpler enum member Visitor.type.  Share a common
assertion in qapi-visit-core as part of the refactoring.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 include/qapi/visitor-impl.h  | 21 -
 qapi/qapi-visit-core.c   | 28 +++-
 qapi/opts-visitor.c  | 12 ++--
 qapi/qapi-dealloc-visitor.c  |  7 +--
 qapi/qmp-input-visitor.c |  2 +-
 qapi/qmp-output-visitor.c|  2 +-
 qapi/string-input-visitor.c  |  2 +-
 qapi/string-output-visitor.c |  2 +-
 8 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 2bd8f29..228a2a6 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -14,6 +14,15 @@

 #include "qapi/visitor.h"

+/* There are three classes of visitors; setting the class determines
+ * how QAPI enums are visited, as well as what additional restrictions
+ * can be asserted.  */
+typedef enum VisitorType {
+VISITOR_INPUT,
+VISITOR_OUTPUT,
+VISITOR_DEALLOC,
+} VisitorType;
+
 struct Visitor
 {
 /* Must be set */
@@ -36,10 +45,6 @@ struct Visitor
 void (*end_alternate)(Visitor *v);

 /* Must be set. */
-void (*type_enum)(Visitor *v, const char *name, int *obj,
-  const char *const strings[], Error **errp);
-
-/* Must be set. */
 void (*type_int64)(Visitor *v, const char *name, int64_t *obj,
Error **errp);
 /* Must be set. */
@@ -58,11 +63,9 @@ struct Visitor

 /* May be NULL; most useful for input visitors. */
 void (*optional)(Visitor *v, const char *name, bool *present);
+
+/* Must be set.  */
+VisitorType type;
 };

-void input_type_enum(Visitor *v, const char *name, int *obj,
- const char *const strings[], Error **errp);
-void output_type_enum(Visitor *v, const char *name, int *obj,
-  const char *const strings[], Error **errp);
-
 #endif
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 856606b..a08d073 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -71,12 +71,6 @@ bool visit_optional(Visitor *v, const char *name, bool 
*present)
 return *present;
 }

-void visit_type_enum(Visitor *v, const char *name, int *obj,
- const char *const strings[], Error **errp)
-{
-v->type_enum(v, name, obj, strings, errp);
-}
-
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
 {
 v->type_int64(v, name, obj, errp);
@@ -207,14 +201,13 @@ void visit_type_any(Visitor *v, const char *name, QObject 
**obj, Error **errp)
 v->type_any(v, name, obj, errp);
 }

-void output_type_enum(Visitor *v, const char *name, int *obj,
-  const char *const strings[], Error **errp)
+static void output_type_enum(Visitor *v, const char *name, int *obj,
+ const char *const strings[], Error **errp)
 {
 int i = 0;
 int value = *obj;
 char *enum_str;

-assert(strings);
 while (strings[i++] != NULL);
 if (value < 0 || value >= i - 1) {
 error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
@@ -225,15 +218,13 @@ void output_type_enum(Visitor *v, const char *name, int 
*obj,
 visit_type_str(v, name, _str, errp);
 }

-void input_type_enum(Visitor *v, const char *name, int *obj,
- const char *const strings[], Error **errp)
+static void input_type_enum(Visitor *v, const char *name, int *obj,
+const char *const strings[], Error **errp)
 {
 Error *local_err = NULL;
 int64_t value = 0;
 char *enum_str;

-assert(strings);
-
 visit_type_str(v, name, _str, _err);
 if (local_err) {
 error_propagate(errp, local_err);
@@ -256,3 +247,14 @@ void input_type_enum(Visitor *v, const char *name, int 
*obj,
 g_free(enum_str);
 *obj = value;
 }
+
+void visit_type_enum(Visitor *v, const char *name, int *obj,
+ const char *const strings[], Error **errp)
+{
+assert(strings);
+if (v->type == VISITOR_INPUT) {
+input_type_enum(v, name, obj, strings, errp);
+} else if (v->type == VISITOR_OUTPUT) {
+

[Qemu-devel] [PATCH v13 16/18] qmp-input: Require struct push to visit members of top dict

[Eric Blake]

Don't embed the root of the visit into the stack of current
containers being visited.  That way, we no longer get confused
on whether the first visit of a dictionary is to the dictionary
itself or to one of the members of the dictionary, and we no
longer have to require the root visit to pass name=NULL.

An audit of all qmp_input_visitor_new* call sites shows that
the only places where the visit starts directly on a QDict,
but where the first visit_type_* within the visit had passed
a non-NULL name, were fixed in the previous two places to
properly push into the object with visit_start_struct().

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 include/qapi/visitor.h   |  3 +--
 include/qapi/qmp-input-visitor.h |  8 --
 qapi/qmp-input-visitor.c | 54 ++--
 3 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 01f9d69..5f86262 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -48,8 +48,7 @@
  *
  * The @name parameter of visit_type_FOO() describes the relation
  * between this QAPI value and its parent container.  When visiting
- * the root of a tree, @name is usually ignored (although some
- * visitors require it to be NULL); when visiting a member of an
+ * the root of a tree, @name is ignored; when visiting a member of an
  * object, @name is the key associated with the value; and when
  * visiting a member of a list, @name is NULL.
  *
diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h
index d75ff98..3ed499c 100644
--- a/include/qapi/qmp-input-visitor.h
+++ b/include/qapi/qmp-input-visitor.h
@@ -19,14 +19,6 @@

 typedef struct QmpInputVisitor QmpInputVisitor;

-/*
- * FIXME: When visiting a QDict, passing a non-NULL @name for the
- * first visit_type_FOO() when the root is a QDict will find that
- * particular key within the QDict.  In the future, the contract may
- * be tightened to require visit_start_struct() with ignored @name as
- * the first visit; in the meantime, the first visit is safest when
- * using NULL for @name.
- */
 QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
 QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 6b69e9a..2daf83f 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -38,9 +38,11 @@ struct QmpInputVisitor
 {
 Visitor visitor;

-/* Stack of objects being visited.  stack[0] is root of visit,
- * stack[1] and below correspond to visit_start_struct (nested
- * QDict) and visit_start_list (nested QList).  */
+/* Root of visit at visitor creation.  */
+QObject *root;
+
+/* Stack of objects being visited (all entries will be either
+ * QDict or QList).  */
 StackObject stack[QIV_STACK_SIZE];
 int nb_stack;

@@ -57,36 +59,40 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
  const char *name,
  bool consume)
 {
-StackObject *tos = >stack[qiv->nb_stack - 1];
-QObject *qobj = tos->obj;
+StackObject *tos;
+QObject *qobj;

+if (!qiv->nb_stack) {
+/* Starting at root, name is ignored.  */
+return qiv->root;
+}
+
+/* We are in a container; find the next element */
+tos = >stack[qiv->nb_stack - 1];
+qobj = tos->obj;
 assert(qobj);

-/* If we have a name, and we're in a dictionary, then return that
- * value. */
-if (name && qobject_type(qobj) == QTYPE_QDICT) {
+if (qobject_type(qobj) == QTYPE_QDICT) {
+assert(name);
 qobj = qdict_get(qobject_to_qdict(qobj), name);
 if (tos->h && consume && qobj) {
 bool removed = g_hash_table_remove(tos->h, name);
 assert(removed);
 }
-return qobj;
-}
-
-/* If we are in the middle of a list, then return the next element
- * of the list.  */
-if (tos->entry) {
+} else {
 assert(qobject_type(qobj) == QTYPE_QLIST);
-assert(!tos->first);
-qobj = qlist_entry_obj(tos->entry);
-if (consume) {
-tos->entry = qlist_next(tos->entry);
+/* FIXME: assertion needs adjustment if we fix visit-core
+ * to pass "name.0" style name during lists.  */
+assert(!name);
+
+if (tos->entry) {
+assert(!tos->first);
+qobj = qlist_entry_obj(tos->entry);
+if (consume) {
+tos->entry = qlist_next(tos->entry);
+}
 }
-return qobj;
 }
-
-/* Otherwise, we are at the root of the visit or the start of a
- * list, and return the object as-is.  */
 return qobj;
 }

@@ -372,7 +378,7 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)

 void qmp_input_visitor_cleanup(QmpInputVisitor *v)
 {
-qobject_decref(v->stack[0].obj);
+

[Qemu-devel] [PATCH v13 06/18] qmp-input: Refactor when list is advanced

[Eric Blake]

Refactor the code to advance the QListEntry pointer at the point
where visit_type_FOO() is called, rather than visit_next_list().
This will allow a future patch to move the visit of the list head
into visit_start_list(), and get rid of the 'first' flag.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 qapi/qmp-input-visitor.c | 40 +---
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 4bb7b21..ed54d4c 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -26,9 +26,10 @@ typedef struct StackObject
 {
 QObject *obj; /* Object being visited */

-/* If obj is list: NULL if list is at head, otherwise tail of list
- * still needing visits */
+/* If obj is list: tail of list still needing visits */
 const QListEntry *entry;
+/* If obj is list: true if head is not visited yet */
+bool first;

 GHashTable *h; /* If obj is dict: remaining keys needing visits */
 } StackObject;
@@ -76,7 +77,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
  * of the list.  */
 if (tos->entry) {
 assert(qobject_type(qobj) == QTYPE_QLIST);
-return qlist_entry_obj(tos->entry);
+assert(!tos->first);
+qobj = qlist_entry_obj(tos->entry);
+if (consume) {
+tos->entry = qlist_next(tos->entry);
+}
+return qobj;
 }

 /* Otherwise, we are at the root of the visit or the start of a
@@ -90,7 +96,8 @@ static void qdict_add_key(const char *key, QObject *obj, void 
*opaque)
 g_hash_table_insert(h, (gpointer) key, NULL);
 }

-static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
+static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
+   const QListEntry *entry, Error **errp)
 {
 GHashTable *h;
 StackObject *tos = >stack[qiv->nb_stack];
@@ -102,7 +109,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject 
*obj, Error **errp)
 }

 tos->obj = obj;
-tos->entry = NULL;
+tos->entry = entry;
+tos->first = true;
 tos->h = NULL;

 if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
@@ -152,7 +160,7 @@ static void qmp_input_start_struct(Visitor *v, const char 
*name, void **obj,
 return;
 }

-qmp_input_push(qiv, qobj, );
+qmp_input_push(qiv, qobj, NULL, );
 if (err) {
 error_propagate(errp, err);
 return;
@@ -174,6 +182,7 @@ static void qmp_input_start_list(Visitor *v, const char 
*name, Error **errp)
 {
 QmpInputVisitor *qiv = to_qiv(v);
 QObject *qobj = qmp_input_get_object(qiv, name, true);
+const QListEntry *entry;

 if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -181,7 +190,8 @@ static void qmp_input_start_list(Visitor *v, const char 
*name, Error **errp)
 return;
 }

-qmp_input_push(qiv, qobj, errp);
+entry = qlist_first(qobject_to_qlist(qobj));
+qmp_input_push(qiv, qobj, entry, errp);
 }

 static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
@@ -190,23 +200,15 @@ static GenericList *qmp_input_next_list(Visitor *v, 
GenericList **list,
 QmpInputVisitor *qiv = to_qiv(v);
 GenericList *entry;
 StackObject *so = >stack[qiv->nb_stack - 1];
-bool first;

-if (so->entry == NULL) {
-so->entry = qlist_first(qobject_to_qlist(so->obj));
-first = true;
-} else {
-so->entry = qlist_next(so->entry);
-first = false;
-}
-
-if (so->entry == NULL) {
+if (!so->entry) {
 return NULL;
 }

 entry = g_malloc0(size);
-if (first) {
+if (so->first) {
 *list = entry;
+so->first = false;
 } else {
 (*list)->next = entry;
 }
@@ -381,7 +383,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
 v->visitor.type_any = qmp_input_type_any;
 v->visitor.optional = qmp_input_optional;

-qmp_input_push(v, obj, NULL);
+qmp_input_push(v, obj, NULL, NULL);
 qobject_incref(obj);

 return v;
-- 
2.5.0

[Qemu-devel] [PATCH v13 15/18] qom: Wrap prop visit in visit_start_struct

[Eric Blake]

The qmp-input visitor was playing rather fast and loose: when
visiting a QDict, you could grab members of the root dictionary
without first pushing into the dict.  But we are about to tighten
the input visitor, at which point user_creatable_add_type() MUST
follow the same paradigms as everyone else, of pushing into the
struct before grabbing its keys, because the value of 'name'
should be ignored on the top-level visit.

The change has no impact to the testsuite now, but is required to
avoid a failure in tests/test-netfilter once qmp-input is made
stricter.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 qom/object_interfaces.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index f075b3a..8efb8d1 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -117,12 +117,23 @@ Object *user_creatable_add_type(const char *type, const 
char *id,

 obj = object_new(type);
 if (qdict) {
+visit_start_struct(v, NULL, NULL, 0, _err);
+if (local_err) {
+goto out;
+}
 for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
 object_property_set(obj, v, e->key, _err);
 if (local_err) {
-goto out;
+break;
 }
 }
+if (!local_err) {
+visit_check_struct(v, _err);
+}
+visit_end_struct(v);
+if (local_err) {
+goto out;
+}
 }

 object_property_add_child(object_get_objects_root(),
-- 
2.5.0

[Qemu-devel] [PATCH v13 14/18] qapi-commands: Wrap argument visit in visit_start_struct

[Eric Blake]

The qmp-input visitor was playing rather fast and loose: when
visiting a QDict, you could grab members of the root dictionary
without first pushing into the dict.  But we are about to tighten
the input visitor, at which point the generated marshal code
MUST follow the same paradigms as everyone else, of pushing into
the struct before grabbing its keys, because the value of 'name'
should be ignored on the top-level visit.

Generated code grows as follows:

|@@ -515,7 +695,15 @@ void qmp_marshal_blockdev_backup(QDict *
| BlockdevBackup qapi = {0};
|
| v = qmp_input_get_visitor(qiv);
|+visit_start_struct(v, NULL, NULL, 0, );
|+if (err) {
|+goto out;
|+}
| visit_type_BlockdevBackup_members(v, , );
|+if (!err) {
|+visit_check_struct(v, );
|+}
|+visit_end_struct(v);
| if (err) {
| goto out;
| }
|@@ -527,7 +715,9 @@ out:
| qmp_input_visitor_cleanup(qiv);
| qdv = qapi_dealloc_visitor_new();
| v = qapi_dealloc_get_visitor(qdv);
|+visit_start_struct(v, NULL, NULL, 0, NULL);
| visit_type_BlockdevBackup_members(v, , NULL);
|+visit_end_struct(v);
| qapi_dealloc_visitor_cleanup(qdv);
| }

Note that this change could also make it possible for the
marshalling code to automatically detect excess input at the top
level, and not just in nested dictionaries.  However, that checking
is not currently useful (and we rely on the manual checking in
monitor.c:qmp_check_client_args() instead) as long as qmp-commands.hx
uses .args_type, and as long as we have 'name:O' as an arg-type that
explicitly allows unknown top-level keys because we haven't yet
converted 'device_add' and 'netdev_add' to introspectible use of
'any'.

Signed-off-by: Eric Blake 

---
v13: rebase to earlier patches
v12: new patch
---
 scripts/qapi-commands.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 710e853..8b1a676 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -121,7 +121,15 @@ def gen_marshal(name, arg_type, ret_type):
 %(c_name)s qapi = {0};

 v = qmp_input_get_visitor(qiv);
+visit_start_struct(v, NULL, NULL, 0, );
+if (err) {
+goto out;
+}
 visit_type_%(c_name)s_members(v, , );
+if (!err) {
+visit_check_struct(v, );
+}
+visit_end_struct(v);
 if (err) {
 goto out;
 }
@@ -150,7 +158,9 @@ out:
 qmp_input_visitor_cleanup(qiv);
 qdv = qapi_dealloc_visitor_new();
 v = qapi_dealloc_get_visitor(qdv);
+visit_start_struct(v, NULL, NULL, 0, NULL);
 visit_type_%(c_name)s_members(v, , NULL);
+visit_end_struct(v);
 qapi_dealloc_visitor_cleanup(qdv);
 ''',
  c_name=arg_type.c_name())
-- 
2.5.0

[Qemu-devel] [PATCH v13 17/18] qapi: Simplify semantics of visit_next_list()

[Eric Blake]

We have two uses of list visits in the entire code base: one in
spapr_drc (which completely avoids visit_next_list(), feeding in
integers from a different source than uint8List), and one in
qapi-visit.py (that is, all other list visitors are generated
in qapi-visit.c, and share the same paradigm based on a qapi
FooList type).  What's more, the semantics of the list visit are
somewhat baroque, with the following pseudocode when FooList is
used:

start()
for (prev = head; cur = next(prev); prev = ) {
visit(>value)
}

Note that these semantics (advance before visit) requires that
the first call to next() return the list head, while all other
calls return the next element of the list; that is, every visitor
implementation is required to track extra state to decide whether
to return the input as-is, or to advance.  It also requires an
argument of 'GenericList **' to next(), solely because the first
iteration might need to modify the caller's GenericList head, so
that all other calls have to do a layer of dereferencing.

We can greatly simplify things by hoisting the special case
into the start() routine, and flipping the order in the loop
to visit before advance:

start(head)
for (tail = *head; tail; tail = next(tail)) {
visit(>value)
}

With the simpler semantics, visitors have less state to track,
the argument to next() is reduced to 'GenericList *', and it
also becomes obvious whether an input visitor is allocating a
FooList during visit_start_list() (rather than the old way of
not knowing if an allocation happened until the first
visit_next_list()).

The signature of visit_start_list() is chosen to match
visit_start_struct(), with the new parameters after 'name'.

The spapr_drc case is a virtual visit, done by passing NULL for
list, similarly to how NULL is passed to visit_start_struct()
when a qapi type is not used in those visits.  It was easy to
provide these semantics for qmp-output and dealloc visitors,
and a bit harder for qmp-input (several prerequisite patches
refactored things to make this patch straightforward).  But it
turned out that the string and opts visitors munge enough other
state during visit_next_list() to make it easier to just
document and require a GenericList visit for now; an assertion
will remind us to adjust things if we need the semantics in the
future.

Signed-off-by: Eric Blake 

---
v13: documentation wording tweaks
v12: rebase to earlier changes, drop R-b, improve documentation,
split out qmp-input cleanups into prereq patches
[no v10, v11]
v9: no change
v8: consistent parameter order, fix qmp_input_get_next_type() to not
skip list entries
v7: new patch
---
 include/qapi/visitor.h   | 47 +++-
 include/qapi/visitor-impl.h  |  7 +++---
 scripts/qapi-visit.py| 18 +++---
 include/qapi/opts-visitor.h  |  2 +-
 include/qapi/string-input-visitor.h  |  3 ++-
 include/qapi/string-output-visitor.h |  3 ++-
 qapi/qapi-visit-core.c   | 12 +
 hw/ppc/spapr_drc.c   |  2 +-
 qapi/opts-visitor.c  | 33 +++--
 qapi/qapi-dealloc-visitor.c  | 35 ++-
 qapi/qmp-input-visitor.c | 32 
 qapi/qmp-output-visitor.c| 22 -
 qapi/string-input-visitor.c  | 36 +--
 qapi/string-output-visitor.c | 41 +++
 docs/qapi-code-gen.txt   | 17 +++--
 15 files changed, 133 insertions(+), 177 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5f86262..8d4837d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -166,7 +166,7 @@
  *  if (err) {
  *  goto outobj;
  *  }
- *  visit_start_list(v, "list", );
+ *  visit_start_list(v, "list", NULL, 0, );
  *  if (err) {
  *  goto outobj;
  *  }
@@ -269,40 +269,43 @@ void visit_end_struct(Visitor *v);
  * @name expresses the relationship of this list to its parent
  * container; see the general description of @name above.
  *
+ * @list must be non-NULL for a real walk, in which case @size
+ * determines how much memory an input visitor will allocate into
+ * *@list (at least sizeof(GenericList)).  Some visitors also allow
+ * @list to be NULL for a virtual walk, in which case @size is
+ * ignored.
+ *
  * @errp must be NULL-initialized, and is set if an error is detected
  * (such as if a member @name is not present, or is present but not a
- * list).
+ * list).  On error, input visitors set *@obj to NULL.
  *
  * After visit_start_list() succeeds, the caller may visit its members
- * one after the other.  A real visit uses visit_next_list() for
- * traversing the linked list, while a virtual visit uses other means.
- * For each list element, call the appropriate visit_type_FOO() with
- * name set to NULL and obj set to the address of the value member of
- * the list 

[Qemu-devel] [PATCH v13 02/18] qapi: Guarantee NULL obj on input visitor callback error

[Eric Blake]

Our existing input visitors were not very consistent on errors
in a function taking 'TYPE **obj'. While all of them set '*obj'
to allocated storage on success, it was not obvious whether
'*obj' was guaranteed safe on failure, or whether it was left
uninitialized.  But a future patch wants to guarantee that
visit_type_FOO() does not leak a partially-constructed obj back
to the caller; it is easier to implement this if we can reliably
state that '*obj' is assigned on exit, even on failures.

The opts-visitor start_struct() doesn't set an error, but it
also was doing a weird check for 0 size; all callers pass in
non-zero size if obj is non-NULL.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 qapi/opts-visitor.c | 3 ++-
 qapi/qmp-input-visitor.c| 4 
 qapi/string-input-visitor.c | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index f9a2346..fd90608 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -132,7 +132,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj,
 const QemuOpt *opt;

 if (obj) {
-*obj = g_malloc0(size > 0 ? size : 1);
+*obj = g_malloc0(size);
 }
 if (ov->depth++ > 0) {
 return;
@@ -313,6 +313,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, 
Error **errp)

 opt = lookup_scalar(ov, name, errp);
 if (!opt) {
+*obj = NULL;
 return;
 }
 *obj = g_strdup(opt->str ? opt->str : "");
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 03dcb65..550aed6 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -119,6 +119,9 @@ static void qmp_input_start_struct(Visitor *v, const char 
*name, void **obj,
 QObject *qobj = qmp_input_get_object(qiv, name, true);
 Error *err = NULL;

+if (obj) {
+*obj = NULL;
+}
 if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"QDict");
@@ -266,6 +269,7 @@ static void qmp_input_type_str(Visitor *v, const char 
*name, char **obj,
 QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));

 if (!qstr) {
+*obj = NULL;
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"string");
 return;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index d591e67..4087bf9 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -292,6 +292,7 @@ static void parse_type_str(Visitor *v, const char *name, 
char **obj,
 if (siv->string) {
 *obj = g_strdup(siv->string);
 } else {
+*obj = NULL;
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"string");
 }
-- 
2.5.0

[Qemu-devel] [PATCH v13 12/18] qmp: Tighten output visitor rules

[Eric Blake]

Add a new qmp_output_visitor_reset(), which must be called before
reusing an exising QmpOutputVisitor on a new root object.  Tighten
assertions to require that qmp_output_get_qobject() can only be
called after pairing a visit_end_* for every visit_start_* (rather
than allowing it to return a partially built object), and that it
must not be called unless at least one visit_type_* or visit_start/
visit_end pair has occurred since creation/reset (the accidental
return of NULL fixed by commit ab8bf1d7 would have been much
easier to diagnose).

Also, check that we are encountering the expected object or list
type (both during visit_end*, and also by validating whether 'name'
was NULL - although the latter may need to change later if we
improve error messages by always passing in a sensible name).
This provides protection against mismatched push(struct)/pop(list)
or push(list)/pop(struct), similar to the qmp-input protection
added in commit bdd8e6b5.

Signed-off-by: Eric Blake 

---
v13: no change
v12: rebase to latest, move type_null() into earlier patches,
don't change signature of pop, don't auto-reset after a single
get_qobject
[no v10, v11]
v9: rebase to added patch, squash in more sanity checks, drop
Marc-Andre's R-b
v8: rename qmp_output_reset to qmp_output_visitor_reset
v7: new patch, based on discussion about spapr_drc.c
---
 include/qapi/qmp-output-visitor.h |  1 +
 qapi/qmp-output-visitor.c | 39 +++
 tests/test-qmp-output-visitor.c   |  6 ++
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/include/qapi/qmp-output-visitor.h 
b/include/qapi/qmp-output-visitor.h
index 2266770..5093f0d 100644
--- a/include/qapi/qmp-output-visitor.h
+++ b/include/qapi/qmp-output-visitor.h
@@ -21,6 +21,7 @@ typedef struct QmpOutputVisitor QmpOutputVisitor;

 QmpOutputVisitor *qmp_output_visitor_new(void);
 void qmp_output_visitor_cleanup(QmpOutputVisitor *v);
+void qmp_output_visitor_reset(QmpOutputVisitor *v);

 QObject *qmp_output_get_qobject(QmpOutputVisitor *v);
 Visitor *qmp_output_get_visitor(QmpOutputVisitor *v);
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 5681ad3..7c48dfb 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -82,9 +82,8 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const 
char *name,
 QObject *cur = e ? e->value : NULL;

 if (!cur) {
-/* FIXME we should require the user to reset the visitor, rather
- * than throwing away the previous root */
-qobject_decref(qov->root);
+/* Don't allow reuse of visitor on more than one root */
+assert(!qov->root);
 qov->root = value;
 } else {
 switch (qobject_type(cur)) {
@@ -93,6 +92,9 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const 
char *name,
 qdict_put_obj(qobject_to_qdict(cur), name, value);
 break;
 case QTYPE_QLIST:
+/* FIXME: assertion needs adjustment if we fix visit-core
+ * to pass "name.0" style name during lists.  */
+assert(!name);
 qlist_append_obj(qobject_to_qlist(cur), value);
 break;
 default:
@@ -114,7 +116,8 @@ static void qmp_output_start_struct(Visitor *v, const char 
*name, void **obj,
 static void qmp_output_end_struct(Visitor *v, Error **errp)
 {
 QmpOutputVisitor *qov = to_qov(v);
-qmp_output_pop(qov);
+QObject *value = qmp_output_pop(qov);
+assert(qobject_type(value) == QTYPE_QDICT);
 }

 static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
@@ -145,7 +148,8 @@ static GenericList *qmp_output_next_list(Visitor *v, 
GenericList **listp,
 static void qmp_output_end_list(Visitor *v)
 {
 QmpOutputVisitor *qov = to_qov(v);
-qmp_output_pop(qov);
+QObject *value = qmp_output_pop(qov);
+assert(qobject_type(value) == QTYPE_QLIST);
 }

 static void qmp_output_type_int64(Visitor *v, const char *name, int64_t *obj,
@@ -202,18 +206,15 @@ static void qmp_output_type_null(Visitor *v, const char 
*name, Error **errp)
 qmp_output_add_obj(qov, name, qnull());
 }

-/* Finish building, and return the root object. Will not be NULL. */
+/* Finish building, and return the root object. Will not be NULL.
+ * Caller must use qobject_decref() on the result.  */
 QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
 {
-/* FIXME: we should require that a visit occurred, and that it is
- * complete (no starts without a matching end) */
-QObject *obj = qov->root;
-if (obj) {
-qobject_incref(obj);
-} else {
-obj = qnull();
-}
-return obj;
+/* A visit must have occurred, with each start paired with end.  */
+assert(qov->root && !QTAILQ_FIRST(>stack));
+
+qobject_incref(qov->root);
+return qov->root;
 }

 Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
@@ -221,7 +222,7 @@ Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
   

[Qemu-devel] [PATCH v13 03/18] qmp: Drop dead command->type

[Eric Blake]

Ever since QMP was first added back in commit 43c20a43, we have
never had any QmpCommandType other than QCT_NORMAL.  It's
pointless to carry around the cruft.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 include/qapi/qmp/dispatch.h |  6 --
 qapi/qmp-dispatch.c | 18 +++---
 qapi/qmp-registry.c |  1 -
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 4955209..5609946 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -19,11 +19,6 @@

 typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);

-typedef enum QmpCommandType
-{
-QCT_NORMAL,
-} QmpCommandType;
-
 typedef enum QmpCommandOptions
 {
 QCO_NO_OPTIONS = 0x0,
@@ -33,7 +28,6 @@ typedef enum QmpCommandOptions
 typedef struct QmpCommand
 {
 const char *name;
-QmpCommandType type;
 QmpCommandFunc *fn;
 QmpCommandOptions options;
 QTAILQ_ENTRY(QmpCommand) node;
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 8f27c34..072e092 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -93,17 +93,13 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
**errp)
 QINCREF(args);
 }

-switch (cmd->type) {
-case QCT_NORMAL:
-cmd->fn(args, , _err);
-if (local_err) {
-error_propagate(errp, local_err);
-} else if (cmd->options & QCO_NO_SUCCESS_RESP) {
-g_assert(!ret);
-} else if (!ret) {
-ret = QOBJECT(qdict_new());
-}
-break;
+cmd->fn(args, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+} else if (cmd->options & QCO_NO_SUCCESS_RESP) {
+g_assert(!ret);
+} else if (!ret) {
+ret = QOBJECT(qdict_new());
 }

 QDECREF(args);
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 4ebfbcc..4332a68 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -25,7 +25,6 @@ void qmp_register_command(const char *name, QmpCommandFunc 
*fn,
 QmpCommand *cmd = g_malloc0(sizeof(*cmd));

 cmd->name = name;
-cmd->type = QCT_NORMAL;
 cmd->fn = fn;
 cmd->enabled = true;
 cmd->options = options;
-- 
2.5.0

[Qemu-devel] [PATCH v13 10/18] qmp: Support explicit null during visits

[Eric Blake]

Implement the new type_null() callback for the qmp input and
output visitors. While we don't yet have a use for this in QAPI
input (the generator will need some tweaks first), one usage
is already envisioned: when changing blockdev parameters, it
would be nice to have a difference between leaving a tuning
parameter unchanged (omit that parameter from the struct) and
to explicitly reset the parameter to its default without having
to know what the default value is (specify the parameter with
an explicit null value, which will require us to allow a QAPI
alternate that chooses between the normal value and an explicit
null).  Meanwhile, the output visitor could already output
explicit null via type_any, but this gives us finer control.

At any rate, it's easy to test that we can round-trip an explicit
null through manual use of visit_type_null().  Repurpose the
test_visitor_out_empty test, particularly since a future patch
will tighten semantics to forbid immediate use of
qmp_output_get_qobject() without at least one visit_type_*.

Signed-off-by: Eric Blake 

---
v13: no change
v12: retitle and merge in output visitor support, move refcount
tests to separate file
[no v10, v11]
v9: new patch
---
 qapi/qmp-input-visitor.c| 12 
 qapi/qmp-output-visitor.c   |  7 +++
 tests/check-qnull.c | 10 +-
 tests/test-qmp-input-visitor.c  | 16 
 tests/test-qmp-output-visitor.c |  9 +
 5 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index ed54d4c..faa40e7 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -338,6 +338,17 @@ static void qmp_input_type_any(Visitor *v, const char 
*name, QObject **obj,
 *obj = qobj;
 }

+static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
+{
+QmpInputVisitor *qiv = to_qiv(v);
+QObject *qobj = qmp_input_get_object(qiv, name, true);
+
+if (qobject_type(qobj) != QTYPE_QNULL) {
+error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+   "null");
+}
+}
+
 static void qmp_input_optional(Visitor *v, const char *name, bool *present)
 {
 QmpInputVisitor *qiv = to_qiv(v);
@@ -381,6 +392,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
 v->visitor.type_str = qmp_input_type_str;
 v->visitor.type_number = qmp_input_type_number;
 v->visitor.type_any = qmp_input_type_any;
+v->visitor.type_null = qmp_input_type_null;
 v->visitor.optional = qmp_input_optional;

 qmp_input_push(v, obj, NULL, NULL);
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 1f2a7ba..5681ad3 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -196,6 +196,12 @@ static void qmp_output_type_any(Visitor *v, const char 
*name, QObject **obj,
 qmp_output_add_obj(qov, name, *obj);
 }

+static void qmp_output_type_null(Visitor *v, const char *name, Error **errp)
+{
+QmpOutputVisitor *qov = to_qov(v);
+qmp_output_add_obj(qov, name, qnull());
+}
+
 /* Finish building, and return the root object. Will not be NULL. */
 QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
 {
@@ -246,6 +252,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
 v->visitor.type_str = qmp_output_type_str;
 v->visitor.type_number = qmp_output_type_number;
 v->visitor.type_any = qmp_output_type_any;
+v->visitor.type_null = qmp_output_type_null;

 QTAILQ_INIT(>stack);

diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index b0fb1e6..e16c783 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -11,6 +11,7 @@

 #include "qapi/qmp/qobject.h"
 #include "qemu-common.h"
+#include "qapi/qmp-input-visitor.h"
 #include "qapi/qmp-output-visitor.h"

 /*
@@ -37,15 +38,22 @@ static void qnull_visit_test(void)
 {
 QObject *obj;
 QmpOutputVisitor *qov;
+QmpInputVisitor *qiv;

 g_assert(qnull_.refcnt == 1);
+obj = qnull();
+qiv = qmp_input_visitor_new(obj);
+qobject_decref(obj);
+visit_type_null(qmp_input_get_visitor(qiv), NULL, _abort);
+
 qov = qmp_output_visitor_new();
-/* FIXME: Empty visits are ugly, we should have a visit_type_null(). */
+visit_type_null(qmp_output_get_visitor(qov), NULL, _abort);
 obj = qmp_output_get_qobject(qov);
 g_assert(obj == _);
 qobject_decref(obj);

 qmp_output_visitor_cleanup(qov);
+qmp_input_visitor_cleanup(qiv);
 g_assert(qnull_.refcnt == 1);
 }

diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 5941e90..46fa1b9 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -278,6 +278,20 @@ static void test_visitor_in_any(TestInputVisitorData *data,
 qobject_decref(res);
 }

+static void test_visitor_in_null(TestInputVisitorData *data,
+ const void *unused)
+{
+Visitor *v;
+
+v = visitor_input_test_init(data, 

[Qemu-devel] [PATCH v13 09/18] qapi: Add visit_type_null() visitor

[Eric Blake]

Right now, qmp-output-visitor happens to produce a QNull result
if nothing is actually visited between the creation of the visitor
and the request for the resulting QObject.  A stronger protocol
would require that a QMP output visit MUST visit something.  But
to still be able to produce a JSON 'null' output, we need a new
visitor function that states our intentions.  Yes, we could say
that such a visit must go through visit_type_any(), but that
feels clunky.

So this patch introduces the new visit_type_null() interface and
its no-op interface in the dealloc visitor, and the next patch
will then wire it up into the qmp visitors.  For the visitors
that will not implement the callback, document the situation.
The code in qapi-visit-core unconditionally dereferences the
callback pointer, so that a segfault will inform a developer if
they need to implement the callback for their choice of visitor.

If QAPI had a 'null' type, we'd also have to use visit_type_null()
in the generated visitor functions (most likely, we'd use it via
an alternate type that permits 'null' or an object); we'll create
that usage when we need it.

Signed-off-by: Eric Blake 

---
v13: no change
v12: rebase to earlier changes, drop R-b due to better documentation
[no v10, v11]
v9: no change
v8: rebase to 'name' motion
v7: new patch, based on discussion about spapr_drc.c
---
 include/qapi/visitor.h   | 12 
 include/qapi/visitor-impl.h  |  3 +++
 include/qapi/opts-visitor.h  |  2 +-
 include/qapi/string-input-visitor.h  |  2 +-
 include/qapi/string-output-visitor.h |  2 +-
 qapi/qapi-visit-core.c   |  5 +
 qapi/qapi-dealloc-visitor.c  |  5 +
 7 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 093bc70..184647d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -508,4 +508,16 @@ void visit_type_number(Visitor *v, const char *name, 
double *obj,
  */
 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);

+/*
+ * Visit a JSON null value.
+ *
+ * @name expresses the relationship of the null value to its parent
+ * container; see the general description of @name above.
+ *
+ * Unlike all other visit_type_* functions, no obj parameter is
+ * needed; rather, this is a witness that an explicit null value is
+ * expected rather than any other type.
+ */
+void visit_type_null(Visitor *v, const char *name, Error **errp);
+
 #endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index d967b18..90bcaec 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -86,6 +86,9 @@ struct Visitor
 void (*type_any)(Visitor *v, const char *name, QObject **obj,
  Error **errp);

+/* Must be set to visit explicit null values.  */
+void (*type_null)(Visitor *v, const char *name, Error **errp);
+
 /* Must be set for input visitors, optional otherwise.  The core
  * takes care of the return type in the public interface.  */
 void (*optional)(Visitor *v, const char *name, bool *present);
diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
index 2002e37..3fcd327 100644
--- a/include/qapi/opts-visitor.h
+++ b/include/qapi/opts-visitor.h
@@ -31,7 +31,7 @@ typedef struct OptsVisitor OptsVisitor;
  * - values above INT64_MAX or LLONG_MAX are rejected.
  *
  * The Opts input visitor does not yet implement support for visiting
- * QAPI alternates, numbers (other than integers), or arbitrary
+ * QAPI alternates, numbers (other than integers), null, or arbitrary
  * QTypes.
  */
 OptsVisitor *opts_visitor_new(const QemuOpts *opts);
diff --git a/include/qapi/string-input-visitor.h 
b/include/qapi/string-input-visitor.h
index 4c8d1ea..1a34c52 100644
--- a/include/qapi/string-input-visitor.h
+++ b/include/qapi/string-input-visitor.h
@@ -19,7 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;

 /*
  * The string input visitor does not yet implement support for
- * visiting QAPI structs, alternates, or arbitrary QTypes.
+ * visiting QAPI structs, alternates, null, or arbitrary QTypes.
  */
 StringInputVisitor *string_input_visitor_new(const char *str);
 void string_input_visitor_cleanup(StringInputVisitor *v);
diff --git a/include/qapi/string-output-visitor.h 
b/include/qapi/string-output-visitor.h
index 094a11e..2564833 100644
--- a/include/qapi/string-output-visitor.h
+++ b/include/qapi/string-output-visitor.h
@@ -19,7 +19,7 @@ typedef struct StringOutputVisitor StringOutputVisitor;

 /*
  * The string output visitor does not yet implement support for
- * visiting QAPI structs, alternates, or arbitrary QTypes.
+ * visiting QAPI structs, alternates, null, or arbitrary QTypes.
  */
 StringOutputVisitor *string_output_visitor_new(bool human);
 void string_output_visitor_cleanup(StringOutputVisitor *v);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 

[Qemu-devel] [PATCH v13 11/18] spapr_drc: Expose 'null' in qom-get when there is no fdt

[Eric Blake]

Now that the QMP output visitor supports an explicit null
output, we should utilize it to make it easier to diagnose
the difference between a missing fdt ('null') vs. a
present-but-empty one ('{}').

(Note that this reverts the behavior of commit ab8bf1d, taking
us back to the behavior of commit 6c2f9a1 [which in turn
stemmed from a crash fix in 1d10b44]; but that this time,
the change is intentional and not an accidental side-effect.)

Signed-off-by: Eric Blake 
Acked-by: David Gibson 

---
v13: no change
v12: tweak commit message
[no v10, v11]
v9: improved commit message
v8: rebase to 'name' motion
v7: new patch, based on discussion about spapr_drc.c
---
 hw/ppc/spapr_drc.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index ef063c0..b1bf193 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -260,11 +260,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const 
char *name,
 void *fdt;

 if (!drc->fdt) {
-visit_start_struct(v, name, NULL, 0, );
-if (!err) {
-visit_end_struct(v, );
-}
-error_propagate(errp, err);
+visit_type_null(v, NULL, errp);
 return;
 }

-- 
2.5.0

[Qemu-devel] [PATCH v13 04/18] qmp-input: Clean up stack handling

[Eric Blake]

Management of the top of stack was a bit verbose; creating a
temporary variable and adding some comments makes the existing
code more legible before the next few patches improve things.
No semantic changes other than asserting that we are always
visiting a QObject, and not a NULL value.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 qapi/qmp-input-visitor.c | 52 ++--
 1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 550aed6..7428d15 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -24,16 +24,26 @@

 typedef struct StackObject
 {
-QObject *obj;
+QObject *obj; /* Object being visited */
+
+/* If obj is list: NULL if list is at head, otherwise tail of list
+ * still needing visits */
 const QListEntry *entry;
-GHashTable *h;
+
+GHashTable *h; /* If obj is dict: remaining keys needing visits */
 } StackObject;

 struct QmpInputVisitor
 {
 Visitor visitor;
+
+/* Stack of objects being visited.  stack[0] is root of visit,
+ * stack[1] and below correspond to visit_start_struct (nested
+ * QDict) and visit_start_list (nested QList).  */
 StackObject stack[QIV_STACK_SIZE];
 int nb_stack;
+
+/* True to track whether all keys in QDict have been parsed.  */
 bool strict;
 };

@@ -46,19 +56,29 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
  const char *name,
  bool consume)
 {
-QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
+StackObject *tos = >stack[qiv->nb_stack - 1];
+QObject *qobj = tos->obj;

-if (qobj) {
-if (name && qobject_type(qobj) == QTYPE_QDICT) {
-if (qiv->stack[qiv->nb_stack - 1].h && consume) {
-g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
-}
-return qdict_get(qobject_to_qdict(qobj), name);
-} else if (qiv->stack[qiv->nb_stack - 1].entry) {
-return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+assert(qobj);
+
+/* If we have a name, and we're in a dictionary, then return that
+ * value. */
+if (name && qobject_type(qobj) == QTYPE_QDICT) {
+if (tos->h && consume) {
+g_hash_table_remove(tos->h, name);
 }
+return qdict_get(qobject_to_qdict(qobj), name);
 }

+/* If we are in the middle of a list, then return the next element
+ * of the list.  */
+if (tos->entry) {
+assert(qobject_type(qobj) == QTYPE_QLIST);
+return qlist_entry_obj(tos->entry);
+}
+
+/* Otherwise, we are at the root of the visit or the start of a
+ * list, and return the object as-is.  */
 return qobj;
 }

@@ -71,20 +91,22 @@ static void qdict_add_key(const char *key, QObject *obj, 
void *opaque)
 static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
 {
 GHashTable *h;
+StackObject *tos = >stack[qiv->nb_stack];

+assert(obj);
 if (qiv->nb_stack >= QIV_STACK_SIZE) {
 error_setg(errp, "An internal buffer overran");
 return;
 }

-qiv->stack[qiv->nb_stack].obj = obj;
-qiv->stack[qiv->nb_stack].entry = NULL;
-qiv->stack[qiv->nb_stack].h = NULL;
+tos->obj = obj;
+tos->entry = NULL;
+tos->h = NULL;

 if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
 h = g_hash_table_new(g_str_hash, g_str_equal);
 qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
-qiv->stack[qiv->nb_stack].h = h;
+tos->h = h;
 }

 qiv->nb_stack++;
-- 
2.5.0

[Qemu-devel] [PATCH v13 00/18] qapi visitor cleanups (post-introspection cleanups subset E)

[Eric Blake]

Requires these patches first ("v4 implicit types"):
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01205.html
which in turn requires Markus' qapi-next branch:
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01182.html

v13: Minor rebase improvements

v12: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg2.html
This series (finally) finishes the review comments against the
tail end of my v9 subset E series:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03504.html

v10 and v11 of the series dealt with the first half, and are now
mostly applied (or part of the pre-requisite series).

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv13e

and will soon be part of my branch with the rest of the v5 series, at:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

001/18:[] [--] 'qapi-visit: Add visitor.type classification'
002/18:[] [--] 'qapi: Guarantee NULL obj on input visitor callback error'
003/18:[] [--] 'qmp: Drop dead command->type'
004/18:[] [--] 'qmp-input: Clean up stack handling'
005/18:[] [--] 'qmp-input: Don't consume input when checking has_member'
006/18:[] [--] 'qmp-input: Refactor when list is advanced'
007/18:[0011] [FC] 'qapi: Document visitor interfaces, add assertions'
008/18:[] [--] 'tests: Add check-qnull'
009/18:[] [--] 'qapi: Add visit_type_null() visitor'
010/18:[] [--] 'qmp: Support explicit null during visits'
011/18:[] [--] 'spapr_drc: Expose 'null' in qom-get when there is no fdt'
012/18:[] [--] 'qmp: Tighten output visitor rules'
013/18:[0011] [FC] 'qapi: Split visit_end_struct() into pieces'
014/18:[0016] [FC] 'qapi-commands: Wrap argument visit in visit_start_struct'
015/18:[] [--] 'qom: Wrap prop visit in visit_start_struct'
016/18:[] [--] 'qmp-input: Require struct push to visit members of top dict'
017/18:[0017] [FC] 'qapi: Simplify semantics of visit_next_list()'
018/18:[0002] [FC] 'qapi: Change visit_type_FOO() to no longer return partial 
objects'

Eric Blake (18):
  qapi-visit: Add visitor.type classification
  qapi: Guarantee NULL obj on input visitor callback error
  qmp: Drop dead command->type
  qmp-input: Clean up stack handling
  qmp-input: Don't consume input when checking has_member
  qmp-input: Refactor when list is advanced
  qapi: Document visitor interfaces, add assertions
  tests: Add check-qnull
  qapi: Add visit_type_null() visitor
  qmp: Support explicit null during visits
  spapr_drc: Expose 'null' in qom-get when there is no fdt
  qmp: Tighten output visitor rules
  qapi: Split visit_end_struct() into pieces
  qapi-commands: Wrap argument visit in visit_start_struct
  qom: Wrap prop visit in visit_start_struct
  qmp-input: Require struct push to visit members of top dict
  qapi: Simplify semantics of visit_next_list()
  qapi: Change visit_type_FOO() to no longer return partial objects

 include/qapi/visitor.h   | 485 +--
 include/qapi/visitor-impl.h  |  80 --
 scripts/qapi-commands.py |  10 +
 scripts/qapi-event.py|   5 +-
 scripts/qapi-visit.py|  56 ++--
 include/qapi/dealloc-visitor.h   |   4 +
 include/qapi/opts-visitor.h  |   4 +
 include/qapi/qmp-output-visitor.h|   1 +
 include/qapi/qmp/dispatch.h  |   6 -
 include/qapi/string-input-visitor.h  |   5 +
 include/qapi/string-output-visitor.h |   5 +
 qapi/qapi-visit-core.c   | 112 ++--
 hw/ppc/spapr_drc.c   |  11 +-
 hw/virtio/virtio-balloon.c   |  15 +-
 qapi/opts-visitor.c  |  65 ++---
 qapi/qapi-dealloc-visitor.c  |  43 +---
 qapi/qmp-dispatch.c  |  18 +-
 qapi/qmp-input-visitor.c | 168 +++-
 qapi/qmp-output-visitor.c|  72 +++---
 qapi/qmp-registry.c  |   1 -
 qapi/string-input-visitor.c  |  39 +--
 qapi/string-output-visitor.c |  43 ++--
 qom/object.c |   5 +-
 qom/object_interfaces.c  |  29 ++-
 tests/check-qnull.c  |  68 +
 tests/test-qmp-commands.c|  13 +-
 tests/test-qmp-input-strict.c|  19 +-
 tests/test-qmp-input-visitor.c   |  27 +-
 tests/test-qmp-output-visitor.c  |  17 +-
 docs/qapi-code-gen.txt   |  33 ++-
 tests/.gitignore |   1 +
 tests/Makefile   |   6 +-
 32 files changed, 1077 insertions(+), 389 deletions(-)
 create mode 100644 tests/check-qnull.c

-- 
2.5.0

[Qemu-devel] [PATCH v13 08/18] tests: Add check-qnull

[Eric Blake]

Add a new test, for checking reference counting of qnull(). As
part of the new file, move a previous reference counting change
added in commit a861564 into a more logical place.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 tests/check-qnull.c | 60 +
 tests/test-qmp-output-visitor.c |  2 --
 tests/.gitignore|  1 +
 tests/Makefile  |  6 -
 4 files changed, 66 insertions(+), 3 deletions(-)
 create mode 100644 tests/check-qnull.c

diff --git a/tests/check-qnull.c b/tests/check-qnull.c
new file mode 100644
index 000..b0fb1e6
--- /dev/null
+++ b/tests/check-qnull.c
@@ -0,0 +1,60 @@
+/*
+ * QNull unit-tests.
+ *
+ * Copyright (C) 2016 Red Hat Inc.
+ *
+ * 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.
+ */
+#include "qemu/osdep.h"
+#include 
+
+#include "qapi/qmp/qobject.h"
+#include "qemu-common.h"
+#include "qapi/qmp-output-visitor.h"
+
+/*
+ * Public Interface test-cases
+ *
+ * (with some violations to access 'private' data)
+ */
+
+static void qnull_ref_test(void)
+{
+QObject *obj;
+
+g_assert(qnull_.refcnt == 1);
+obj = qnull();
+g_assert(obj);
+g_assert(obj == _);
+g_assert(qnull_.refcnt == 2);
+g_assert(qobject_type(obj) == QTYPE_QNULL);
+qobject_decref(obj);
+g_assert(qnull_.refcnt == 1);
+}
+
+static void qnull_visit_test(void)
+{
+QObject *obj;
+QmpOutputVisitor *qov;
+
+g_assert(qnull_.refcnt == 1);
+qov = qmp_output_visitor_new();
+/* FIXME: Empty visits are ugly, we should have a visit_type_null(). */
+obj = qmp_output_get_qobject(qov);
+g_assert(obj == _);
+qobject_decref(obj);
+
+qmp_output_visitor_cleanup(qov);
+g_assert(qnull_.refcnt == 1);
+}
+
+int main(int argc, char **argv)
+{
+g_test_init(, , NULL);
+
+g_test_add_func("/public/qnull_ref", qnull_ref_test);
+g_test_add_func("/public/qnull_visit", qnull_visit_test);
+
+return g_test_run();
+}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index dc35752..81c90bc 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -483,8 +483,6 @@ static void test_visitor_out_empty(TestOutputVisitorData 
*data,

 arg = qmp_output_get_qobject(data->qov);
 g_assert(qobject_type(arg) == QTYPE_QNULL);
-/* Check that qnull reference counting is sane */
-g_assert(arg->refcnt == 2);
 qobject_decref(arg);
 }

diff --git a/tests/.gitignore b/tests/.gitignore
index 787c95c..615f96d 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -3,6 +3,7 @@ check-qfloat
 check-qint
 check-qjson
 check-qlist
+check-qnull
 check-qstring
 check-qom-interface
 check-qom-proplist
diff --git a/tests/Makefile b/tests/Makefile
index cd4bbd4..b0a4f8e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -16,6 +16,8 @@ check-unit-y += tests/check-qstring$(EXESUF)
 gcov-files-check-qstring-y = qobject/qstring.c
 check-unit-y += tests/check-qlist$(EXESUF)
 gcov-files-check-qlist-y = qobject/qlist.c
+check-unit-y += tests/check-qnull$(EXESUF)
+gcov-files-check-qnull-y = qobject/qnull.c
 check-unit-y += tests/check-qjson$(EXESUF)
 gcov-files-check-qjson-y = qobject/qjson.c
 check-unit-y += tests/test-qmp-output-visitor$(EXESUF)
@@ -371,7 +373,8 @@ GENERATED_HEADERS += tests/test-qapi-types.h 
tests/test-qapi-visit.h \
tests/test-qmp-introspect.h

 test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
-   tests/check-qlist.o tests/check-qfloat.o tests/check-qjson.o \
+   tests/check-qlist.o tests/check-qfloat.o tests/check-qnull.o \
+   tests/check-qjson.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
@@ -399,6 +402,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o 
$(test-util-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
 tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
 tests/check-qfloat$(EXESUF): tests/check-qfloat.o $(test-util-obj-y)
+tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o 
$(test-qom-obj-y)
 tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y)
-- 
2.5.0

[Qemu-devel] [PATCH v13 05/18] qmp-input: Don't consume input when checking has_member

[Eric Blake]

Commit e8316d7 mistakenly passed consume=true when checking if
an optional member was present, but the mistake was silently
ignored since the code happily let us extract a member more than
once.  Tighten up the input visitor to ensure that a member is
consumed exactly once.  To keep the testsuite happy in the case
of incomplete input, we have to check whether a member exists
in the dictionary before trying to remove it.

Signed-off-by: Eric Blake 

---
v13: no change
v12: new patch
---
 qapi/qmp-input-visitor.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 7428d15..4bb7b21 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -64,10 +64,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
 /* If we have a name, and we're in a dictionary, then return that
  * value. */
 if (name && qobject_type(qobj) == QTYPE_QDICT) {
-if (tos->h && consume) {
-g_hash_table_remove(tos->h, name);
+qobj = qdict_get(qobject_to_qdict(qobj), name);
+if (tos->h && consume && qobj) {
+bool removed = g_hash_table_remove(tos->h, name);
+assert(removed);
 }
-return qdict_get(qobject_to_qdict(qobj), name);
+return qobj;
 }

 /* If we are in the middle of a list, then return the next element
@@ -337,7 +339,7 @@ static void qmp_input_type_any(Visitor *v, const char 
*name, QObject **obj,
 static void qmp_input_optional(Visitor *v, const char *name, bool *present)
 {
 QmpInputVisitor *qiv = to_qiv(v);
-QObject *qobj = qmp_input_get_object(qiv, name, true);
+QObject *qobj = qmp_input_get_object(qiv, name, false);

 if (!qobj) {
 *present = false;
-- 
2.5.0

[Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Nikos Filippakis]

Hello everyone! I am interested in getting to know the codebase a little better
so that I can eventually apply for a GSOC position.
This is my first attempt at posting a patch to a mailing list, any feedback
is greatly appreciated.

Signed-off-by: Nikos Filippakis 
---
 net/net.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/net.c b/net/net.c
index aebf753..79e9d7c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const 
uint8_t *buf, int size)
 static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
int iovcnt, unsigned flags)
 {
-uint8_t buf[NET_BUFSIZE];
 uint8_t *buffer;
 size_t offset;
+ssize_t ret;
 
 if (iovcnt == 1) {
 buffer = iov[0].iov_base;
 offset = iov[0].iov_len;
 } else {
-buffer = buf;
-offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
+buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
+offset = iov_to_buf(iov, iovcnt, 0, buffer,
+NET_BUFSIZE * sizeof(uint8_t));
 }
 
 if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
-return nc->info->receive_raw(nc, buffer, offset);
+ret = nc->info->receive_raw(nc, buffer, offset);
 } else {
-return nc->info->receive(nc, buffer, offset);
+ret = nc->info->receive(nc, buffer, offset);
 }
+
+if (iovcnt != 1) {
+g_free(buffer);
+}
+
+return ret;
 }
 
 ssize_t qemu_deliver_packet_iov(NetClientState *sender,
-- 
1.9.1

[Qemu-devel] [PATCH] ui/console: add escape sequence \e[5,6n

[Ren Kimura]

This patch add support of escape sequence "\e[5,6n".
This implementation similar to that of linux tty driver.

Signed-off-by: Ren Kimura 
---
 ui/console.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index ae61382..854ec98 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -757,6 +757,25 @@ static void console_clear_xy(QemuConsole *s, int x, int y)
 update_xy(s, x, y);
 }
 
+static void console_respond_str(QemuConsole *s, const char *buf)
+{
+TextCell *c;
+int y1;
+while (*buf) {
+if (s->x >= s->width) {
+s->x = 0;
+console_put_lf(s);
+}
+y1 = (s->y_base + s->y) % s->total_height;
+c = >cells[y1 * s->width + s->x];
+c->ch = *buf;
+c->t_attrib = s->t_attrib;
+update_xy(s, s->x, s->y);
+s->x++;
+buf++;
+}
+}
+
 /* set cursor, checking bounds */
 static void set_cursor(QemuConsole *s, int x, int y)
 {
@@ -782,6 +801,7 @@ static void console_putchar(QemuConsole *s, int ch)
 TextCell *c;
 int y1, i;
 int x, y;
+char response[40];
 
 switch(s->state) {
 case TTY_STATE_NORM:
@@ -957,7 +977,17 @@ static void console_putchar(QemuConsole *s, int ch)
 break;
 case 'n':
 /* report cursor position */
-/* TODO: send ESC[row;colR */
+switch (s->esc_params[0]) {
+case 5:
+console_respond_str(s, "\033[0n");
+break;
+case 6:
+sprintf(response, "\033[%d;%dR",
+   (s->y_base + s->y) % s->total_height + 1,
+s->x + 1);
+console_respond_str(s, response);
+break;
+}
 break;
 case 's':
 /* save cursor position */
-- 
2.5.0

[Qemu-devel] [PATCH] ui/console: add escape sequence \e[5,6n

[Ren Kimura]

Signed-off-by: Ren Kimura 
---
 ui/console.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index ae61382..854ec98 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -757,6 +757,25 @@ static void console_clear_xy(QemuConsole *s, int x, int y)
 update_xy(s, x, y);
 }
 
+static void console_respond_str(QemuConsole *s, const char *buf)
+{
+TextCell *c;
+int y1;
+while (*buf) {
+if (s->x >= s->width) {
+s->x = 0;
+console_put_lf(s);
+}
+y1 = (s->y_base + s->y) % s->total_height;
+c = >cells[y1 * s->width + s->x];
+c->ch = *buf;
+c->t_attrib = s->t_attrib;
+update_xy(s, s->x, s->y);
+s->x++;
+buf++;
+}
+}
+
 /* set cursor, checking bounds */
 static void set_cursor(QemuConsole *s, int x, int y)
 {
@@ -782,6 +801,7 @@ static void console_putchar(QemuConsole *s, int ch)
 TextCell *c;
 int y1, i;
 int x, y;
+char response[40];
 
 switch(s->state) {
 case TTY_STATE_NORM:
@@ -957,7 +977,17 @@ static void console_putchar(QemuConsole *s, int ch)
 break;
 case 'n':
 /* report cursor position */
-/* TODO: send ESC[row;colR */
+switch (s->esc_params[0]) {
+case 5:
+console_respond_str(s, "\033[0n");
+break;
+case 6:
+sprintf(response, "\033[%d;%dR",
+   (s->y_base + s->y) % s->total_height + 1,
+s->x + 1);
+console_respond_str(s, response);
+break;
+}
 break;
 case 's':
 /* save cursor position */
-- 
2.5.0

Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization

[Michael S. Tsirkin]

On Fri, Mar 04, 2016 at 03:49:37PM +, Li, Liang Z wrote:
> > > > > > > Only detect the unmapped/zero mapped pages is not enough.
> > > > Consider
> > > > > > the
> > > > > > > situation like case 2, it can't achieve the same result.
> > > > > >
> > > > > > Your case 2 doesn't exist in the real world.  If people could
> > > > > > stop their main memory consumer in the guest prior to migration
> > > > > > they wouldn't need live migration at all.
> > > > >
> > > > > The case 2 is just a simplified scenario, not a real case.
> > > > > As long as the guest's memory usage does not keep increasing, or
> > > > > not always run out, it can be covered by the case 2.
> > > >
> > > > The memory usage will keep increasing due to ever growing caches,
> > > > etc, so you'll be left with very little free memory fairly soon.
> > > >
> > >
> > > I don't think so.
> > 
> > Here's my laptop:
> > KiB Mem : 16048560 total,  8574956 free,  3360532 used,  4113072 buff/cache
> > 
> > But here's a server:
> > KiB Mem:  32892768 total, 20092812 used, 12799956 free,   368704 buffers
> > 
> > What is the difference? A ton of tiny daemons not doing anything, staying
> > resident in memory.
> > 
> > > > > > I tend to think you can safely assume there's no free memory in
> > > > > > the guest, so there's little point optimizing for it.
> > > > >
> > > > > If this is true, we should not inflate the balloon either.
> > > >
> > > > We certainly should if there's "available" memory, i.e. not free but
> > > > cheap to reclaim.
> > > >
> > >
> > > What's your mean by "available" memory? if they are not free, I don't 
> > > think
> > it's cheap.
> > 
> > clean pages are cheap to drop as they don't have to be written.
> > whether they will be ever be used is another matter.
> > 
> > > > > > OTOH it makes perfect sense optimizing for the unmapped memory
> > > > > > that's made up, in particular, by the ballon, and consider
> > > > > > inflating the balloon right before migration unless you already
> > > > > > maintain it at the optimal size for other reasons (like e.g. a
> > > > > > global resource manager
> > > > optimizing the VM density).
> > > > > >
> > > > >
> > > > > Yes, I believe the current balloon works and it's simple. Do you
> > > > > take the
> > > > performance impact for consideration?
> > > > > For and 8G guest, it takes about 5s to  inflating the balloon. But
> > > > > it only takes 20ms to  traverse the free_list and construct the
> > > > > free pages
> > > > bitmap.
> > > >
> > > > I don't have any feeling of how important the difference is.  And if
> > > > the limiting factor for balloon inflation speed is the granularity
> > > > of communication it may be worth optimizing that, because quick
> > > > balloon reaction may be important in certain resource management
> > scenarios.
> > > >
> > > > > By inflating the balloon, all the guest's pages are still be
> > > > > processed (zero
> > > > page checking).
> > > >
> > > > Not sure what you mean.  If you describe the current state of
> > > > affairs that's exactly the suggested optimization point: skip unmapped
> > pages.
> > > >
> > >
> > > You'd better check the live migration code.
> > 
> > What's there to check in migration code?
> > Here's the extent of what balloon does on output:
> > 
> > 
> > while (iov_to_buf(elem->out_sg, elem->out_num, offset, , 4) == 
> > 4)
> > {
> > ram_addr_t pa;
> > ram_addr_t addr;
> > int p = virtio_ldl_p(vdev, );
> > 
> > pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> > offset += 4;
> > 
> > /* FIXME: remove get_system_memory(), but how? */
> > section = memory_region_find(get_system_memory(), pa, 1);
> > if (!int128_nz(section.size) || 
> > !memory_region_is_ram(section.mr))
> > continue;
> > 
> > 
> > trace_virtio_balloon_handle_output(memory_region_name(section.mr),
> >pa);
> > /* Using memory_region_get_ram_ptr is bending the rules a bit, 
> > but
> >should be OK because we only want a single page.  */
> > addr = section.offset_within_region;
> > balloon_page(memory_region_get_ram_ptr(section.mr) + addr,
> >  !!(vq == s->dvq));
> > memory_region_unref(section.mr);
> > }
> > 
> > so all that happens when we get a page is balloon_page.
> > and
> > 
> > static void balloon_page(void *addr, int deflate) { #if defined(__linux__)
> > if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
> >  kvm_has_sync_mmu())) {
> > qemu_madvise(addr, TARGET_PAGE_SIZE,
> > deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
> > }
> > #endif
> > }
> > 
> > 
> > Do you see anything that tracks pages to help migration skip the ballooned
> > memory? I don't.
> > 
> 
> No. And it's exactly what I mean. The ballooned memory 

Re: [Qemu-devel] [PATCH v10 3/3] qmp: add monitor command to add/remove a child

[Max Reitz]

On 16.02.2016 10:37, Changlong Xie wrote:
> From: Wen Congyang 
> 
> The new QMP command name is x-blockdev-change. It's just for adding/removing
> quorum's child now, and doesn't support all kinds of children, all kinds of
> operations, nor all block drivers. So it is experimental now.
> 
> Signed-off-by: Wen Congyang 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Gonglei 
> Signed-off-by: Changlong Xie 
> ---
>  blockdev.c   | 54 
> 
>  qapi/block-core.json | 32 +++
>  qmp-commands.hx  | 50 
>  3 files changed, 136 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1f73478..ca040b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3983,6 +3983,60 @@ out:
>  aio_context_release(aio_context);
>  }
>  
> +static BlockDriverState *bdrv_find_child(BlockDriverState *parent_bs,
> + const char *child_name)
> +{
> +BdrvChild *child;
> +
> +QLIST_FOREACH(child, _bs->children, next) {
> +if (strcmp(child->name, child_name) == 0) {
> +return child->bs;
> +}
> +}
> +
> +return NULL;
> +}

As I said for patch 1, making this function return a BdrvChild would be
trivial...

> +
> +void qmp_x_blockdev_change(const char *parent, bool has_child,
> +   const char *child, bool has_node,
> +   const char *node, Error **errp)
> +{
> +BlockDriverState *parent_bs, *child_bs = NULL, *new_bs = NULL;
> +
> +parent_bs = bdrv_lookup_bs(parent, parent, errp);
> +if (!parent_bs) {
> +return;
> +}
> +
> +if (has_child == has_node) {
> +if (has_child) {
> +error_setg(errp, "The parameters child and node are in 
> conflict");
> +} else {
> +error_setg(errp, "Either child or node must be specified");
> +}
> +return;
> +}
> +
> +if (has_child) {
> +child_bs = bdrv_find_child(parent_bs, child);
> +if (!child_bs) {
> +error_setg(errp, "Node '%s' does not have child '%s'",
> +   parent, child);
> +return;
> +}
> +bdrv_del_child(parent_bs, child_bs, errp);

...and then we could pass the BdrvChild here.

(It's your choice.)

> +}
> +
> +if (has_node) {
> +new_bs = bdrv_find_node(node);
> +if (!new_bs) {
> +error_setg(errp, "Node '%s' not found", node);
> +return;
> +}
> +bdrv_add_child(parent_bs, new_bs, errp);
> +}
> +}
> +
>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  {
>  BlockJobInfoList *head = NULL, **p_next = 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 33012b8..92eb7fe 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2482,3 +2482,35 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-blockdev-change
> +#
> +# Dynamically reconfigure the block driver state graph. It can be used
> +# to add, remove, insert or replace a graph node. Currently only the
> +# Quorum driver implements this feature to add or remove its child. This
> +# is useful to fix a broken quorum child.
> +#
> +# If @node is specified, it will be inserted under @parent. @child
> +# may not be specified in this case. If both @parent and @child are
> +# specified but @node is not, @child will be detached from @parent.
> +#
> +# @parent: the id or name of the parent node.
> +#
> +# @child: #optional the name of a child under the given parent node.
> +#
> +# @node: #optional the name of the node that will be added.
> +#
> +# Note: this command is experimental, and its API is not stable. It
> +# does not support all kinds of operations, all kinds of children, nor
> +# all block drivers.
> +#
> +# Warning: The data in a new quorum child MUST be consistent with that of
> +# the rest of the array.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'x-blockdev-change',
> +  'data' : { 'parent': 'str',
> + '*child': 'str',
> + '*node': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 020e5ee..1c9a06f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4364,6 +4364,56 @@ Example:
>  EQMP
>  
>  {
> +.name   = "x-blockdev-change",
> +.args_type  = "parent:B,child:B?,node:B?",
> +.mhandler.cmd_new = qmp_marshal_x_blockdev_change,
> +},
> +
> +SQMP
> +x-blockdev-change
> +-
> +
> +Dynamically reconfigure the block driver state graph. It can be used
> +to add, remove, insert or replace a graph node. Currently only the
> +Quorum driver implements this feature to add or remove its child. This
> +is useful 

Re: [Qemu-devel] [PATCH v10 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Max Reitz]

On 16.02.2016 10:37, Changlong Xie wrote:
> From: Wen Congyang 
> 
> Signed-off-by: Wen Congyang 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Gonglei 
> Signed-off-by: Changlong Xie 
> ---
>  block.c   |   8 ++--
>  block/quorum.c| 122 
> +-
>  include/block/block.h |   4 ++
>  3 files changed, 128 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 08aa979..c3c9dc0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1198,10 +1198,10 @@ static int bdrv_fill_options(QDict **options, const 
> char *filename,
>  return 0;
>  }
>  
> -static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> -BlockDriverState *child_bs,
> -const char *child_name,
> -const BdrvChildRole *child_role)
> +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> + BlockDriverState *child_bs,
> + const char *child_name,
> + const BdrvChildRole *child_role)
>  {
>  BdrvChild *child = g_new(BdrvChild, 1);
>  *child = (BdrvChild) {
> diff --git a/block/quorum.c b/block/quorum.c
> index a5ae4b8..e5a7e4f 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -24,6 +24,7 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qapi-event.h"
>  #include "crypto/hash.h"
> +#include "qemu/bitmap.h"
>  
>  #define HASH_LENGTH 32
>  
> @@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
>  bool rewrite_corrupted;/* true if the driver must rewrite-on-read 
> corrupted
>  * block if Quorum is reached.
>  */
> +unsigned long *index_bitmap;
> +int bsize;
>  
>  QuorumReadPattern read_pattern;
>  } BDRVQuorumState;
> @@ -876,9 +879,9 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  ret = -EINVAL;
>  goto exit;
>  }
> -if (s->num_children < 2) {
> +if (s->num_children < 1) {
>  error_setg(_err,
> -   "Number of provided children must be greater than 1");
> +   "Number of provided children must be 1 or more");

Side note: Actually, we could work with 0 children, too. Quorum would
then need to implement bdrv_is_inserted() and return false if there are
no children.

But that is something that can be implemented later on if the need arises.

>  ret = -EINVAL;
>  goto exit;
>  }
> @@ -927,6 +930,7 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  /* allocate the children array */
>  s->children = g_new0(BdrvChild *, s->num_children);
>  opened = g_new0(bool, s->num_children);
> +s->index_bitmap = bitmap_new(s->num_children);
>  
>  for (i = 0; i < s->num_children; i++) {
>  char indexstr[32];
> @@ -942,6 +946,8 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  opened[i] = true;
>  }
> +bitmap_set(s->index_bitmap, 0, s->num_children);
> +s->bsize = s->num_children;
>  
>  g_free(opened);
>  goto exit;
> @@ -998,6 +1004,115 @@ static void quorum_attach_aio_context(BlockDriverState 
> *bs,
>  }
>  }
>  
> +static int get_new_child_index(BDRVQuorumState *s)
> +{
> +int index;
> +
> +index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
> +if (index < s->bsize) {
> +return index;
> +}
> +
> +if ((s->bsize % BITS_PER_LONG) == 0) {
> +s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
> + s->bsize + 1);

I think this function needs to be called unconditionally. Looking into
its implementation, its call to g_realloc() will not do anything (and it
will probably be pretty quick at that), but the following bitmap_clear()
will only clear the bits from old_nbits (s->bsize) to new_nbits
(s->bsize + 1).

Thus, if you only call this function every 32nd/64th child, only that
child's bit will be initialized to zero. All the rest is undefined.

You probably didn't notice because bitmap_new() returns a
zero-initialized bitmap, and thus you'd have to create around 64
children (on an x64 machine) to notice.

> +}
> +
> +return s->bsize++;
> +}
> +
> +static void remove_child_index(BDRVQuorumState *s, int index)
> +{
> +int last_index;
> +long new_len;

size_t would be the more appropriate type.

> +
> +assert(index < s->bsize);
> +
> +clear_bit(index, s->index_bitmap);
> +if (index < s->bsize - 1) {
> +/*
> + * The last bit is always set, and we don't clear

s/don't/didn't/

> + * the last bit.
> + */
> +return;
> +}
> +
> +last_index = find_last_bit(s->index_bitmap, 

[Qemu-devel] [PATCH v2] target-i386: Dump unknown opcodes with -d unimp

[Richard Henderson]

We discriminate here between opcodes that are illegal in the current
cpu mode or with illegal arguments (such as modrm.mod == 3) and
encodings that are unknown (such as an unimplemented isa extension).

Signed-off-by: Richard Henderson 
---

PS: The upshot of this rule is that most if's are illegal, and most
switch default cases are unknown.


r~
---
 target-i386/translate.c | 127 +++-
 1 file changed, 83 insertions(+), 44 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 92cb1c8..dd8d5cc 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -99,6 +99,7 @@ typedef struct DisasContext {
 int prefix;
 TCGMemOp aflag;
 TCGMemOp dflag;
+target_ulong pc_start;
 target_ulong pc; /* pc = eip + cs_base */
 int is_jmp; /* 1 = means jump (stop translation), 2 means CPU
static state change (stop translation) */
@@ -2368,6 +2369,30 @@ static void gen_exception(DisasContext *s, int trapno, 
target_ulong cur_eip)
 s->is_jmp = DISAS_TB_JUMP;
 }
 
+/* Generate #UD for the current instruction.  The assumption here is that
+   the instruction is known, but it isn't allowed in the current cpu mode.  */
+static void gen_illegal_opcode(DisasContext *s)
+{
+gen_exception(s, EXCP06_ILLOP, s->pc_start - s->cs_base);
+}
+
+/* Similarly, except that the assumption here is that we don't decode
+   the instruction at all -- either a missing opcode, an unimplemented
+   feature, or just a bogus instruction stream.  */
+static void gen_unknown_opcode(CPUX86State *env, DisasContext *s)
+{
+gen_illegal_opcode(s);
+
+if (qemu_loglevel_mask(LOG_UNIMP)) {
+target_ulong pc = s->pc_start, end = s->pc;
+qemu_log("ILLOPC: " TARGET_FMT_lx ":", pc);
+for (; pc < end; ++pc) {
+qemu_log(" %02x", cpu_ldub_code(env, pc));
+}
+qemu_log("\n");
+}
+}
+
 /* an interrupt is different from an exception because of the
privilege checks */
 static void gen_interrupt(DisasContext *s, int intno,
@@ -2887,7 +2912,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 b1 = 0;
 sse_fn_epp = sse_op_table1[b][b1];
 if (!sse_fn_epp) {
-goto illegal_op;
+goto unknown_op;
 }
 if ((b <= 0x5f && b >= 0x10) || b == 0xc6 || b == 0xc2) {
 is_xmm = 1;
@@ -2906,15 +2931,19 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 }
 if (s->flags & HF_EM_MASK) {
 illegal_op:
-gen_exception(s, EXCP06_ILLOP, pc_start - s->cs_base);
+gen_illegal_opcode(s);
 return;
 }
-if (is_xmm && !(s->flags & HF_OSFXSR_MASK))
-if ((b != 0x38 && b != 0x3a) || (s->prefix & PREFIX_DATA))
-goto illegal_op;
+if (is_xmm
+&& !(s->flags & HF_OSFXSR_MASK)
+&& ((b != 0x38 && b != 0x3a) || (s->prefix & PREFIX_DATA))) {
+goto unknown_op;
+}
 if (b == 0x0e) {
-if (!(s->cpuid_ext2_features & CPUID_EXT2_3DNOW))
-goto illegal_op;
+if (!(s->cpuid_ext2_features & CPUID_EXT2_3DNOW)) {
+/* If we were fully decoding this we might use illegal_op.  */
+goto unknown_op;
+}
 /* femms */
 gen_helper_emms(cpu_env);
 return;
@@ -2939,8 +2968,9 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 b |= (b1 << 8);
 switch(b) {
 case 0x0e7: /* movntq */
-if (mod == 3)
+if (mod == 3) {
 goto illegal_op;
+}
 gen_lea_modrm(env, s, modrm);
 gen_stq_env_A0(s, offsetof(CPUX86State, fpregs[reg].mmx));
 break;
@@ -3266,7 +3296,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 case 0x172:
 case 0x173:
 if (b1 >= 2) {
-   goto illegal_op;
+   goto unknown_op;
 }
 val = cpu_ldub_code(env, s->pc++);
 if (is_xmm) {
@@ -3285,7 +3315,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 sse_fn_epp = sse_op_table2[((b - 1) & 3) * 8 +
(((modrm >> 3)) & 7)][b1];
 if (!sse_fn_epp) {
-goto illegal_op;
+goto unknown_op;
 }
 if (is_xmm) {
 rm = (modrm & 7) | REX_B(s);
@@ -3509,12 +3539,12 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 reg = ((modrm >> 3) & 7) | rex_r;
 mod = (modrm >> 6) & 3;
 if (b1 >= 2) {
-goto illegal_op;
+goto unknown_op;
 }
 
 sse_fn_epp = sse_op_table6[b].op[b1];
 if (!sse_fn_epp) {
-goto illegal_op;
+goto unknown_op;
 }
 if (!(s->cpuid_ext_features & sse_op_table6[b].ext_mask))
  

Re: [Qemu-devel] [PATCH][Outreachy]

[Paolo Bonzini]

On 05/03/2016 07:45, Sarah Khan wrote:
> util/envlist.c:This patch replaces malloc with g_malloc

This should be placed in the subject.  Please follow the instructions at
http://wiki.qemu.org/Contribute/SubmitAPatch#Submitting_your_Patches to
format the patch correctly.

> - if ((envlist = malloc(sizeof (*envlist))) == NULL)
> + if ((envlist = g_malloc(sizeof (*envlist))) == NULL)
>   return (NULL);

g_malloc will never return NULL, so you can remove the if.

> - if ((entry = malloc(sizeof (*entry))) == NULL)
> + if ((entry = g_malloc(sizeof (*entry))) == NULL)
>   return (errno);

Likewise.

>   if ((entry->ev_var = strdup(env)) == NULL) {

You could also replace strdup with g_strdup...

> - free(entry);
> + g_free(entry);
>   return (errno);
>   }
>   QLIST_INSERT_HEAD(>el_entries, entry, ev_link);
> @@ -201,8 +201,8 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
>   }
>   if (entry != NULL) {
>   QLIST_REMOVE(entry, ev_link);
> - free((char *)entry->ev_var);
> - free(entry);
> + g_free((char *)entry->ev_var);

... because here you are replacing its freeing with g_free.

> + g_free(entry);
> 
>   envlist->el_count--;
>   }
> @@ -225,7 +225,7 @@ envlist_to_environ(const envlist_t *envlist, size_t 
> *count)
>   struct envlist_entry *entry;
>   char **env, **penv;
> 
> - penv = env = malloc((envlist->el_count + 1) * sizeof (char *));
> + penv = env = g_malloc((envlist->el_count + 1) * sizeof (char *));
>   if (env == NULL)
>   return (NULL);

This if can be removed.

Note that you have included the patch twice in your message.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v10 1/3] Add new block driver interface to add/delete a BDS's child

[Max Reitz]

Sorry that I wasn't so pedantic last time; or maybe I should rather be
sorry that I'm so pedantic this time.

On 16.02.2016 10:37, Changlong Xie wrote:
> From: Wen Congyang 
> 
> In some cases, we want to take a quorum child offline, and take
> another child online.
> 
> Signed-off-by: Wen Congyang 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Gonglei 
> Signed-off-by: Changlong Xie 
> Reviewed-by: Eric Blake 
> Reviewed-by: Alberto Garcia 
> ---
>  block.c   | 50 
> +++
>  include/block/block.h |  5 +
>  include/block/block_int.h |  5 +
>  3 files changed, 60 insertions(+)
> 
> diff --git a/block.c b/block.c
> index efc3c43..08aa979 100644
> --- a/block.c
> +++ b/block.c
> @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>  QDECREF(json);
>  }
>  }
> +
> +/*
> + * Hot add/remove a BDS's child. So the user can take a child offline when
> + * it is broken and take a new child online
> + */
> +void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
> +Error **errp)
> +{
> +
> +if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) {
> +error_setg(errp, "The node %s doesn't support adding a child",

As I said in my reply to v9's patch 3 (and I see you've followed it), I
don't quite like contractions in error messages, so I'd rather like this
to be "does not" instead of "doesn't".

If you don't decide to change this patch, then feel free to leave this
as it is, because that way you can keep Eric's and Berto's R-bs.

> +   bdrv_get_device_or_node_name(parent_bs));
> +return;
> +}
> +
> +if (!QLIST_EMPTY(_bs->parents)) {
> +error_setg(errp, "The node %s already has a parent",
> +   child_bs->node_name);
> +return;
> +}
> +
> +parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp);
> +}
> +
> +void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
> +Error **errp)

I wondered before and now I'm wondering why I didn't say anything. Why
is this function taking a BDS pointer as child_bs? Why not just
"BdrvChild *child"? Its only caller will be qmp_x_blockdev_change()
which gets the child BDS from bdrv_find_child(), which could just as
well return a BdrvChild instead of a BDS pointer.

I see two advantages to this: First, it's a bit clearer since this
operation actually does not delete the child *BDS* but only the *edge*
between parent and child, and that edge is what a BdrvChild is.

And second, some block drivers may prefer the BdrvChild over the BDS
itself. They can always trivially get the BDS from the BdrvChild
structure, but the other way around is difficult.

The only disadvantage I can see is that it then becomes asymmetric to
bdrv_add_child(). But that's fine, it's a different function after all.
bdrv_add_child() creates a BdrvChild object (an edge), bdrv_del_child()
deletes it.

(I don't know whether you had this discussion with someone else before,
though. Sorry if you did, I missed it, then.)

> +{
> +BdrvChild *child;
> +
> +if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) {
> +error_setg(errp, "The node %s doesn't support removing a child",
> +   bdrv_get_device_or_node_name(parent_bs));

Again, optional s/doesn't/does not/.

> +return;
> +}
> +
> +QLIST_FOREACH(child, _bs->children, next) {
> +if (child->bs == child_bs) {
> +break;
> +}
> +}
> +
> +if (!child) {
> +error_setg(errp, "The node %s is not a child of %s",
> +   bdrv_get_device_or_node_name(child_bs),
> +   bdrv_get_device_or_node_name(parent_bs));

Is there a special reason why you are using
bdrv_get_device_or_node_name() for the child here, while in
bdrv_add_child() you directly use the node name?

Neither seems wrong to me. A child is unlikely to have a BB of its own,
but if it does, printing its name instead of the node name may be
appropriate. I'm just wondering about the difference between
bdrv_add_child() and bdrv_del_child().

Max

> +return;
> +}
> +
> +parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp);
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index 1c4f4d8..ecde190 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
>   */
>  void bdrv_drained_end(BlockDriverState *bs);
>  
> +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
> +Error **errp);
> +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child,
> +Error **errp);
> +
>  #endif
> diff --git 

[Qemu-devel] [PATCH v4 09/10] qapi: Use anonymous bases in QMP flat unions

[Eric Blake]

Now that the generator supports it, we might as well use an
anonymous base rather than breaking out a single-use Base
structure, for all three of our current QMP flat unions.

Oddly enough, this change does not affect the resulting
introspection output (because we already inline the members of
a base type into an object, and had no independent use of the
base type reachable from a command).

The case_whitelist now has to list the name of an implicit
type; the next patch will address that wart.

Signed-off-by: Eric Blake 

---
v4: retitle, merge, add BlockdevOptions
[no v3]
v2: no change
v1: no change
Previously posted as part of qapi cleanup subset F:
v6: new patch
---
 scripts/qapi.py  |  2 +-
 qapi-schema.json | 20 ---
 qapi/block-core.json | 98 
 qapi/introspect.json | 12 +--
 4 files changed, 53 insertions(+), 79 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 227f47d..ebcd207 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -62,8 +62,8 @@ returns_whitelist = [
 # Whitelist of entities allowed to violate case conventions
 case_whitelist = [
 # From QMP:
+':obj-CpuInfo-base',# CPU, visible through query-cpu
 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
-'CpuInfoBase',  # CPU, visible through query-cpu
 'CpuInfoMIPS',  # PC, visible through query-cpu
 'CpuInfoTricore',   # PC, visible through query-cpu
 'QapiErrorClass',   # all members, visible through errors
diff --git a/qapi-schema.json b/qapi-schema.json
index 362c9d8..bd03bcc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -753,9 +753,9 @@
   'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }

 ##
-# @CpuInfoBase:
+# @CpuInfo:
 #
-# Common information about a virtual CPU
+# Information about a virtual CPU
 #
 # @CPU: the index of the virtual CPU
 #
@@ -776,18 +776,10 @@
 # Notes: @halted is a transient state that changes frequently.  By the time the
 #data is sent to the client, the guest may no longer be halted.
 ##
-{ 'struct': 'CpuInfoBase',
-  'data': {'CPU': 'int', 'current': 'bool', 'halted': 'bool',
-   'qom_path': 'str', 'thread_id': 'int', 'arch': 'CpuInfoArch' } }
-
-##
-# @CpuInfo:
-#
-# Information about a virtual CPU
-#
-# Since: 0.14.0
-##
-{ 'union': 'CpuInfo', 'base': 'CpuInfoBase', 'discriminator': 'arch',
+{ 'union': 'CpuInfo',
+  'base': {'CPU': 'int', 'current': 'bool', 'halted': 'bool',
+   'qom_path': 'str', 'thread_id': 'int', 'arch': 'CpuInfoArch' },
+  'discriminator': 'arch',
   'data': { 'x86': 'CpuInfoX86',
 'sparc': 'CpuInfoSPARC',
 'ppc': 'CpuInfoPPC',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9bf1b22..b1cf77d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1644,57 +1644,6 @@
 'vmdk', 'vpc', 'vvfat' ] }

 ##
-# @BlockdevOptionsBase
-#
-# Options that are available for all block devices, independent of the block
-# driver.
-#
-# @driver:block driver name
-# @id:#optional id by which the new block device can be referred 
to.
-# This option is only allowed on the top level of blockdev-add.
-# A BlockBackend will be created by blockdev-add if and only if
-# this option is given.
-# @node-name: #optional the name of a block driver state node (Since 2.0).
-# This option is required on the top level of blockdev-add if
-# the @id option is not given there.
-# @discard:   #optional discard-related options (default: ignore)
-# @cache: #optional cache-related options
-# @aio:   #optional AIO backend (default: threads)
-# @rerror:#optional how to handle read errors on the device
-# (default: report)
-# @werror:#optional how to handle write errors on the device
-# (default: enospc)
-# @read-only: #optional whether the block device should be read-only
-# (default: false)
-# @stats-account-invalid: #optional whether to include invalid
-# operations when computing last access statistics
-# (default: true) (Since 2.5)
-# @stats-account-failed: #optional whether to include failed
-# operations when computing latency and last
-# access statistics (default: true) (Since 2.5)
-# @stats-intervals: #optional list of intervals for collecting I/O
-#   statistics, in seconds (default: none) (Since 2.5)
-# @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
-# (default: off)
-#
-# Since: 1.7
-##
-{ 'struct': 'BlockdevOptionsBase',
-  'data': { 'driver': 'BlockdevDriver',
-'*id': 'str',
-'*node-name': 'str',
-'*discard': 'BlockdevDiscardOptions',
-'*cache': 

[Qemu-devel] [PATCH v4 04/10] qapi: Emit implicit structs in generated C

[Eric Blake]

We already have several places that want to visit all the members
of an implicit object within a larger context (simple union variant,
event with anonymous data, command with anonymous arguments struct);
and will be adding another one soon (the ability to declare an
anonymous base for a flat union).  Having a C struct declared for
these implicit types, along with a visit_type_FOO_members() helper
function, will make for fewer special cases in our generator.

We do not, however, need qapi_free_FOO() or visit_type_FOO()
functions for implicit types, because they should not be used
directly outside of the generated code.  This is done by adding a
conditional in visit_object_type() for both qapi-types.py and
qapi-visit.py based on the object name.  The comparison of
"name[0] == ':'" feels a bit hacky, but beats changing the
signature of the visit_object_type() callback to pass a new
'implicit' flag.  Also, now that we WANT to output C code for
implicits, we have to change the condition in the visit_needed()
filter.

We have to special-case ':empty' as something that does not get
output: because it is a built-in type, it would be emitted in
multiple files (the main qapi-types.h and in qga-qapi-types.h)
causing compilation failure due to redefinition.  But since it
has no members, it's easier to just avoid an attempt to visit
that particular type.

The patch relies on the fact that all our implicit objects start
with a leading ':', which can be transliteratated to a leading
single underscore, and we've already documented and enforce that
the user can't create QAPI names with a leading underscore, so
exposing the types won't create any collisions.  It is a bit
unfortunate that the generated struct names don't match normal
naming conventions, but it's not too bad, since they will only
be used in generated code.

The generated code grows substantially in size; in part because
it was easier to output every implicit type rather than adding
generator complexity to try and only output types as needed.

Signed-off-by: Eric Blake 

---
v4: new patch
---
 scripts/qapi.py   |  3 +--
 scripts/qapi-types.py | 12 ++--
 scripts/qapi-visit.py | 18 --
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b1b87ee..6cf0a75 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -999,7 +999,6 @@ class QAPISchemaObjectType(QAPISchemaType):
 return self.name[0] == ':'

 def c_name(self):
-assert not self.is_implicit()
 return QAPISchemaType.c_name(self)

 def c_type(self):
@@ -1476,7 +1475,7 @@ def c_enum_const(type_name, const_name, prefix=None):
 type_name = prefix
 return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()

-c_name_trans = string.maketrans('.-', '__')
+c_name_trans = string.maketrans('.-:', '___')


 # Map @name to a valid C identifier.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f194bea..fa2d6af 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -61,8 +61,7 @@ def gen_object(name, base, members, variants):
 ret = ''
 if variants:
 for v in variants.variants:
-if (isinstance(v.type, QAPISchemaObjectType) and
-not v.type.is_implicit()):
+if isinstance(v.type, QAPISchemaObjectType):
 ret += gen_object(v.type.name, v.type.base,
   v.type.local_members, v.type.variants)

@@ -197,9 +196,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self._btin = None

 def visit_needed(self, entity):
-# Visit everything except implicit objects
-return not (entity.is_implicit() and
-isinstance(entity, QAPISchemaObjectType))
+# Visit everything except the special :empty builtin
+return entity.name != ':empty'

 def _gen_type_cleanup(self, name):
 self.decl += gen_type_cleanup_decl(name)
@@ -233,7 +231,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self.decl += gen_object(name, base, members, variants)
 if base:
 self.decl += gen_upcast(name, base)
-self._gen_type_cleanup(name)
+if name[0] != ':':
+# implicit types won't be directly allocated/freed
+self._gen_type_cleanup(name)

 def visit_alternate_type(self, name, info, variants):
 self._fwdecl += gen_fwd_object_or_array(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a712e9a..b4303a7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -215,13 +215,11 @@ out:


 def gen_visit_object(name, base, members, variants):
-ret = gen_visit_object_members(name, base, members, variants)
-
 # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
 # *obj, but then visit_type_FOO_members() fails, we should clean up *obj
 # rather than leaving it non-NULL. As 

[Qemu-devel] [PATCH v4 10/10] qapi: Populate info['name'] for each entity

[Eric Blake]

Every non-implicit entity is associated with an 'info'
dictionary, but it is not easy to reverse-engineer the name of
the top-most entity associated with that 'info'.  Our use of
'case_whitelist' (added in commit 893e1f2) is thus currently
tied to the owner of a member instead; but as the previous patch
showed with CpuInfo, this requires whitelist exceptions to know
how an implicit name will be generated.

While we have a ._pretty_owner() that maps from implicit names
back to a human readable phrase, that produces more than just a
plain top-level entity name.  What's more, the current use of
._pretty_owner() is via .check_clash(), which can be called on
the same member object more than once (once through the
standalone type, and a second time when used as a base class of
a derived tpye); if a clash is only introduced in the derived
class, using ._pretty_owner() to report the error on behalf of
the base class named in member.owner seems wrong.  Therefore,
we need a new mechanism.

Add a new info['name'] field to track the information we need,
allowing us to change 'case_whitelist' to use only names present
in the qapi files.

Unfortunately, there is no one good place to add the mapping:
at the point 'info' is created in QAPISchemaParser.__init__(),
we don't know the name.  Info is then stored into
QAPISchemaParser.exprs, which then then gets fed to
QAPISchema.__init__() via the global check_exprs(), but we want
check_exprs() to go away.  And QAPISchema._def_exprs() sees
every entity, except that the various _def_FOO() helpers don't
return anything.  So we have to modify all of the _def_FOO()
methods.

Signed-off-by: Eric Blake 

---
v4: Change series again :)
[previously posted in subset F]:
v6: sink later in series, and rework commit message
[previously posted in subset D]:
v14: rearrange assignment, improve commit message
v13: new patch
---
 scripts/qapi.py | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index ebcd207..6c991c3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -62,8 +62,8 @@ returns_whitelist = [
 # Whitelist of entities allowed to violate case conventions
 case_whitelist = [
 # From QMP:
-':obj-CpuInfo-base',# CPU, visible through query-cpu
 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
+'CpuInfo',  # CPU and PC, visible through query-cpu
 'CpuInfoMIPS',  # PC, visible through query-cpu
 'CpuInfoTricore',   # PC, visible through query-cpu
 'QapiErrorClass',   # all members, visible through errors
@@ -1035,7 +1035,7 @@ class QAPISchemaMember(object):

 def check_clash(self, info, seen):
 cname = c_name(self.name)
-if cname.lower() != cname and self.owner not in case_whitelist:
+if cname.lower() != cname and info['name'] not in case_whitelist:
 raise QAPIExprError(info,
 "%s should not use uppercase" % 
self.describe())
 if cname in seen:
@@ -1296,7 +1296,7 @@ class QAPISchema(object):
 return name

 def _def_enum_type(self, expr, info):
-name = expr['enum']
+name = info['name'] = expr['enum']
 data = expr['data']
 prefix = expr.get('prefix')
 self._def_entity(QAPISchemaEnumType(
@@ -1317,7 +1317,7 @@ class QAPISchema(object):
 for (key, value) in data.iteritems()]

 def _def_struct_type(self, expr, info):
-name = expr['struct']
+name = info['name'] = expr['struct']
 base = expr.get('base')
 data = expr['data']
 self._def_entity(QAPISchemaObjectType(name, info, base,
@@ -1336,7 +1336,7 @@ class QAPISchema(object):
 return QAPISchemaObjectTypeVariant(case, typ)

 def _def_union_type(self, expr, info):
-name = expr['union']
+name = info['name'] = expr['union']
 data = expr['data']
 base = expr.get('base')
 tag_name = expr.get('discriminator')
@@ -1362,7 +1362,7 @@ class QAPISchema(object):
   variants)))

 def _def_alternate_type(self, expr, info):
-name = expr['alternate']
+name = info['name'] = expr['alternate']
 data = expr['data']
 variants = [self._make_variant(key, value)
 for (key, value) in data.iteritems()]
@@ -1374,7 +1374,7 @@ class QAPISchema(object):
  variants)))

 def _def_command(self, expr, info):
-name = expr['command']
+name = info['name'] = expr['command']
 data = expr.get('data')
 rets = expr.get('returns')
 gen = expr.get('gen', True)
@@ -1389,7 +1389,7 @@ class QAPISchema(object):
success_response))

 def _def_event(self, expr, info):
-name = expr['event']
+name = 

[Qemu-devel] [PATCH v4 03/10] qapi: Make c_type() more OO-like

[Eric Blake]

QAPISchemaType.c_type() was a bit awkward.  Rather than having two
optional orthogonal boolean flags that should never both be true,
and where all callers should pass a compile-time constant (well,
our use of is_unboxed wasn't constant, but a future patch is about
to remove the special case for simple unions, so we can live with
the churn of refactoring the code in the meantime), the more
object-oriented approach uses different method names that can be
overridden as needed, and which convey the intent by the method
name.  The caller just makes sure to use the right variant, rather
than having to worry about boolean flags.

It requires slightly more Python, but is arguably easier to read.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 

---
v4: hoist earlier in series, rework simple union hack in qapi-types,
improve docs
[no v3]
v2: no change
---
 scripts/qapi.py   | 39 ++-
 scripts/qapi-types.py |  7 +--
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index dc26ef9..b1b87ee 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -822,8 +822,18 @@ class QAPISchemaVisitor(object):


 class QAPISchemaType(QAPISchemaEntity):
-def c_type(self, is_param=False, is_unboxed=False):
-return c_name(self.name) + pointer_suffix
+# Return the C type for common use.
+# For the types we commonly box, this is a pointer type.
+def c_type(self):
+pass
+
+# Return the C type to be used in a parameter list.
+def c_param_type(self):
+return self.c_type()
+
+# Return the C type to be used where we suppress boxing.
+def c_unboxed_type(self):
+return self.c_type()

 def c_null(self):
 return 'NULL'
@@ -855,8 +865,11 @@ class QAPISchemaBuiltinType(QAPISchemaType):
 def c_name(self):
 return self.name

-def c_type(self, is_param=False, is_unboxed=False):
-if is_param and self.name == 'str':
+def c_type(self):
+return self._c_type_name
+
+def c_param_type(self):
+if self.name == 'str':
 return 'const ' + self._c_type_name
 return self._c_type_name

@@ -889,7 +902,7 @@ class QAPISchemaEnumType(QAPISchemaType):
 # See QAPISchema._make_implicit_enum_type()
 return self.name.endswith('Kind')

-def c_type(self, is_param=False, is_unboxed=False):
+def c_type(self):
 return c_name(self.name)

 def member_names(self):
@@ -921,6 +934,9 @@ class QAPISchemaArrayType(QAPISchemaType):
 def is_implicit(self):
 return True

+def c_type(self):
+return c_name(self.name) + pointer_suffix
+
 def json_type(self):
 return 'array'

@@ -986,12 +1002,14 @@ class QAPISchemaObjectType(QAPISchemaType):
 assert not self.is_implicit()
 return QAPISchemaType.c_name(self)

-def c_type(self, is_param=False, is_unboxed=False):
+def c_type(self):
 assert not self.is_implicit()
-if is_unboxed:
-return c_name(self.name)
 return c_name(self.name) + pointer_suffix

+def c_unboxed_type(self):
+assert not self.is_implicit()
+return c_name(self.name)
+
 def json_type(self):
 return 'object'

@@ -1140,6 +1158,9 @@ class QAPISchemaAlternateType(QAPISchemaType):
 for v in self.variants.variants:
 v.check_clash(self.info, seen)

+def c_type(self):
+return c_name(self.name) + pointer_suffix
+
 def json_type(self):
 return 'value'

@@ -1631,7 +1652,7 @@ def gen_params(arg_type, extra):
 sep = ', '
 if memb.optional:
 ret += 'bool has_%s, ' % c_name(memb.name)
-ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
+ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name))
 if extra:
 ret += sep + extra
 return ret
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 0306a88..f194bea 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -124,11 +124,14 @@ def gen_variants(variants):
 for var in variants.variants:
 # Ugly special case for simple union TODO get rid of it
 simple_union_type = var.simple_union_type()
-typ = simple_union_type or var.type
+if simple_union_type:
+typ = simple_union_type.c_type()
+else:
+typ = var.type.c_unboxed_type()
 ret += mcgen('''
 %(c_type)s %(c_name)s;
 ''',
- c_type=typ.c_type(is_unboxed=not simple_union_type),
+ c_type=typ,
  c_name=c_name(var.name))

 ret += mcgen('''
-- 
2.5.0

[Qemu-devel] [PATCH v4 02/10] qapi: Fix command with named empty argument type

[Eric Blake]

The generator special-cased

 { 'command':'foo', 'data': {} }

to avoid emitting a visitor variable, but failed to see that

 { 'struct':'NamedEmptyType, 'data': {} }
 { 'command':'foo', 'data':'NamedEmptyType' }

needs the same treatment.  There, the generator happily generates a
visitor to get no arguments, and a visitor to destroy no arguments;
and the compiler isn't happy with that, as demonstrated by the updated
qapi-schema-test.json:

  tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
  tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used 
[-Werror=unused-but-set-variable]
   Visitor *v;
^

No change to generated code except for the testsuite addition.

Signed-off-by: Eric Blake 

---
v4: move earlier in series, rebase without is_empty, drop R-b
[no v3]
v2: no change
v1: enhance commit message
Previously posted as part of qapi cleanup subset E:
v9: no change
v8: no change
v7: no change
v6: new patch
---
 scripts/qapi-commands.py| 4 ++--
 tests/test-qmp-commands.c   | 5 +
 tests/qapi-schema/qapi-schema-test.json | 2 ++
 tests/qapi-schema/qapi-schema-test.out  | 2 ++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index edcbd10..3784f33 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -66,7 +66,7 @@ def gen_marshal_vars(arg_type, ret_type):
 ''',
  c_type=ret_type.c_type())

-if arg_type:
+if arg_type and arg_type.members:
 ret += mcgen('''
 QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
 QapiDeallocVisitor *qdv;
@@ -98,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type):
 def gen_marshal_input_visit(arg_type, dealloc=False):
 ret = ''

-if not arg_type:
+if not arg_type or not arg_type.members:
 return ret

 if dealloc:
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index d6171f2..650ba46 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -13,6 +13,11 @@ void qmp_user_def_cmd(Error **errp)
 {
 }

+Empty2 *qmp_user_def_cmd0(Error **errp)
+{
+return g_new0(Empty2, 1);
+}
+
 void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
 {
 }
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 728659e..e722748 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -18,6 +18,8 @@
 { 'struct': 'Empty1', 'data': { } }
 { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }

+{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
+
 # for testing override of default naming heuristic
 { 'enum': 'QEnumTwo',
   'prefix': 'QENUM_TWO',
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index f5e2a73..f531961 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -203,6 +203,8 @@ command guest-sync :obj-guest-sync-arg -> any
gen=True success_response=True
 command user_def_cmd None -> None
gen=True success_response=True
+command user_def_cmd0 Empty2 -> Empty2
+   gen=True success_response=True
 command user_def_cmd1 :obj-user_def_cmd1-arg -> None
gen=True success_response=True
 command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
-- 
2.5.0

[Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits

[Eric Blake]

Rather than generate inline per-member visits, take advantage
of the 'visit_type_FOO_members()' function for both event and
command marshalling.  This is possible now that implicit
structs can be visited like any other.

Generated code shrinks accordingly; events initialize a struct
based on parameters, such as:

|@@ -381,6 +293,9 @@ void qapi_event_send_block_job_error(con
| QmpOutputVisitor *qov;
| Visitor *v;
| QObject *obj;
|+_obj_BLOCK_JOB_ERROR_arg qapi = {
|+(char *)device, operation, action
|+};
|
| emit = qmp_event_get_func_emit();
| if (!emit) {
|@@ -396,19 +311,7 @@ void qapi_event_send_block_job_error(con
| if (err) {
| goto out;
| }
|-visit_type_str(v, "device", (char **), );
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_IoOperationType(v, "operation", , );
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_BlockErrorAction(v, "action", , );
|-if (err) {
|-goto out_obj;
|-}
|-out_obj:
|+visit_type__obj_BLOCK_JOB_ERROR_arg_members(v, , );
| visit_end_struct(v, err ? NULL : );

And command marshalling generates call arguments from a stack-allocated
struct:

|@@ -57,26 +57,15 @@ void qmp_marshal_add_fd(QDict *args, QOb
|QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
|QapiDeallocVisitor *qdv;
|Visitor *v;
|-bool has_fdset_id = false;
|-int64_t fdset_id = 0;
|-bool has_opaque = false;
|-char *opaque = NULL;
|-
|-v = qmp_input_get_visitor(qiv);
|-if (visit_optional(v, "fdset-id", _fdset_id)) {
|-visit_type_int(v, "fdset-id", _id, );
|-if (err) {
|-goto out;
|-}
|-}
|-if (visit_optional(v, "opaque", _opaque)) {
|-visit_type_str(v, "opaque", , );
|-if (err) {
|-goto out;
|-}
|+_obj_add_fd_arg qapi = {0};
|+
|+v = qmp_input_get_visitor(qiv);
|+visit_type__obj_add_fd_arg_members(v, , );
|+if (err) {
|+goto out;
| }
|
|-retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, );
|+retval = qmp_add_fd(qapi.has_fdset_id, qapi.fdset_id, qapi.has_opaque, 
qapi.opaque, );
| if (err) {
| goto out;
| }
|@@ -88,12 +77,7 @@ out:
| qmp_input_visitor_cleanup(qiv);
| qdv = qapi_dealloc_visitor_new();
| v = qapi_dealloc_get_visitor(qdv);
|-if (visit_optional(v, "fdset-id", _fdset_id)) {
|-visit_type_int(v, "fdset-id", _id, NULL);
|-}
|-if (visit_optional(v, "opaque", _opaque)) {
|-visit_type_str(v, "opaque", , NULL);
|-}
|+visit_type__obj_add_fd_arg_members(v, , NULL);
| qapi_dealloc_visitor_cleanup(qdv);
| }

Signed-off-by: Eric Blake 

---
v4: new patch
---
 scripts/qapi.py  | 25 +
 scripts/qapi-commands.py | 28 
 scripts/qapi-event.py| 16 +++-
 3 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6cf0a75..797ac7a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1640,6 +1640,7 @@ extern const char *const %(c_name)s_lookup[];
 return ret


+# Explode arg_type into a parameter list, along with extra parameters
 def gen_params(arg_type, extra):
 if not arg_type:
 return extra
@@ -1657,6 +1658,30 @@ def gen_params(arg_type, extra):
 return ret


+# Declare and initialize an object 'qapi' using parameters from gen_params()
+def gen_struct_init(typ):
+assert not typ.variants
+ret = mcgen('''
+%(c_name)s qapi = {
+''',
+c_name=typ.c_name())
+sep = ''
+for memb in typ.members:
+ret += sep
+sep = ', '
+if memb.optional:
+ret += 'has_' + c_name(memb.name) + sep
+if memb.type.name == 'str':
+# Cast away const added in gen_params()
+ret += '(char *)'
+ret += c_name(memb.name)
+ret += mcgen('''
+
+};
+''')
+return ret
+
+
 def gen_err_check(label='out', skiperr=False):
 if skiperr:
 return ''
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 3784f33..5ffc381 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -33,8 +33,8 @@ def gen_call(name, arg_type, ret_type):
 assert not arg_type.variants
 for memb in arg_type.members:
 if memb.optional:
-argstr += 'has_%s, ' % c_name(memb.name)
-argstr += '%s, ' % c_name(memb.name)
+argstr += 'qapi.has_%s, ' % c_name(memb.name)
+argstr += 'qapi.%s, ' % c_name(memb.name)

 lhs = ''
 if ret_type:
@@ -71,21 +71,10 @@ def gen_marshal_vars(arg_type, ret_type):
 QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
 QapiDeallocVisitor *qdv;
 Visitor *v;
-''')
+%(c_name)s qapi = {0};

-for memb in arg_type.members:
-if memb.optional:
-   

[Qemu-devel] [PATCH v4 00/10] easier unboxed visits/qapi implicit types

[Eric Blake]

This is patches 10-19 of v2:
https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06079.html

Depends on Markus' qapi-next branch (which includes v3 of this
series mapping to the first half of v2).

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git qapi-implicit-v4

and will soon be part of my branch at:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

The big change here is a refactoring of how to visit implicit types:
I bit the bullet, and exposed the types in qapi-types.h, along with
visit_type_FOO_members() in qapi-visit.h.  It made a lot of the
earlier series pointless (I no longer needed patch 10, 11, 13, or
15), at the expense of a couple new patches.  The generated code
gets larger, but I reclaimed some of the growth in qapi-types.h by
using the visit_type_FOO_members() functions in more places in
qapi-commands.py and qapi-events.py.  This series also rearranges
some patches posted earlier, and squashed 17-18 as part of
converting one more flat union.

I suspect there may be some conversation on how best to detect
implicit types (I hacked in "name[0] == ':'", which is not the
prettiest), but am overall pleased with how this turned out in
comparison to v2.

backport-diff gets confused by a patch split and some renames:

001/10:[down] 'qapi: Assert in places where variants are not handled'
002/10:[0006] [FC] 'qapi: Fix command with named empty argument type'
003/10:[0016] [FC] 'qapi: Make c_type() more OO-like'
004/10:[down] 'qapi: Emit implicit structs in generated C'
005/10:[down] 'qapi: Utilize implicit struct visits'
006/10:[down] 'qapi-commands: Inline single-use helpers of gen_marshal()'
007/10:[0098] [FC] 'qapi: Don't special-case simple union wrappers'
008/10:[0038] [FC] 'qapi: Allow anonymous base for flat union'
009/10:[down] 'qapi: Use anonymous bases in QMP flat unions'
010/10:[] [-C] 'qapi: Populate info['name'] for each entity'

Eric Blake (10):
  qapi: Assert in places where variants are not handled
  qapi: Fix command with named empty argument type
  qapi: Make c_type() more OO-like
  qapi: Emit implicit structs in generated C
  qapi: Utilize implicit struct visits
  qapi-commands: Inline single-use helpers of gen_marshal()
  qapi: Don't special-case simple union wrappers
  qapi: Allow anonymous base for flat union
  qapi: Use anonymous bases in QMP flat unions
  qapi: Populate info['name'] for each entity

 scripts/qapi.py| 105 ++
 scripts/qapi-commands.py   | 117 +++--
 scripts/qapi-event.py  |  17 ++---
 scripts/qapi-types.py  |  27 ---
 scripts/qapi-visit.py  |  40 +++---
 backends/baum.c|   2 +-
 backends/msmouse.c |   2 +-
 block/nbd.c|   6 +-
 block/qcow2.c  |   6 +-
 block/vmdk.c   |   8 +-
 blockdev.c |  24 +++---
 hmp.c  |   8 +-
 hw/char/escc.c |   2 +-
 hw/input/hid.c |   8 +-
 hw/input/ps2.c |   6 +-
 hw/input/virtio-input-hid.c|   8 +-
 hw/mem/pc-dimm.c   |   2 +-
 net/dump.c |   2 +-
 net/hub.c  |   2 +-
 net/l2tpv3.c   |   2 +-
 net/net.c  |   4 +-
 net/netmap.c   |   2 +-
 net/slirp.c|   2 +-
 net/socket.c   |   2 +-
 net/tap-win32.c|   2 +-
 net/tap.c  |   4 +-
 net/vde.c  |   2 +-
 net/vhost-user.c   |   2 +-
 numa.c |   4 +-
 qemu-char.c|  82 ++--
 qemu-nbd.c |   6 +-
 replay/replay-input.c  |  44 +--
 spice-qemu-char.c  |  14 ++--
 tests/test-io-channel-socket.c |  40 +-
 tests/test-qmp-commands.c  |   7 +-
 tests/test-qmp-input-visitor.c |  25 +++---
 tests/test-qmp-output-visitor.c|  24 +++---
 tpm.c  |   2 +-
 ui/console.c   |   4 +-
 ui/input-keymap.c  |  10 +--
 ui/input-legacy.c  |   8 +-
 ui/input.c |  34 -
 ui/vnc-auth-sasl.c |   3 +-
 ui/vnc.c   |  29 +++
 util/qemu-sockets.c|  35 -
 docs/qapi-code-gen.txt |  30 

[Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat union

[Eric Blake]

Rather than requiring all flat unions to explicitly create
a separate base struct, we can allow the qapi schema to specify
the common members via an inline dictionary. This is similar to
how commands can specify an inline anonymous type for its 'data'.
We already have several struct types that only exist to serve as
a single flat union's base; the next commit will clean them up.

Now that anonymous bases are legal, we need to rework the
flat-union-bad-base negative test (as previously written, it
forms what is now valid QAPI; tweak it to now provide coverage
of a new error message path), and add a positive test in
qapi-schema-test to use an anonymous base (making the integer
argument optional, for even more coverage).

Note that this patch only allows anonymous bases for flat unions;
simple unions are enough syntactic sugar that we do not want to
burden them further.  Meanwhile, while it would be easy to also
allow an anonymous base for structs, that would be quite
redundant, as the members can be put right into the struct
instead.

Signed-off-by: Eric Blake 

---
v4: rebase to use implicit type, improve commit message
[no v3]
v2: rebase onto s/fields/members/ change
v1: rebase and rework to use gen_visit_fields_call(), test new error
Previously posted as part of qapi cleanup subset F:
v6: avoid redundant error check in gen_visit_union(), rebase to
earlier gen_err_check() improvements
---
 scripts/qapi.py| 12 ++--
 scripts/qapi-types.py  | 10 ++
 docs/qapi-code-gen.txt | 30 +++---
 tests/qapi-schema/flat-union-bad-base.err  |  2 +-
 tests/qapi-schema/flat-union-bad-base.json |  5 ++---
 tests/qapi-schema/qapi-schema-test.json|  6 +-
 tests/qapi-schema/qapi-schema-test.out | 10 +-
 7 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0574b8b..227f47d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -327,6 +327,8 @@ class QAPISchemaParser(object):


 def find_base_members(base):
+if isinstance(base, dict):
+return base
 base_struct_define = find_struct(base)
 if not base_struct_define:
 return None
@@ -560,9 +562,10 @@ def check_union(expr, expr_info):

 # Else, it's a flat union.
 else:
-# The object must have a string member 'base'.
+# The object must have a string or dictionary 'base'.
 check_type(expr_info, "'base' for union '%s'" % name,
-   base, allow_metas=['struct'])
+   base, allow_dict=True, allow_optional=True,
+   allow_metas=['struct'])
 if not base:
 raise QAPIExprError(expr_info,
 "Flat union '%s' must have a base"
@@ -1049,6 +1052,8 @@ class QAPISchemaMember(object):
 owner = owner[5:]
 if owner.endswith('-arg'):
 return '(parameter of %s)' % owner[:-4]
+elif owner.endswith('-base'):
+return '(base of %s)' % owner[:-5]
 else:
 assert owner.endswith('-wrapper')
 # Unreachable and not implemented
@@ -1336,6 +1341,9 @@ class QAPISchema(object):
 base = expr.get('base')
 tag_name = expr.get('discriminator')
 tag_member = None
+if isinstance(base, dict):
+base = (self._make_implicit_object_type(
+name, info, 'base', self._make_members(base, info)))
 if tag_name:
 variants = [self._make_variant(key, value)
 for (key, value) in data.iteritems()]
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b8499ac..c003721 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -72,12 +72,14 @@ struct %(c_name)s {
  c_name=c_name(name))

 if base:
-ret += mcgen('''
+if not base.is_implicit():
+ret += mcgen('''
 /* Members inherited from %(c_name)s: */
 ''',
- c_name=base.c_name())
+ c_name=base.c_name())
 ret += gen_struct_members(base.members)
-ret += mcgen('''
+if not base.is_implicit():
+ret += mcgen('''
 /* Own members: */
 ''')
 ret += gen_struct_members(members)
@@ -223,7 +225,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 def visit_object_type(self, name, info, base, members, variants):
 self._fwdecl += gen_fwd_object_or_array(name)
 self.decl += gen_object(name, base, members, variants)
-if base:
+if base and not base.is_implicit():
 self.decl += gen_upcast(name, base)
 if name[0] != ':':
 # implicit types won't be directly allocated/freed
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index e0b2ef1..a92d4da 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ 

[Qemu-devel] [PATCH v4 07/10] qapi: Don't special-case simple union wrappers

[Eric Blake]

Simple unions were carrying a special case that hid their 'data'
QMP member from the resulting C struct, via the hack method
QAPISchemaObjectTypeVariant.simple_union_type().  But by using
the work we started by unboxing flat union and alternate
branches, coupled with the ability to visit the members of an
implicit type, we can now expose the simple union's implicit
type in qapi-types.h:

| struct _obj_ImageInfoSpecificQCow2_wrapper {
| ImageInfoSpecificQCow2 *data;
| };
|
| struct _obj_ImageInfoSpecificVmdk_wrapper {
| ImageInfoSpecificVmdk *data;
| };
...
| struct ImageInfoSpecific {
| ImageInfoSpecificKind type;
| union { /* union tag is @type */
| void *data;
|-ImageInfoSpecificQCow2 *qcow2;
|-ImageInfoSpecificVmdk *vmdk;
|+_obj_ImageInfoSpecificQCow2_wrapper qcow2;
|+_obj_ImageInfoSpecificVmdk_wrapper vmdk;
| } u;
| };

Doing this removes asymmetry between QAPI's QMP side and its
C side (both sides now expose 'data'), and means that the
treatment of a simple union as sugar for a flat union is now
equivalent in both languages (previously the two approaches used
a different layer of dereferencing, where the simple union could
be converted to a flat union with equivalent C layout but
different {} on the wire, or to an equivalent QMP wire form
but with different C representation).  Using the implicit type
also lets us get rid of the simple_union_type() hack.

Of course, now all clients of simple unions have to adjust from
using su->u.member to using su->u.member.data; while this touches
a number of files in the tree, some earlier cleanup patches
helped minimize the change to the initialization of a temporary
variable rather than every single member access.  The generated
qapi-visit.c code is also affected by the layout change:

|@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member
| }
| switch (obj->type) {
| case IMAGE_INFO_SPECIFIC_KIND_QCOW2:
|-visit_type_ImageInfoSpecificQCow2(v, "data", >u.qcow2, );
|+visit_type__obj_ImageInfoSpecificQCow2_wrapper_members(v, 
>u.qcow2, );
| break;
| case IMAGE_INFO_SPECIFIC_KIND_VMDK:
|-visit_type_ImageInfoSpecificVmdk(v, "data", >u.vmdk, );
|+visit_type__obj_ImageInfoSpecificVmdk_wrapper_members(v, 
>u.vmdk, );
| break;
| default:
| abort();

Signed-off-by: Eric Blake 

---
v4: improve commit message, rebase onto exposed implicit types
[no v3]
v2: rebase onto s/fields/members/ change, changes in master; pick
up missing net/ files
---
 scripts/qapi.py | 11 --
 scripts/qapi-types.py   |  8 +---
 scripts/qapi-visit.py   | 22 ++-
 backends/baum.c |  2 +-
 backends/msmouse.c  |  2 +-
 block/nbd.c |  6 +--
 block/qcow2.c   |  6 +--
 block/vmdk.c|  8 ++--
 blockdev.c  | 24 ++--
 hmp.c   |  8 ++--
 hw/char/escc.c  |  2 +-
 hw/input/hid.c  |  8 ++--
 hw/input/ps2.c  |  6 +--
 hw/input/virtio-input-hid.c |  8 ++--
 hw/mem/pc-dimm.c|  2 +-
 net/dump.c  |  2 +-
 net/hub.c   |  2 +-
 net/l2tpv3.c|  2 +-
 net/net.c   |  4 +-
 net/netmap.c|  2 +-
 net/slirp.c |  2 +-
 net/socket.c|  2 +-
 net/tap-win32.c |  2 +-
 net/tap.c   |  4 +-
 net/vde.c   |  2 +-
 net/vhost-user.c|  2 +-
 numa.c  |  4 +-
 qemu-char.c | 82 +
 qemu-nbd.c  |  6 +--
 replay/replay-input.c   | 44 +++---
 spice-qemu-char.c   | 14 ---
 tests/test-io-channel-socket.c  | 40 ++--
 tests/test-qmp-commands.c   |  2 +-
 tests/test-qmp-input-visitor.c  | 25 +++--
 tests/test-qmp-output-visitor.c | 24 ++--
 tpm.c   |  2 +-
 ui/console.c|  4 +-
 ui/input-keymap.c   | 10 ++---
 ui/input-legacy.c   |  8 ++--
 ui/input.c  | 34 -
 ui/vnc-auth-sasl.c  |  3 +-
 ui/vnc.c| 29 ---
 util/qemu-sockets.c | 35 +-
 43 files changed, 246 insertions(+), 269 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 797ac7a..0574b8b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1006,7 +1006,6 @@ class QAPISchemaObjectType(QAPISchemaType):
 return c_name(self.name) + pointer_suffix

 def c_unboxed_type(self):
-assert not self.is_implicit()
 return c_name(self.name)

 def json_type(self):
@@ -1126,16 +1125,6 @@ class 

[Qemu-devel] [PATCH v4 06/10] qapi-commands: Inline single-use helpers of gen_marshal()

[Eric Blake]

Originally, gen_marshal_input_visit() (or gen_visitor_input_block()
before commit f1538019) was factored out to make it easy to do two
passes of a visit to each member of a (possibly-implicit) object,
without duplicating lots of code.  But after recent changes, those
visits now occupy a single line of emitted code, and the helper
method has become a series of conditionals both before and after
the one important line, making it rather awkward to see at a glance
what gets emitted on the first (parsing) or second (deallocation)
pass.  It's a lot easier to read the generator code if we just
inline both uses directly into gen_marshal(), without all the
conditionals.

Once we've done that, it's easy to notice that gen_marshal_vars()
is used only once, and inlining it too lets us consolidate some
mcgen() calls that used to be split across helpers.

gen_call() remains a single-use helper function, but it has
enough indentation and complexity that inlining it would hamper
legibility.

No change to generated output.  The fact that the diffstat shows
a net reduction in lines is an argument in favor of this cleanup.

Signed-off-by: Eric Blake 

---
v4: new patch
---
 scripts/qapi-commands.py | 106 +--
 1 file changed, 39 insertions(+), 67 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 5ffc381..710e853 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -55,68 +55,6 @@ def gen_call(name, arg_type, ret_type):
 return ret


-def gen_marshal_vars(arg_type, ret_type):
-ret = mcgen('''
-Error *err = NULL;
-''')
-
-if ret_type:
-ret += mcgen('''
-%(c_type)s retval;
-''',
- c_type=ret_type.c_type())
-
-if arg_type and arg_type.members:
-ret += mcgen('''
-QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
-QapiDeallocVisitor *qdv;
-Visitor *v;
-%(c_name)s qapi = {0};
-
-''',
- c_name=arg_type.c_name())
-else:
-ret += mcgen('''
-
-(void)args;
-''')
-
-return ret
-
-
-def gen_marshal_input_visit(arg_type, dealloc=False):
-ret = ''
-
-if not arg_type or not arg_type.members:
-return ret
-
-if dealloc:
-ret += mcgen('''
-qmp_input_visitor_cleanup(qiv);
-qdv = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(qdv);
-''')
-errp = 'NULL'
-else:
-ret += mcgen('''
-v = qmp_input_get_visitor(qiv);
-''')
-errp = ''
-
-ret += mcgen('''
-visit_type_%(c_name)s_members(v, , %(errp)s);
-''',
- c_name=arg_type.c_name(), errp=errp)
-
-if dealloc:
-ret += mcgen('''
-qapi_dealloc_visitor_cleanup(qdv);
-''')
-else:
-ret += gen_err_check()
-return ret
-
-
 def gen_marshal_output(ret_type):
 return mcgen('''

@@ -165,15 +103,40 @@ def gen_marshal(name, arg_type, ret_type):

 %(proto)s
 {
+Error *err = NULL;
 ''',
 proto=gen_marshal_proto(name))

-ret += gen_marshal_vars(arg_type, ret_type)
-ret += gen_marshal_input_visit(arg_type)
+if ret_type:
+ret += mcgen('''
+%(c_type)s retval;
+''',
+ c_type=ret_type.c_type())
+
+if arg_type and arg_type.members:
+ret += mcgen('''
+QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+QapiDeallocVisitor *qdv;
+Visitor *v;
+%(c_name)s qapi = {0};
+
+v = qmp_input_get_visitor(qiv);
+visit_type_%(c_name)s_members(v, , );
+if (err) {
+goto out;
+}
+''',
+ c_name=arg_type.c_name())
+
+else:
+ret += mcgen('''
+
+(void)args;
+''')
+
 ret += gen_call(name, arg_type, ret_type)

-# 'goto out' produced by gen_marshal_input_visit->gen_visit_members()
-# for each arg_type member, and by gen_call() for ret_type
+# 'goto out' produced above for arg_type, and by gen_call() for ret_type
 if (arg_type and arg_type.members) or ret_type:
 ret += mcgen('''

@@ -182,7 +145,16 @@ out:
 ret += mcgen('''
 error_propagate(errp, err);
 ''')
-ret += gen_marshal_input_visit(arg_type, dealloc=True)
+if arg_type and arg_type.members:
+ret += mcgen('''
+qmp_input_visitor_cleanup(qiv);
+qdv = qapi_dealloc_visitor_new();
+v = qapi_dealloc_get_visitor(qdv);
+visit_type_%(c_name)s_members(v, , NULL);
+qapi_dealloc_visitor_cleanup(qdv);
+''',
+ c_name=arg_type.c_name())
+
 ret += mcgen('''
 }
 ''')
-- 
2.5.0

[Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants are not handled

[Eric Blake]

We are getting closer to the point where we could use one union
as the base or variant type within another union type (as long
as there are no collisions between any possible combination of
member names allowed across all discriminator choices).  But
until we get to that point, it is worth asserting that variants
are not present in places where we are not prepared to handle
them: base types must still be plain structs, and anywhere we
explode a struct into a parameter list (events and command
marshalling), we don't support variants in that explosion.

Signed-off-by: Eric Blake 

---
v4: new patch, split out from .is_empty() patch
---
 scripts/qapi.py  | 1 +
 scripts/qapi-commands.py | 3 ++-
 scripts/qapi-event.py| 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6b2aa6e..dc26ef9 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -960,6 +960,7 @@ class QAPISchemaObjectType(QAPISchemaType):
 assert isinstance(self.base, QAPISchemaObjectType)
 self.base.check(schema)
 self.base.check_clash(schema, self.info, seen)
+assert not self.base.variants
 for m in self.local_members:
 m.check(schema)
 m.check_clash(self.info, seen)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f44e01f..edcbd10 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -2,7 +2,7 @@
 # QAPI command marshaller generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (C) 2014-2015 Red Hat, Inc.
+# Copyright (C) 2014-2016 Red Hat, Inc.
 #
 # Authors:
 #  Anthony Liguori 
@@ -30,6 +30,7 @@ def gen_call(name, arg_type, ret_type):

 argstr = ''
 if arg_type:
+assert not arg_type.variants
 for memb in arg_type.members:
 if memb.optional:
 argstr += 'has_%s, ' % c_name(memb.name)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index fb579dd..c03cb78 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -59,6 +59,7 @@ def gen_event_send(name, arg_type):
  name=name)

 if arg_type and arg_type.members:
+assert not arg_type.variants
 ret += mcgen('''
 qov = qmp_output_visitor_new();
 v = qmp_output_get_visitor(qov);
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 01/10] qdict: implement a qdict_crumple method for un-flattening a dict

[Max Reitz]

On 03.03.2016 12:01, Daniel P. Berrange wrote:
> On Wed, Mar 02, 2016 at 05:13:59PM +0100, Max Reitz wrote:
>> On 19.02.2016 17:47, Daniel P. Berrange wrote:
>>> The qdict_flatten() method will take a dict whose elements are
>>> further nested dicts/lists and flatten them by concatenating
>>> keys.
>>>
>>> The qdict_crumple() method aims todo the reverse, taking a flat
>>> qdict, and turning it into a set of nested dicts/lists. It will
>>> apply nesting based on the key name, with a '.' indicating a
>>> new level in the hierarchy. If the keys in the nested structure
>>> are all numeric, it will create a list, otherwise it will create
>>> a dict.
>>>
>>> If the keys are a mixture of numeric and non-numeric, or the
>>> numeric keys are not in strictly ascending order, an error will
>>> be reported.

[...]

>>> +if (!child) {
>>> +goto error;
>>> +}
>>> +
>>> +qdict_put_obj(tmp2, entry->key, child);
>>> +} else {
>>> +qobject_incref(entry->value);
>>> +qdict_put_obj(tmp2, entry->key, entry->value);
>>> +}
>>> +
>>> +entry = next;
>>> +}
>>> +QDECREF(tmp1);
>>> +
>>> +/* Step 3: detect if we need to turn our dict into list */
>>
>> You could use qdict_array_entries(tmp2, "") > 0 for this.
>>
>> If you do want to make { "0": 42, "2": 23 } and { "0": 42, "x": 23 }
>> errors (my qdict_unflatten() just kept those QDicts), then you'd have to
>> check on qdict_array_entries() error whether the QDict contained an
>> integer key, but that would still be simpler than the following loop and
>> the check in step 4.
> 
> If I use qdict_array_entries() then it does 2 O(N) iterations
> of the input qdict, and to check the errors, I'll have todo
> yet another iteration.  My inlined code here does everything
> in a single O(N) iteration instead of 3. I think that's
> compelling enough to reason to not use that function.

O(3N) = O(N) :-)

Making qdict_array_entries() O(N) (pretending that O(N) and O(2N) were
different things) for this case is trivial: Just omit the second loop if
"" has been passed as the subqdict.

So then you'd end up with "O(2N)", if you do the error checking.

So you are right, there is a reason not to use qdict_array_entries(),
but I'd personally still use it. I only have very limited knowledge of
the whole qemu code base, but it doesn't appear to me as if QDict
functions are ever used in a hot path or as if QDicts ever contain a
large number of elements (with large being more than, say, 1000),
especially not the kind of QDicts you'd want to crumple.

Because of that, I chose to use these existing functions, even while
knowing that they are probably not optimal for this case.

You did propose removing qdict_array_entries() and qdict_array_split()
in favor of just using this function in their stead (see my reply
below), so if we do so, reusing them would obviously not be ideal any
more. In that case, I'd still put this into its own static function if
possible.

tl;dr: I don't think runtime complexity is an issue here, but if we are
going to drop qdict_array_entries() and qdict_array_split() anyway, then
using them is a bad idea, obviously. It would then still be nice to put
this into an own function.

>>> +entry = qdict_first(tmp2);
>>> +while (entry != NULL) {
>>> +next = qdict_next(tmp2, entry);
>>> +
>>> +errno = 0;
>>> +if (qemu_strtoll(entry->key, NULL, 10, ) == 0) {
>>> +if (!dst) {
>>> +dst = (QObject *)qlist_new();
>>> +isList = true;
>>> +} else if (!isList) {
>>> +error_setg(errp,
>>> +   "Key '%s' is for a list, but previous key is "
>>> +   "for a dict", entry->key);
>>> +goto error;
>>> +}
>>> +listLen++;
>>> +if (val > listMax) {
>>> +listMax = val;
>>> +}
>>> +} else {
>>> +if (!dst) {
>>> +dst = (QObject *)tmp2;
>>> +qobject_incref(dst);
>>> +isList = false;
>>> +} else if (isList) {
>>> +error_setg(errp,
>>> +   "Key '%s' is for a dict, but previous key is "
>>> +   "for a list", entry->key);
>>> +goto error;
>>> +}
>>> +}
>>> +
>>> +entry = next;
>>> +}
>>> +
>>> +/* Step 4: Turn the dict into a list */
>>
>> Why not just use qdict_array_split(tmp2, );?
> 
> Again qdict_array_split() is a pretty inefficiently written
> function. It does multiple nested iterations over its input
> giving it O(n^2) time, compared to O(n) for my code here.

Strictly speaking, it's not O(n^2) but O(nm), where n is qdict_size(src)
and m is the size of the resulting QList.

(Side note: This function only is O(n) if qdict_get() is O(1), which it
is in best case. In worst case, 

Re: [Qemu-devel] [PATCH v8 5/9] qemu-log: new option -dfilter to limit output

[Richard Henderson]

On 03/04/2016 03:18 AM, Alex Bennée wrote:

+case '+':
+{
+unsigned long len;
+err |= qemu_strtoull(r2, NULL, 0, );
+if (len > 0) {
+range.end = range.begin + (len - 1);
+} else {
+err |= true;
+}
+break;
+}
+case '-':
+{
+unsigned long len;
+err |= qemu_strtoull(r2, NULL, 0, );
+range.end = range.begin;
+range.begin = range.end - len;
+break;
+}


Doesn't '-' have the same off-by-one bug?
Although really I don't understand the need for negative ranges...


r~

[Qemu-devel] [PATCH] bitmap: refine and move BITMAP_{FIRST/LAST}_WORD_MASK

[Wei Yang]

According to linux kernel commit <89c1e79eb30> ("linux/bitmap.h: improve
BITMAP_{LAST,FIRST}_WORD_MASK"), these two macro could be improved.

This patch takes this change and also move them all in header file.

Signed-off-by: Wei Yang 
---
 include/qemu/bitmap.h | 7 ++-
 util/bitmap.c | 2 --
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 0e33fa5..864982d 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -58,11 +58,8 @@
  * find_next_bit(addr, nbits, bit) Position next set bit in *addr >= bit
  */
 
-#define BITMAP_LAST_WORD_MASK(nbits)\
-(   \
-((nbits) % BITS_PER_LONG) ? \
-(1UL<<((nbits) % BITS_PER_LONG))-1 : ~0UL   \
-)
+#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
+#define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
 
 #define DECLARE_BITMAP(name,bits)  \
 unsigned long name[BITS_TO_LONGS(bits)]
diff --git a/util/bitmap.c b/util/bitmap.c
index 40aadfb..43ed011 100644
--- a/util/bitmap.c
+++ b/util/bitmap.c
@@ -157,8 +157,6 @@ int slow_bitmap_andnot(unsigned long *dst, const unsigned 
long *bitmap1,
 return result != 0;
 }
 
-#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) % BITS_PER_LONG))
-
 void bitmap_set(unsigned long *map, long start, long nr)
 {
 unsigned long *p = map + BIT_WORD(start);
-- 
2.5.0

Re: [Qemu-devel] [PATCH v4 RFC 00/17] qcow2: persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

On 04.03.2016 21:00, John Snow wrote:


On 02/17/2016 10:28 AM, Vladimir Sementsov-Ogievskiy wrote:

This series add persistent dirty bitmaps feature to qcow2.
Specification is in docs/spec/qcow2.txt (not merged yet, see
[PATCH v10] spec: add qcow2 bitmaps extension specification)

This series are based on Fam's
[PATCH v2 00/13] Dirty bitmap changes for migration/persistence work
(meta bitmaps not used, and only hbitmap_deserialize_finish from
serialization)

This also needs some preparation patches, most of them are in my
bitmap-migration series. I've not listed them here to keep things
simpler, this is RFC any way.

v4:

Previous version was posted more than five months ago, so I will not
carefully list all changes.

What should be noted:
  - some changes are made to sutisfy last version of specification
- removed staff, related to possibility of saving bitmaps for one
  disk in the other qcow2.
  - to make bitmap store/load zero-copy, I've moved load/store code to
HBitmap - this is new patch 01.
so, bdrv_dirty_bitmap_serialize_part and friends are not needed,
only hbitmap_deserialize_finish, to repair bitmap consistency after
loading its last level.
  - two patches added about AUTO and EXTRA_DATA_COMPATIBLE flags
  - some fixes made after John's comments on v3

Vladimir Sementsov-Ogievskiy (17):
   hbitmap: load/store
   qcow2: Bitmaps extension: structs and consts
   qcow2-dirty-bitmap: read dirty bitmap directory
   qcow2-dirty-bitmap: add qcow2_bitmap_load()
   qcow2-dirty-bitmap: add qcow2_bitmap_store()
   qcow2: add dirty bitmaps extension
   qcow2-dirty-bitmap: add qcow2_bitmap_load_check()
   block: store persistent dirty bitmaps
   block: add bdrv_load_dirty_bitmap()
   qcow2-dirty-bitmap: add autoclear bit
   qemu: command line option for dirty bitmaps
   qcow2-dirty-bitmap: add IN_USE flag
   qcow2-dirty-bitmaps: disallow stroing bitmap to other bs
   iotests: add VM.test_launcn()
   iotests: test internal persistent dirty bitmap
   qcow2-dirty-bitmap: add AUTO flag
   qcow2-dirty-bitmap: add EXTRA_DATA_COMPATIBLE flag

  block.c   |   3 +
  block/Makefile.objs   |   2 +-
  block/dirty-bitmap.c  | 101 +
  block/qcow2-dirty-bitmap.c| 839 ++
  block/qcow2.c | 105 +-
  block/qcow2.h |  59 +++
  blockdev.c|  36 ++
  include/block/block_int.h |   9 +
  include/block/dirty-bitmap.h  |  21 ++
  include/qemu/hbitmap.h|  12 +
  include/sysemu/blockdev.h |   1 +
  include/sysemu/sysemu.h   |   1 +
  qemu-options.hx   |  35 ++
  tests/qemu-iotests/160| 112 ++
  tests/qemu-iotests/160.out|  21 ++
  tests/qemu-iotests/group  |   1 +
  tests/qemu-iotests/iotests.py |  25 ++
  util/hbitmap.c| 182 +
  vl.c  |  78 
  19 files changed, 1640 insertions(+), 3 deletions(-)
  create mode 100644 block/qcow2-dirty-bitmap.c
  create mode 100755 tests/qemu-iotests/160
  create mode 100644 tests/qemu-iotests/160.out


In your prerequisite patches,

"iotests-add-default-node-name" breaks qemu iotests 055 and 118. Didn't
look hard enough to find out why, yet.


118 doesn't work for me anyway (./check -qcow2 118 , yes?), for 055 you 
are right, it's not hard, the following helps:


 def add_drive(self, path, opts='', interface='virtio'):
 '''Add a virtio-blk drive to the VM'''
 options = ['if=%s' % interface,
-   'id=drive%d' % self._num_drives,
-   'node-name=drivenode%d' % self._num_drives]
+   'id=drive%d' % self._num_drives]

 if path is not None:
+options.append('node-name=drivenode%d' % self._num_drives)
 options.append('file=%s' % path)
 options.append('format=%s' % imgfmt)
 options.append('cache=%s' % cachemode)

--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 02/10] ipmi: replace IPMI_ADD_RSP_DATA() macro with inline helpers

[Corey Minyard]

On 03/02/2016 04:14 AM, Cédric Le Goater wrote:

The IPMI command handlers in the BMC simulator use a macro
IPMI_ADD_RSP_DATA() to push bytes in a response buffer. The macro
hides the fact that it implicitly uses variables local to the handler,
which is misleading.

This patch introduces a simple 'struct rsp_buffer' and inlined helper
routines to store byte(s) in a response buffer. rsp_buffer_push()
replaces the macro IPMI_ADD_RSP_DATA() and rsp_buffer_pushmore() is
new helper to push multiple bytes. The latest is used in the command
handlers get_msg() and get_sdr() which are manipulating the buffer
directly.

Signed-off-by: Cédric Le Goater 


Acked-by: Corey Minyard 


---
  hw/ipmi/ipmi_bmc_sim.c | 482 +++--
  1 file changed, 229 insertions(+), 253 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 30b9fb48ea2d..32efb87c8232 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -153,14 +153,14 @@ typedef struct IPMISensor {
  #define IPMI_WATCHDOG_SENSOR 0

  typedef struct IPMIBmcSim IPMIBmcSim;
+struct rsp_buffer;

  #define MAX_NETFNS 64

  typedef struct IPMICmdHandler {
  void (*cmd_handler)(IPMIBmcSim *s,
  uint8_t *cmd, unsigned int cmd_len,
-uint8_t *rsp, unsigned int *rsp_len,
-unsigned int max_rsp_len);
+struct rsp_buffer *rsp);
  unsigned int cmd_len_min;
  } IPMICmdHandler;

@@ -263,22 +263,41 @@ struct IPMIBmcSim {
  #define IPMI_BMC_WATCHDOG_ACTION_POWER_DOWN  2
  #define IPMI_BMC_WATCHDOG_ACTION_POWER_CYCLE 3

+struct rsp_buffer {
+uint8_t buffer[MAX_IPMI_MSG_SIZE];
+unsigned int len;
+unsigned int max_len;
+};
+
+#define RSP_BUFFER_INITIALIZER { { 0 }, 0, MAX_IPMI_MSG_SIZE }

  /* Add a byte to the response. */
-#define IPMI_ADD_RSP_DATA(b) \
-do {   \
-if (*rsp_len >= max_rsp_len) { \
-rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;   \
-return;\
-}  \
-rsp[(*rsp_len)++] = (b);   \
-} while (0)
+static inline void rsp_buffer_push(struct rsp_buffer *rsp, uint8_t byte)
+{
+if (rsp->len >= rsp->max_len) {
+rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+return;
+}
+rsp->buffer[rsp->len++] = byte;
+}
+
+static inline void rsp_buffer_pushmore(struct rsp_buffer *rsp, uint8_t *bytes,
+   unsigned int n)
+{
+if (rsp->len + n >= rsp->max_len) {
+rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+return;
+}
+
+memcpy(>buffer[rsp->len], bytes, n);
+rsp->len += n;
+}

  /* Check that the reservation in the command is valid. */
  #define IPMI_CHECK_RESERVATION(off, r) \
  do {   \
  if ((cmd[off] | (cmd[off + 1] << 8)) != r) {   \
-rsp[2] = IPMI_CC_INVALID_RESERVATION;  \
+rsp->buffer[2] = IPMI_CC_INVALID_RESERVATION; \
  return;\
  }  \
  } while (0)
@@ -585,35 +604,32 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
  IPMIInterface *s = ibs->parent.intf;
  IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
  unsigned int netfn;
-uint8_t rsp[MAX_IPMI_MSG_SIZE];
-unsigned int rsp_len_holder = 0;
-unsigned int *rsp_len = _len_holder;
-unsigned int max_rsp_len = sizeof(rsp);
+struct rsp_buffer rsp = RSP_BUFFER_INITIALIZER;

  /* Set up the response, set the low bit of NETFN. */
  /* Note that max_rsp_len must be at least 3 */
-if (max_rsp_len < 3) {
-rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+if (rsp.max_len < 3) {
+rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
  goto out;
  }

-IPMI_ADD_RSP_DATA(cmd[0] | 0x04);
-IPMI_ADD_RSP_DATA(cmd[1]);
-IPMI_ADD_RSP_DATA(0); /* Assume success */
+rsp_buffer_push(, cmd[0] | 0x04);
+rsp_buffer_push(, cmd[1]);
+rsp_buffer_push(, 0); /* Assume success */

  /* If it's too short or it was truncated, return an error. */
  if (cmd_len < 2) {
-rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
  goto out;
  }
  if (cmd_len > max_cmd_len) {
-rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
  goto out;
  }

  if ((cmd[0] & 0x03) != 0) {
  /* Only have stuff on LUN 0 */
-rsp[2] = IPMI_CC_COMMAND_INVALID_FOR_LUN;
+rsp.buffer[2] = IPMI_CC_COMMAND_INVALID_FOR_LUN;
  goto out;
  }

@@ -623,20 +639,20 @@ 

Re: [Qemu-devel] [PATCH v2 04/10] ipmi: add rsp_buffer_set_error() helper

[Corey Minyard]

On 03/02/2016 04:14 AM, Cédric Le Goater wrote:

The third byte in the response buffer of an IPMI command holds the
error code. In many IPMI command handlers, this byte is updated
directly. This patch adds a helper routine to clarify why this byte is
being used.

Signed-off-by: Cédric Le Goater 


Yes, more clear.

Acked-by: Corey Minyard 


---
  hw/ipmi/ipmi_bmc_sim.c | 115 ++---
  1 file changed, 60 insertions(+), 55 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 72166a7da291..2626aff06c60 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -271,11 +271,16 @@ struct rsp_buffer {

  #define RSP_BUFFER_INITIALIZER { { 0 }, 0, MAX_IPMI_MSG_SIZE }

+static inline void rsp_buffer_set_error(struct rsp_buffer *rsp, uint8_t byte)
+{
+rsp->buffer[2] = byte;
+}
+
  /* Add a byte to the response. */
  static inline void rsp_buffer_push(struct rsp_buffer *rsp, uint8_t byte)
  {
  if (rsp->len >= rsp->max_len) {
-rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
  return;
  }
  rsp->buffer[rsp->len++] = byte;
@@ -285,7 +290,7 @@ static inline void rsp_buffer_pushmore(struct rsp_buffer 
*rsp, uint8_t *bytes,
 unsigned int n)
  {
  if (rsp->len + n >= rsp->max_len) {
-rsp->buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+rsp_buffer_set_error(rsp, IPMI_CC_REQUEST_DATA_TRUNCATED);
  return;
  }

@@ -599,7 +604,7 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
  /* Set up the response, set the low bit of NETFN. */
  /* Note that max_rsp_len must be at least 3 */
  if (rsp.max_len < 3) {
-rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+rsp_buffer_set_error(, IPMI_CC_REQUEST_DATA_TRUNCATED);
  goto out;
  }

@@ -609,17 +614,17 @@ static void ipmi_sim_handle_command(IPMIBmc *b,

  /* If it's too short or it was truncated, return an error. */
  if (cmd_len < 2) {
-rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+rsp_buffer_set_error(, IPMI_CC_REQUEST_DATA_LENGTH_INVALID);
  goto out;
  }
  if (cmd_len > max_cmd_len) {
-rsp.buffer[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+rsp_buffer_set_error(, IPMI_CC_REQUEST_DATA_TRUNCATED);
  goto out;
  }

  if ((cmd[0] & 0x03) != 0) {
  /* Only have stuff on LUN 0 */
-rsp.buffer[2] = IPMI_CC_COMMAND_INVALID_FOR_LUN;
+rsp_buffer_set_error(, IPMI_CC_COMMAND_INVALID_FOR_LUN);
  goto out;
  }

@@ -629,12 +634,12 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
  if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
  (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
  (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
-rsp.buffer[2] = IPMI_CC_INVALID_CMD;
+rsp_buffer_set_error(, IPMI_CC_INVALID_CMD);
  goto out;
  }

  if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
-rsp.buffer[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+rsp_buffer_set_error(, IPMI_CC_REQUEST_DATA_LENGTH_INVALID);
  goto out;
  }

@@ -745,26 +750,26 @@ static void chassis_control(IPMIBmcSim *ibs,

  switch (cmd[2] & 0xf) {
  case 0: /* power down */
-rsp->buffer[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
+rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0));
  break;
  case 1: /* power up */
-rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERON_CHASSIS, 0);
+rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWERON_CHASSIS, 0));
  break;
  case 2: /* power cycle */
-rsp->buffer[2] = k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0);
+rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_POWERCYCLE_CHASSIS, 0));
  break;
  case 3: /* hard reset */
-rsp->buffer[2] = k->do_hw_op(s, IPMI_RESET_CHASSIS, 0);
+rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_RESET_CHASSIS, 0));
  break;
  case 4: /* pulse diagnostic interrupt */
-rsp->buffer[2] = k->do_hw_op(s, IPMI_PULSE_DIAG_IRQ, 0);
+rsp_buffer_set_error(rsp, k->do_hw_op(s, IPMI_PULSE_DIAG_IRQ, 0));
  break;
  case 5: /* soft shutdown via ACPI by overtemp emulation */
-rsp->buffer[2] = k->do_hw_op(s,
- IPMI_SHUTDOWN_VIA_ACPI_OVERTEMP, 0);
+rsp_buffer_set_error(rsp, k->do_hw_op(s,
+  IPMI_SHUTDOWN_VIA_ACPI_OVERTEMP, 0));
  break;
  default:
-rsp->buffer[2] = IPMI_CC_INVALID_DATA_FIELD;
+rsp_buffer_set_error(rsp, IPMI_CC_INVALID_DATA_FIELD);
  return;
  }
  }
@@ -903,7 +908,7 @@ static void read_evt_msg_buf(IPMIBmcSim *ibs,
  unsigned int i;

  if (!(ibs->msg_flags 

Re: [Qemu-devel] [PATCH v2 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro

[Corey Minyard]

On 03/02/2016 04:14 AM, Cédric Le Goater wrote:

Most IPMI command handlers in the BMC simulator start with a call to
the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
arguments expected by the command are indeed available. To achieve
this task, the macro implicitly uses local variables which is
misleading in the code.

This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
defining the minimal number of arguments expected by the command and
moves this check in the global command handler ipmi_sim_handle_command().


This is much better.  One style comment inline...

Acked-by: Corey Minyard 


Signed-off-by: Cédric Le Goater 
---
  hw/ipmi/ipmi_bmc_sim.c | 137 ++---
  1 file changed, 62 insertions(+), 75 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 51d234aa1bf2..30b9fb48ea2d 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -155,10 +155,15 @@ typedef struct IPMISensor {
  typedef struct IPMIBmcSim IPMIBmcSim;

  #define MAX_NETFNS 64
-typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
-   uint8_t *cmd, unsigned int cmd_len,
-   uint8_t *rsp, unsigned int *rsp_len,
-   unsigned int max_rsp_len);
+
+typedef struct IPMICmdHandler {
+void (*cmd_handler)(IPMIBmcSim *s,
+uint8_t *cmd, unsigned int cmd_len,
+uint8_t *rsp, unsigned int *rsp_len,
+unsigned int max_rsp_len);
+unsigned int cmd_len_min;
+} IPMICmdHandler;
+
  typedef struct IPMINetfn {
  unsigned int cmd_nums;
  const IPMICmdHandler *cmd_handlers;
@@ -269,13 +274,6 @@ struct IPMIBmcSim {
  rsp[(*rsp_len)++] = (b);   \
  } while (0)

-/* Verify that the received command is a certain length. */
-#define IPMI_CHECK_CMD_LEN(l) \
-if (cmd_len < l) { \
-rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;  \
-return; \
-}
-
  /* Check that the reservation in the command is valid. */
  #define IPMI_CHECK_RESERVATION(off, r) \
  do {   \
@@ -623,14 +621,19 @@ static void ipmi_sim_handle_command(IPMIBmc *b,

  /* Odd netfns are not valid, make sure the command is registered */
  if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
-(cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
-(!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
+(cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
+(!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
  rsp[2] = IPMI_CC_INVALID_CMD;
  goto out;


I'm not sure of the qemu style here, but the above change makes the code 
hard to read.


The qemu style really seems to be to not have big if statements like
this, so maybe this crasy check should be moved to a separate function?


  }

-ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, 
rsp_len,
-max_rsp_len);
+if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
+rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+goto out;
+}
+
+ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler(ibs, cmd, cmd_len,
+rsp, rsp_len, max_rsp_len);

   out:
  k->handle_rsp(s, msg_id, rsp, *rsp_len);
@@ -737,7 +740,6 @@ static void chassis_control(IPMIBmcSim *ibs,
  IPMIInterface *s = ibs->parent.intf;
  IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);

-IPMI_CHECK_CMD_LEN(3);
  switch (cmd[2] & 0xf) {
  case 0: /* power down */
  rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
@@ -838,7 +840,6 @@ static void set_acpi_power_state(IPMIBmcSim *ibs,
uint8_t *rsp, unsigned int *rsp_len,
unsigned int max_rsp_len)
  {
-IPMI_CHECK_CMD_LEN(4);
  ibs->acpi_power_state[0] = cmd[2];
  ibs->acpi_power_state[1] = cmd[3];
  }
@@ -869,7 +870,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
 uint8_t *rsp, unsigned int *rsp_len,
 unsigned int max_rsp_len)
  {
-IPMI_CHECK_CMD_LEN(3);
  set_global_enables(ibs, cmd[2]);
  }

@@ -889,7 +889,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
  IPMIInterface *s = ibs->parent.intf;
  IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);

-IPMI_CHECK_CMD_LEN(3);
  ibs->msg_flags &= ~cmd[2];
  k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
  }
@@ -976,15 +975,17 @@ static void send_msg(IPMIBmcSim *ibs,
  uint8_t *buf;
  uint8_t netfn, rqLun, rsLun, rqSeq;

-IPMI_CHECK_CMD_LEN(3);
-
  if (cmd[2] != 0) {
  /* We 

Re: [Qemu-devel] [PATCH v2][Outreachy Round 12]

[Markus Armbruster]

Your commit message isn't quite right, yet.  The first line is empty,
which leads to

Subject: [PATCH v2][Outreachy Round 12]

in e-mail, which isn't really useful.  It should be a line of the form

subsystem: Headline describing the patch briefly

In e-mail, this becomes something like

Subject: [PATCH v2] subsystem: Headline describing the patch briefly

The [PATCH v2] gets inserted by git-format-patch.

Finding the appropriate subsystem is unfortunately less than
straightforward.  You can use "git log --oneline FILENAME..." for ideas.
For your patch, I'd use linux-user.

Since this is a rather busy list, it's important to cc the right people
on patches.  Start with "scripts/get_maintainer PATCH-FILE".  The script
looks up the files you patch in MAINTAINERS for you.  In your case, its
output is

Riku Voipio  (maintainer:Overall)
qemu-devel@nongnu.org (open list:All patches CC here)

Send the patch to qemu-devel, cc: Riku.

All of this and much more is documented at
.  It's a learning curve,
but we'll gladly help you along.

Sarah Khan  writes:

> Replaced g_malloc() with g_new() as it would detect multiplication overflow 
> if nb_fields ever oversize.

Long line, please break it like you do in the next paragraph.

> There is no corresponding free(). thunk_register_struct() is called
> only at startup from the linux-user code in order to populate the
> struct_entries array; this data structure then remains live for
> the entire lifetime of the program and is automatically freed when
> QEMU exits.
>
> This replacement was suggested as part of the bite-sized tasks.
>
> Signed-off-by: Sarah Khan 
> ---
> Changes in v2 :Make commit message clearer
>  Replace g_malloc() with g_new()  
> ---
>  thunk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/thunk.c b/thunk.c
> index bddabae..6237702 100644
> --- a/thunk.c
> +++ b/thunk.c
> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const 
> argtype *types)
>  for(i = 0;i < 2; i++) {
>  offset = 0;
>  max_align = 1;
> -se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
> +se->field_offsets[i] = g_new(int,nb_fields);
>  type_ptr = se->field_types;
>  for(j = 0;j < nb_fields; j++) {
>  size = thunk_type_size(type_ptr, i);

Oh, this is an *incremental* patch: it applies on top of your v1.
That's awkward.  Your v2 should *replace* v1, not add to it.

Style nitpick: we want a space after comma.  Recommend to check your
patches with scripts/checkpatch.pl.  Output for this one is:

ERROR: space required after that ',' (ctx:VxV)
#127: FILE: thunk.c:91:
+se->field_offsets[i] = g_new(int,nb_fields);
 ^

total: 1 errors, 0 warnings, 8 lines checked

/home/armbru/Mail/mail/redhat/xlst/qemu-devel/385421 has style problems, 
please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Welcome to qemu-devel, Sarah!

[Qemu-devel] Hi good afternoon, some small support

[Sanchez, Juan Carlos (GE Aviation)]

Hi,
I am trying to execute an image created by ARM in QEMU using windows 
OS, could you give me guide to example the command line? 

The command line in Unix work well, but I don't understood all the options. 
This the reason that I looking for some help

/home/nhcu02t/yocto_i686/prebuild/sysroots/i686-pokysdk-linux/usr/bin/qemu-system-arm
 -kernel zImage-qemuarm.bin -net nic,model=virtio -net 
tap,vlan=0,ifname=tap0,script=no,downscript=no -M versatilepb -drive 
file=/home/nhcu02t/yocto_org_built/images/qemuarm/core-image-minimal-qemuarm.ext4,if=virtio,format=raw
 -no-reboot -show-cursor -usb -usbdevice wacom-tablet -no-reboot -m 128 -serial 
mon:vc -serial null --append "root=/dev/vda rw console=ttyAMA0,115200 
console=tty ip=192.168.7.2::192.168.7.1:255.255.255.0 mem=128M highres=off 
rootfstype=ext4 "


I accept any help that you provide me.

Juan Carlos Sanchez

[Qemu-devel] [PULL v2 06/12] qapi: Update docs to match recent generator changes

[Markus Armbruster]

From: Eric Blake 

Several commits have been changing the generator, but not updating
the docs to match:
- The implicit tag member is named "type", not "kind".  Screwed up in
commit 39a1815.
- Commit 9f08c8ec made list types lazy, and thereby dropped
UserDefOneList if nothing explicitly uses the list type.
- Commit 51e72bc1 switched the parameter order with 'name' occurring
earlier.
- Commit e65d89bf changed the layout of UserDefOneList.
- Prefer the term 'member' over 'field'.
- We now expose visit_type_FOO_members() for objects.
- etc.

Rework the examples to show slightly more output (we don't want to
show too much; that's what the testsuite is for), and regenerate the
output to match all recent changes.  Also, rearrange output to show
.h files before .c (understanding the interface first often makes
the implementation easier to follow).

Reported-by: Marc-André Lureau 
Signed-off-by: Eric Blake 
Signed-off-by: Markus Armbruster 
Message-Id: <1457021813-10704-5-git-send-email-ebl...@redhat.com>
---
 docs/qapi-code-gen.txt | 364 ++---
 docs/qmp-spec.txt  |   4 +-
 2 files changed, 192 insertions(+), 176 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 999f3b9..e0b2ef1 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -1,7 +1,7 @@
 = How to use the QAPI code generator =
 
 Copyright IBM Corp. 2011
-Copyright (C) 2012-2015 Red Hat, Inc.
+Copyright (C) 2012-2016 Red Hat, Inc.
 
 This work is licensed under the terms of the GNU GPL, version 2 or
 later. See the COPYING file in the top-level directory.
@@ -52,7 +52,7 @@ schema.  The documentation is delimited between two lines of 
##, then
 the first line names the expression, an optional overview is provided,
 then individual documentation about each member of 'data' is provided,
 and finally, a 'Since: x.y.z' tag lists the release that introduced
-the expression.  Optional fields are tagged with the phrase
+the expression.  Optional members are tagged with the phrase
 '#optional', often with their default value; and extensions added
 after the expression was first released are also given a '(since
 x.y.z)' comment.  For example:
@@ -108,15 +108,15 @@ user-defined type names, while built-in types are 
lowercase. Type
 definitions should not end in 'Kind', as this namespace is used for
 creating implicit C enums for visiting union types, or in 'List', as
 this namespace is used for creating array types.  Command names,
-and field names within a type, should be all lower case with words
+and member names within a type, should be all lower case with words
 separated by a hyphen.  However, some existing older commands and
 complex types use underscore; when extending such expressions,
 consistency is preferred over blindly avoiding underscore.  Event
-names should be ALL_CAPS with words separated by underscore.  Field
+names should be ALL_CAPS with words separated by underscore.  Member
 names cannot start with 'has-' or 'has_', as this is reserved for
-tracking optional fields.
+tracking optional members.
 
-Any name (command, event, type, field, or enum value) beginning with
+Any name (command, event, type, member, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
 incompatibly in a future release.  All names must begin with a letter,
 and contain only ASCII letters, digits, dash, and underscore.  There
@@ -127,7 +127,7 @@ the vendor), even if the rest of the name uses dash 
(example:
 __com.redhat_drive-mirror).  Names beginning with 'q_' are reserved
 for the generator: QMP names that resemble C keywords or other
 problematic strings will be munged in C to use this prefix.  For
-example, a field named "default" in qapi becomes "q_default" in the
+example, a member named "default" in qapi becomes "q_default" in the
 generated C code.
 
 In the rest of this document, usage lines are given for each
@@ -217,17 +217,18 @@ and must continue to work).
 
 On output structures (only mentioned in the 'returns' side of a command),
 changing from mandatory to optional is in general unsafe (older clients may be
-expecting the field, and could crash if it is missing), although it can be done
-if the only way that the optional argument will be omitted is when it is
-triggered by the presence of a new input flag to the command that older clients
-don't know to send.  Changing from optional to mandatory is safe.
+expecting the member, and could crash if it is missing), although it
+can be done if the only way that the optional argument will be omitted
+is when it is triggered by the presence of a new input flag to the
+command that older clients don't know to send.  Changing from optional
+to mandatory is safe.
 
 A structure that is used in both input and output of various commands
 must consider the backwards compatibility constraints of both 

[Qemu-devel] [PULL v2 12/12] qapi: Drop useless 'data' member of unions

[Markus Armbruster]

From: Eric Blake 

We started moving away from the use of the 'void *data' member
in the C union corresponding to a QAPI union back in commit
544a373; recent commits have gotten rid of other uses.  Now
that it is completely unused, we can remove the member itself
as well as the FIXME comment.  Update the testsuite to drop the
negative test union-clash-data.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrange 
Message-Id: <1457021813-10704-11-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 scripts/qapi-types.py   | 9 -
 tests/Makefile  | 1 -
 tests/qapi-schema/union-clash-data.err  | 0
 tests/qapi-schema/union-clash-data.exit | 1 -
 tests/qapi-schema/union-clash-data.json | 7 ---
 tests/qapi-schema/union-clash-data.out  | 9 -
 6 files changed, 27 deletions(-)
 delete mode 100644 tests/qapi-schema/union-clash-data.err
 delete mode 100644 tests/qapi-schema/union-clash-data.exit
 delete mode 100644 tests/qapi-schema/union-clash-data.json
 delete mode 100644 tests/qapi-schema/union-clash-data.out

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 19d1fff..0306a88 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -116,17 +116,8 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
%(c_name)s *obj)
 
 
 def gen_variants(variants):
-# FIXME: What purpose does data serve, besides preventing a union that
-# has a branch named 'data'? We use it in qapi-visit.py to decide
-# whether to bypass the switch statement if visiting the discriminator
-# failed; but since we 0-initialize structs, and cannot tell what
-# branch of the union is in use if the discriminator is invalid, there
-# should not be any data leaks even without a data pointer.  Or, if
-# 'data' is merely added to guarantee we don't have an empty union,
-# shouldn't we enforce that at .json parse time?
 ret = mcgen('''
 union { /* union tag is @%(c_name)s */
-void *data;
 ''',
 c_name=c_name(variants.tag_member.name))
 
diff --git a/tests/Makefile b/tests/Makefile
index 04e34b5..cd4bbd4 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -358,7 +358,6 @@ qapi-schema += unicode-str.json
 qapi-schema += union-base-no-discriminator.json
 qapi-schema += union-branch-case.json
 qapi-schema += union-clash-branches.json
-qapi-schema += union-clash-data.json
 qapi-schema += union-empty.json
 qapi-schema += union-invalid-base.json
 qapi-schema += union-optional-branch.json
diff --git a/tests/qapi-schema/union-clash-data.err 
b/tests/qapi-schema/union-clash-data.err
deleted file mode 100644
index e69de29..000
diff --git a/tests/qapi-schema/union-clash-data.exit 
b/tests/qapi-schema/union-clash-data.exit
deleted file mode 100644
index 573541a..000
--- a/tests/qapi-schema/union-clash-data.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/union-clash-data.json 
b/tests/qapi-schema/union-clash-data.json
deleted file mode 100644
index 7308e69..000
--- a/tests/qapi-schema/union-clash-data.json
+++ /dev/null
@@ -1,7 +0,0 @@
-# Union branch 'data'
-# FIXME: this parses, but then fails to compile due to a duplicate 'data'
-# (one from the branch name, another as a filler to avoid an empty union).
-# we should either detect the collision at parse time, or change the
-# generated struct to allow this to compile.
-{ 'union': 'TestUnion',
-  'data': { 'data': 'int' } }
diff --git a/tests/qapi-schema/union-clash-data.out 
b/tests/qapi-schema/union-clash-data.out
deleted file mode 100644
index f5752f4..000
--- a/tests/qapi-schema/union-clash-data.out
+++ /dev/null
@@ -1,9 +0,0 @@
-object :empty
-object :obj-int-wrapper
-member data: int optional=False
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 
'qbool']
-prefix QTYPE
-object TestUnion
-member type: TestUnionKind optional=False
-case data: :obj-int-wrapper
-enum TestUnionKind ['data']
-- 
2.4.3

[Qemu-devel] [PULL v2 09/12] ui: Shorten references into InputEvent

[Markus Armbruster]

From: Eric Blake 

An upcoming patch will alter how simple unions, like InputEvent, are
laid out, which will impact all lines of the form 'evt->u.XXX'
(expanding it to the longer 'evt->u.XXX.data').  For better
legibility in that patch, and less need for line wrapping, it's better
to use a temporary variable to reduce the effect of a layout change to
just the variable initializations, rather than every reference within
an InputEvent.

There was one instance in hid.c:hid_pointer_event() where the code
was referring to evt->u.rel inside the case label where evt->u.abs
is the correct name; thankfully, both members of the union have the
same type, so it happened to work, but it is now cleaner.

Signed-off-by: Eric Blake 
Message-Id: <1457021813-10704-8-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 hw/char/escc.c  | 12 +-
 hw/input/hid.c  | 36 +-
 hw/input/ps2.c  | 27 ++-
 hw/input/virtio-input-hid.c | 33 ---
 replay/replay-input.c   | 31 --
 ui/input-legacy.c   | 25 -
 ui/input.c  | 54 ++---
 7 files changed, 129 insertions(+), 89 deletions(-)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 98a1c21..c7a24ac 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -842,14 +842,16 @@ static void sunkbd_handle_event(DeviceState *dev, 
QemuConsole *src,
 {
 ChannelState *s = (ChannelState *)dev;
 int qcode, keycode;
+InputKeyEvent *key;
 
 assert(evt->type == INPUT_EVENT_KIND_KEY);
-qcode = qemu_input_key_value_to_qcode(evt->u.key->key);
+key = evt->u.key;
+qcode = qemu_input_key_value_to_qcode(key->key);
 trace_escc_sunkbd_event_in(qcode, QKeyCode_lookup[qcode],
-   evt->u.key->down);
+   key->down);
 
 if (qcode == Q_KEY_CODE_CAPS_LOCK) {
-if (evt->u.key->down) {
+if (key->down) {
 s->caps_lock_mode ^= 1;
 if (s->caps_lock_mode == 2) {
 return; /* Drop second press */
@@ -863,7 +865,7 @@ static void sunkbd_handle_event(DeviceState *dev, 
QemuConsole *src,
 }
 
 if (qcode == Q_KEY_CODE_NUM_LOCK) {
-if (evt->u.key->down) {
+if (key->down) {
 s->num_lock_mode ^= 1;
 if (s->num_lock_mode == 2) {
 return; /* Drop second press */
@@ -877,7 +879,7 @@ static void sunkbd_handle_event(DeviceState *dev, 
QemuConsole *src,
 }
 
 keycode = qcode_to_keycode[qcode];
-if (!evt->u.key->down) {
+if (!key->down) {
 keycode |= 0x80;
 }
 trace_escc_sunkbd_event_out(keycode);
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 81a85fb..41a9387 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -116,37 +116,42 @@ static void hid_pointer_event(DeviceState *dev, 
QemuConsole *src,
 };
 HIDState *hs = (HIDState *)dev;
 HIDPointerEvent *e;
+InputMoveEvent *move;
+InputBtnEvent *btn;
 
 assert(hs->n < QUEUE_LENGTH);
 e = >ptr.queue[(hs->head + hs->n) & QUEUE_MASK];
 
 switch (evt->type) {
 case INPUT_EVENT_KIND_REL:
-if (evt->u.rel->axis == INPUT_AXIS_X) {
-e->xdx += evt->u.rel->value;
-} else if (evt->u.rel->axis == INPUT_AXIS_Y) {
-e->ydy += evt->u.rel->value;
+move = evt->u.rel;
+if (move->axis == INPUT_AXIS_X) {
+e->xdx += move->value;
+} else if (move->axis == INPUT_AXIS_Y) {
+e->ydy += move->value;
 }
 break;
 
 case INPUT_EVENT_KIND_ABS:
-if (evt->u.rel->axis == INPUT_AXIS_X) {
-e->xdx = evt->u.rel->value;
-} else if (evt->u.rel->axis == INPUT_AXIS_Y) {
-e->ydy = evt->u.rel->value;
+move = evt->u.abs;
+if (move->axis == INPUT_AXIS_X) {
+e->xdx = move->value;
+} else if (move->axis == INPUT_AXIS_Y) {
+e->ydy = move->value;
 }
 break;
 
 case INPUT_EVENT_KIND_BTN:
-if (evt->u.btn->down) {
-e->buttons_state |= bmap[evt->u.btn->button];
-if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
+btn = evt->u.btn;
+if (btn->down) {
+e->buttons_state |= bmap[btn->button];
+if (btn->button == INPUT_BUTTON_WHEEL_UP) {
 e->dz--;
-} else if (evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+} else if (btn->button == INPUT_BUTTON_WHEEL_DOWN) {
 e->dz++;
 }
 } else {
-e->buttons_state &= ~bmap[evt->u.btn->button];
+e->buttons_state &= ~bmap[btn->button];
 }
 break;
 
@@ -223,9 +228,10 @@ static void hid_keyboard_event(DeviceState *dev, 
QemuConsole *src,
 HIDState *hs = 

[Qemu-devel] [PULL v2 01/12] qmp-shell: fix pretty printing of JSON responses

[Markus Armbruster]

From: "Daniel P. Berrange" 

Pretty printing of JSON responses is important to be able to understand
large responses from query commands in particular. Unfortunately this
was broken during the addition of the verbose flag in

  commit 1ceca07e48ead0dd2e41576c81d40e6a91cafefd
  Author: John Snow 
  Date:   Wed Apr 29 15:14:04 2015 -0400

scripts: qmp-shell: Add verbose flag

This is because that change turned the python data structure into a
formatted JSON string before the pretty print was given it. So we're
just pretty printing a string, which is a no-op.

The original pretty printer would output python objects.

(QEMU) query-chardev
{   u'return': [   {   u'filename': u'vc',
   u'frontend-open': False,
   u'label': u'parallel0'},
   {   u'filename': u'vc',
   u'frontend-open': True,
   u'label': u'serial0'},
   {   u'filename': u'unix:/tmp/qemp,server',
   u'frontend-open': True,
   u'label': u'compat_monitor0'}]}

This fixes the problem by switching to outputting pretty formatted JSON
text instead. This has the added benefit that the pretty printed output
is now valid JSON text. Due to the way the verbose flag was handled, the
pretty printing now applies to the command sent, as well as its response:

(QEMU) query-chardev
{
"execute": "query-chardev",
"arguments": {}
}
{
"return": [
{
"frontend-open": false,
"label": "parallel0",
"filename": "vc"
},
{
"frontend-open": true,
"label": "serial0",
"filename": "vc"
},
{
"frontend-open": true,
"label": "compat_monitor0",
"filename": "unix:/tmp/qmp,server"
}
]
}

Signed-off-by: Daniel P. Berrange 
Message-Id: <1456224706-1591-1-git-send-email-berra...@redhat.com>
Tested-by: Kashyap Chamarthy 
Reviewed-by: John Snow 
[Bonus fix: multiple -p now work]
Signed-off-by: Markus Armbruster 
---
 scripts/qmp/qmp-shell | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 7a402ed..0373b24 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -70,7 +70,6 @@ import json
 import ast
 import readline
 import sys
-import pprint
 
 class QMPCompleter(list):
 def complete(self, text, state):
@@ -103,11 +102,11 @@ class FuzzyJSON(ast.NodeTransformer):
 # TODO: QMPShell's interface is a bit ugly (eg. _fill_completion() and
 #   _execute_cmd()). Let's design a better one.
 class QMPShell(qmp.QEMUMonitorProtocol):
-def __init__(self, address, pp=None):
+def __init__(self, address, pretty=False):
 qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address))
 self._greeting = None
 self._completer = None
-self._pp = pp
+self._pretty = pretty
 self._transmode = False
 self._actions = list()
 
@@ -231,11 +230,11 @@ class QMPShell(qmp.QEMUMonitorProtocol):
 return qmpcmd
 
 def _print(self, qmp):
-jsobj = json.dumps(qmp)
-if self._pp is not None:
-self._pp.pprint(jsobj)
-else:
-print str(jsobj)
+indent = None
+if self._pretty:
+indent = 4
+jsobj = json.dumps(qmp, indent=indent)
+print str(jsobj)
 
 def _execute_cmd(self, cmdline):
 try:
@@ -377,7 +376,7 @@ def main():
 addr = ''
 qemu = None
 hmp = False
-pp = None
+pretty = False
 verbose = False
 
 try:
@@ -387,9 +386,7 @@ def main():
 fail_cmdline(arg)
 hmp = True
 elif arg == "-p":
-if pp is not None:
-fail_cmdline(arg)
-pp = pprint.PrettyPrinter(indent=4)
+pretty = True
 elif arg == "-v":
 verbose = True
 else:
@@ -398,7 +395,7 @@ def main():
 if hmp:
 qemu = HMPShell(arg)
 else:
-qemu = QMPShell(arg, pp)
+qemu = QMPShell(arg, pretty)
 addr = arg
 
 if qemu is None:
-- 
2.4.3

[Qemu-devel] [PULL v2 11/12] chardev: Drop useless ChardevDummy type

[Markus Armbruster]

From: Eric Blake 

Commit d0d7708b made ChardevDummy be an empty wrapper type around
ChardevCommon.  But there is no technical reason for this indirection,
so simplify the code by directly using the base type.

Also change the fallback assignment to assign u.null rather than
u.data, since a future patch will remove the data member of the C
struct generated for QAPI unions.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrange 
Message-Id: <1457106160-23614-1-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 backends/baum.c|  2 +-
 backends/msmouse.c |  2 +-
 qapi-schema.json   | 15 ++-
 qemu-char.c|  8 
 4 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/backends/baum.c b/backends/baum.c
index 374562a..c11320e 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -567,7 +567,7 @@ static CharDriverState *chr_baum_init(const char *id,
   ChardevReturn *ret,
   Error **errp)
 {
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.braille);
+ChardevCommon *common = backend->u.braille;
 BaumDriverState *baum;
 CharDriverState *chr;
 brlapi_handle_t *handle;
diff --git a/backends/msmouse.c b/backends/msmouse.c
index 9a82efd..5e1833c 100644
--- a/backends/msmouse.c
+++ b/backends/msmouse.c
@@ -68,7 +68,7 @@ static CharDriverState *qemu_chr_open_msmouse(const char *id,
   ChardevReturn *ret,
   Error **errp)
 {
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.msmouse);
+ChardevCommon *common = backend->u.msmouse;
 CharDriverState *chr;
 
 chr = qemu_chr_alloc(common, errp);
diff --git a/qapi-schema.json b/qapi-schema.json
index 42fd61b..362c9d8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3323,23 +3323,20 @@
 #
 # Since: 1.4 (testdev since 2.2)
 ##
-{ 'struct': 'ChardevDummy', 'data': { },
-  'base': 'ChardevCommon' }
-
 { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
'serial' : 'ChardevHostdev',
'parallel': 'ChardevHostdev',
'pipe'   : 'ChardevHostdev',
'socket' : 'ChardevSocket',
'udp': 'ChardevUdp',
-   'pty': 'ChardevDummy',
-   'null'   : 'ChardevDummy',
+   'pty': 'ChardevCommon',
+   'null'   : 'ChardevCommon',
'mux': 'ChardevMux',
-   'msmouse': 'ChardevDummy',
-   'braille': 'ChardevDummy',
-   'testdev': 'ChardevDummy',
+   'msmouse': 'ChardevCommon',
+   'braille': 'ChardevCommon',
+   'testdev': 'ChardevCommon',
'stdio'  : 'ChardevStdio',
-   'console': 'ChardevDummy',
+   'console': 'ChardevCommon',
'spicevmc' : 'ChardevSpiceChannel',
'spiceport' : 'ChardevSpicePort',
'vc' : 'ChardevVC',
diff --git a/qemu-char.c b/qemu-char.c
index af31102..e0147f3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -420,7 +420,7 @@ static CharDriverState *qemu_chr_open_null(const char *id,
Error **errp)
 {
 CharDriverState *chr;
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.null);
+ChardevCommon *common = backend->u.null;
 
 chr = qemu_chr_alloc(common, errp);
 if (!chr) {
@@ -1366,7 +1366,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
 PtyCharDriver *s;
 int master_fd, slave_fd;
 char pty_name[PATH_MAX];
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.pty);
+ChardevCommon *common = backend->u.pty;
 
 master_fd = qemu_openpty_raw(_fd, pty_name);
 if (master_fd < 0) {
@@ -2183,7 +2183,7 @@ static CharDriverState *qemu_chr_open_win_con(const char 
*id,
   ChardevReturn *ret,
   Error **errp)
 {
-ChardevCommon *common = qapi_ChardevDummy_base(backend->u.console);
+ChardevCommon *common = backend->u.console;
 return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE),
   common, errp);
 }
@@ -3817,7 +3817,7 @@ CharDriverState 

[Qemu-devel] [PULL v2 10/12] qapi: Avoid use of 'data' member of QAPI unions

[Markus Armbruster]

From: Eric Blake 

QAPI code generators currently create a 'void *data' member as
part of the anonymous union embedded in the C struct corresponding
to a QAPI union.  However, directly assigning to this member of
the union feels a bit fishy, when we can assign to another member
of the struct instead.

Signed-off-by: Eric Blake 
Reviewed-by: Daniel P. Berrange 
Message-Id: <1457021813-10704-9-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 blockdev.c | 31 +--
 ui/input.c |  2 +-
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d4bc435..0f20c65 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1202,15 +1202,11 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
 }
 }
 
-static void blockdev_do_action(TransactionActionKind type, void *data,
-   Error **errp)
+static void blockdev_do_action(TransactionAction *action, Error **errp)
 {
-TransactionAction action;
 TransactionActionList list;
 
-action.type = type;
-action.u.data = data;
-list.value = 
+list.value = action;
 list.next = NULL;
 qmp_transaction(, false, NULL, errp);
 }
@@ -1236,8 +1232,11 @@ void qmp_blockdev_snapshot_sync(bool has_device, const 
char *device,
 .has_mode = has_mode,
 .mode = mode,
 };
-blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
-   , errp);
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
+.u.blockdev_snapshot_sync = ,
+};
+blockdev_do_action(, errp);
 }
 
 void qmp_blockdev_snapshot(const char *node, const char *overlay,
@@ -1247,9 +1246,11 @@ void qmp_blockdev_snapshot(const char *node, const char 
*overlay,
 .node = (char *) node,
 .overlay = (char *) overlay
 };
-
-blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
-   _data, errp);
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
+.u.blockdev_snapshot = _data,
+};
+blockdev_do_action(, errp);
 }
 
 void qmp_blockdev_snapshot_internal_sync(const char *device,
@@ -1260,9 +1261,11 @@ void qmp_blockdev_snapshot_internal_sync(const char 
*device,
 .device = (char *) device,
 .name = (char *) name
 };
-
-blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
-   , errp);
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+.u.blockdev_snapshot_internal_sync = ,
+};
+blockdev_do_action(, errp);
 }
 
 SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
diff --git a/ui/input.c b/ui/input.c
index 13ee117..b035f86 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -470,7 +470,7 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
 InputMoveEvent *move = g_new0(InputMoveEvent, 1);
 
 evt->type = kind;
-evt->u.data = move;
+evt->u.rel = move; /* evt->u.rel is the same as evt->u.abs */
 move->axis = axis;
 move->value = value;
 return evt;
-- 
2.4.3

[Qemu-devel] [PULL v2 05/12] qapi-visit: Expose visit_type_FOO_members()

[Markus Armbruster]

From: Eric Blake 

Dan Berrange reported a case where he needs to work with a
QCryptoBlockOptions union type using the OptsVisitor, but only
visit one of the branches of that type (the discriminator is not
visited directly, but learned externally).  When things were
boxed, it was easy: just visit the variant directly, which took
care of both allocating the variant and visiting its members, then
store that pointer in the union type.  But now that things are
unboxed, we need a way to visit the members without allocation,
done by exposing visit_type_FOO_members() to the user.

Before the patch, we had quite a bit of code associated with
object_members_seen to make sure that a declaration of the helper
was in scope before any use of the function.  But now that the
helper is public and declared in the header, the .c file no
longer needs to worry about topological sorting (the helper is
always in scope), which leads to some nice cleanups.

Signed-off-by: Eric Blake 
Message-Id: <1457021813-10704-4-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 scripts/qapi-visit.py | 33 +++--
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 1e52f76..a712e9a 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
 from qapi import *
 import re
 
-# visit_type_FOO_members() is always emitted; track if a forward declaration
-# or implementation has already been output.
-object_members_seen = set()
-
 
 def gen_visit_decl(name, scalar=False):
 c_type = c_name(name) + ' *'
@@ -30,37 +26,23 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_type)sobj, Error **
  c_name=c_name(name), c_type=c_type)
 
 
-def gen_visit_members_decl(typ):
-if typ.name in object_members_seen:
-return ''
-object_members_seen.add(typ.name)
+def gen_visit_members_decl(name):
 return mcgen('''
 
-static void visit_type_%(c_type)s_members(Visitor *v, %(c_type)s *obj, Error 
**errp);
+void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
 ''',
- c_type=typ.c_name())
+ c_name=c_name(name))
 
 
 def gen_visit_object_members(name, base, members, variants):
-ret = ''
+ret = mcgen('''
 
-if base:
-ret += gen_visit_members_decl(base)
-if variants:
-for var in variants.variants:
-# Ugly special case for simple union TODO get rid of it
-if not var.simple_union_type():
-ret += gen_visit_members_decl(var.type)
-
-object_members_seen.add(name)
-ret += mcgen('''
-
-static void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error 
**errp)
+void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
 {
 Error *err = NULL;
 
 ''',
- c_name=c_name(name))
+c_name=c_name(name))
 
 if base:
 ret += mcgen('''
@@ -173,8 +155,6 @@ def gen_visit_alternate(name, variants):
 for var in variants.variants:
 if var.type.alternate_qtype() == 'QTYPE_QINT':
 promote_int = 'false'
-if isinstance(var.type, QAPISchemaObjectType):
-ret += gen_visit_members_decl(var.type)
 
 ret += mcgen('''
 
@@ -316,6 +296,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
 self.defn += defn
 
 def visit_object_type(self, name, info, base, members, variants):
+self.decl += gen_visit_members_decl(name)
 self.decl += gen_visit_decl(name)
 self.defn += gen_visit_object(name, base, members, variants)
 
-- 
2.4.3

[Qemu-devel] [PULL v2 04/12] qapi: Rename 'fields' to 'members' in generated C code

[Markus Armbruster]

From: Eric Blake 

C types and JSON objects don't have fields, but members.  We
shouldn't gratuitously invent terminology.  This patch is a
strict renaming of static genarated functions, plus the naming
of the dummy filler member for empty structs, before the next
patch exposes some of that naming to the rest of the code base.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 
Message-Id: <1457021813-10704-3-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 scripts/qapi-types.py |  2 +-
 scripts/qapi-visit.py | 12 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 8858d29..19d1fff 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -92,7 +92,7 @@ struct %(c_name)s {
 # struct is size 1).
 if not (base and base.members) and not members and not variants:
 ret += mcgen('''
-char qapi_dummy_field_for_empty_struct;
+char qapi_dummy_for_empty_struct;
 ''')
 
 ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b21d3ef..1e52f76 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -36,7 +36,7 @@ def gen_visit_members_decl(typ):
 object_members_seen.add(typ.name)
 return mcgen('''
 
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error 
**errp);
+static void visit_type_%(c_type)s_members(Visitor *v, %(c_type)s *obj, Error 
**errp);
 ''',
  c_type=typ.c_name())
 
@@ -55,7 +55,7 @@ def gen_visit_object_members(name, base, members, variants):
 object_members_seen.add(name)
 ret += mcgen('''
 
-static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error 
**errp)
+static void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error 
**errp)
 {
 Error *err = NULL;
 
@@ -64,7 +64,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
%(c_name)s *obj, Error **er
 
 if base:
 ret += mcgen('''
-visit_type_%(c_type)s_fields(v, (%(c_type)s *)obj, );
+visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, );
 ''',
  c_type=base.c_name())
 ret += gen_err_check()
@@ -94,7 +94,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
%(c_name)s *obj, Error **er
  c_name=c_name(var.name))
 else:
 ret += mcgen('''
-visit_type_%(c_type)s_fields(v, >u.%(c_name)s, );
+visit_type_%(c_type)s_members(v, >u.%(c_name)s, );
 ''',
  c_type=var.type.c_name(),
  c_name=c_name(var.name))
@@ -202,7 +202,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 if (err) {
 break;
 }
-visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, );
+visit_type_%(c_type)s_members(v, &(*obj)->u.%(c_name)s, );
 error_propagate(errp, err);
 err = NULL;
 visit_end_struct(v, );
@@ -254,7 +254,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 if (!*obj) {
 goto out_obj;
 }
-visit_type_%(c_name)s_fields(v, *obj, );
+visit_type_%(c_name)s_members(v, *obj, );
 error_propagate(errp, err);
 err = NULL;
 out_obj:
-- 
2.4.3

[Qemu-devel] [PULL v2 08/12] util: Shorten references into SocketAddress

[Markus Armbruster]

From: Eric Blake 

An upcoming patch will alter how simple unions, like SocketAddress,
are laid out, which will impact all lines of the form 'addr->u.XXX'
(expanding it to the longer 'addr->u.XXX.data').  For better
legibility in that patch, and less need for line wrapping, it's better
to use a temporary variable to reduce the effect of a layout change to
just the variable initializations, rather than every reference within
a SocketAddress.  Also, take advantage of some C99 initialization where
it makes sense (simplifying g_new0() to g_new()).

Signed-off-by: Eric Blake 
Message-Id: <1457021813-10704-7-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 block/nbd.c| 14 ++--
 qemu-char.c| 49 --
 qemu-nbd.c |  9 
 tests/test-io-channel-socket.c | 34 ++---
 ui/vnc.c   | 39 +
 util/qemu-sockets.c| 11 +-
 6 files changed, 88 insertions(+), 68 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index db57b49..9f333c9 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -204,18 +204,20 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options, char **export,
 saddr = g_new0(SocketAddress, 1);
 
 if (qdict_haskey(options, "path")) {
+UnixSocketAddress *q_unix;
 saddr->type = SOCKET_ADDRESS_KIND_UNIX;
-saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
-saddr->u.q_unix->path = g_strdup(qdict_get_str(options, "path"));
+q_unix = saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+q_unix->path = g_strdup(qdict_get_str(options, "path"));
 qdict_del(options, "path");
 } else {
+InetSocketAddress *inet;
 saddr->type = SOCKET_ADDRESS_KIND_INET;
-saddr->u.inet = g_new0(InetSocketAddress, 1);
-saddr->u.inet->host = g_strdup(qdict_get_str(options, "host"));
+inet = saddr->u.inet = g_new0(InetSocketAddress, 1);
+inet->host = g_strdup(qdict_get_str(options, "host"));
 if (!qdict_get_try_str(options, "port")) {
-saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
 } else {
-saddr->u.inet->port = g_strdup(qdict_get_str(options, "port"));
+inet->port = g_strdup(qdict_get_str(options, "port"));
 }
 qdict_del(options, "host");
 qdict_del(options, "port");
diff --git a/qemu-char.c b/qemu-char.c
index 5ea1d34..af31102 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3659,20 +3659,23 @@ static void qemu_chr_parse_socket(QemuOpts *opts, 
ChardevBackend *backend,
 
 addr = g_new0(SocketAddress, 1);
 if (path) {
+UnixSocketAddress *q_unix;
 addr->type = SOCKET_ADDRESS_KIND_UNIX;
-addr->u.q_unix = g_new0(UnixSocketAddress, 1);
-addr->u.q_unix->path = g_strdup(path);
+q_unix = addr->u.q_unix = g_new0(UnixSocketAddress, 1);
+q_unix->path = g_strdup(path);
 } else {
 addr->type = SOCKET_ADDRESS_KIND_INET;
-addr->u.inet = g_new0(InetSocketAddress, 1);
-addr->u.inet->host = g_strdup(host);
-addr->u.inet->port = g_strdup(port);
-addr->u.inet->has_to = qemu_opt_get(opts, "to");
-addr->u.inet->to = qemu_opt_get_number(opts, "to", 0);
-addr->u.inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
-addr->u.inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
-addr->u.inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
-addr->u.inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+addr->u.inet = g_new(InetSocketAddress, 1);
+*addr->u.inet = (InetSocketAddress) {
+.host = g_strdup(host),
+.port = g_strdup(port),
+.has_to = qemu_opt_get(opts, "to"),
+.to = qemu_opt_get_number(opts, "to", 0),
+.has_ipv4 = qemu_opt_get(opts, "ipv4"),
+.ipv4 = qemu_opt_get_bool(opts, "ipv4", 0),
+.has_ipv6 = qemu_opt_get(opts, "ipv6"),
+.ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
+};
 }
 sock->addr = addr;
 }
@@ -3711,22 +3714,26 @@ static void qemu_chr_parse_udp(QemuOpts *opts, 
ChardevBackend *backend,
 
 addr = g_new0(SocketAddress, 1);
 addr->type = SOCKET_ADDRESS_KIND_INET;
-addr->u.inet = g_new0(InetSocketAddress, 1);
-addr->u.inet->host = g_strdup(host);
-addr->u.inet->port = g_strdup(port);
-addr->u.inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
-addr->u.inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
-addr->u.inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
-addr->u.inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+addr->u.inet = g_new(InetSocketAddress, 1);
+*addr->u.inet = (InetSocketAddress) {
+.host = g_strdup(host),
+   

[Qemu-devel] [PULL v2 03/12] qapi: Rename 'fields' to 'members' in generator

[Markus Armbruster]

From: Eric Blake 

C types and JSON objects don't have fields, but members.  We
shouldn't gratuitously invent terminology.  This patch is a
strict renaming of generator code internals (including testsuite
comments), before later patches rename C interfaces.

No change to generated code with this patch.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 
Message-Id: <1457021813-10704-2-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 scripts/qapi-commands.py|  4 ++--
 scripts/qapi-event.py   |  4 ++--
 scripts/qapi-types.py   |  8 
 scripts/qapi-visit.py   | 28 ++--
 scripts/qapi.py | 20 ++--
 tests/qapi-schema/qapi-schema-test.json |  2 +-
 6 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f831621..f44e01f 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -111,7 +111,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
 v = qmp_input_get_visitor(qiv);
 ''')
 
-ret += gen_visit_fields(arg_type.members, skiperr=dealloc)
+ret += gen_visit_members(arg_type.members, skiperr=dealloc)
 
 if dealloc:
 ret += mcgen('''
@@ -175,7 +175,7 @@ def gen_marshal(name, arg_type, ret_type):
 ret += gen_marshal_input_visit(arg_type)
 ret += gen_call(name, arg_type, ret_type)
 
-# 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
+# 'goto out' produced by gen_marshal_input_visit->gen_visit_members()
 # for each arg_type member, and by gen_call() for ret_type
 if (arg_type and arg_type.members) or ret_type:
 ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 544ae12..fb579dd 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -67,8 +67,8 @@ def gen_event_send(name, arg_type):
 ''',
  name=name)
 ret += gen_err_check()
-ret += gen_visit_fields(arg_type.members, need_cast=True,
-label='out_obj')
+ret += gen_visit_members(arg_type.members, need_cast=True,
+ label='out_obj')
 ret += mcgen('''
 out_obj:
 visit_end_struct(v, err ? NULL : );
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index eac90d2..8858d29 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -38,7 +38,7 @@ struct %(c_name)s {
  c_name=c_name(name), c_type=element_type.c_type())
 
 
-def gen_struct_fields(members):
+def gen_struct_members(members):
 ret = ''
 for memb in members:
 if memb.optional:
@@ -77,16 +77,16 @@ struct %(c_name)s {
 /* Members inherited from %(c_name)s: */
 ''',
  c_name=base.c_name())
-ret += gen_struct_fields(base.members)
+ret += gen_struct_members(base.members)
 ret += mcgen('''
 /* Own members: */
 ''')
-ret += gen_struct_fields(members)
+ret += gen_struct_members(members)
 
 if variants:
 ret += gen_variants(variants)
 
-# Make sure that all structs have at least one field; this avoids
+# Make sure that all structs have at least one member; this avoids
 # potential issues with attempting to malloc space for zero-length
 # structs in C, and also incompatibility with C++ (where an empty
 # struct is size 1).
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2308268..b21d3ef 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,9 +15,9 @@
 from qapi import *
 import re
 
-# visit_type_FOO_fields() is always emitted; track if a forward declaration
+# visit_type_FOO_members() is always emitted; track if a forward declaration
 # or implementation has already been output.
-struct_fields_seen = set()
+object_members_seen = set()
 
 
 def gen_visit_decl(name, scalar=False):
@@ -30,10 +30,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_type)sobj, Error **
  c_name=c_name(name), c_type=c_type)
 
 
-def gen_visit_fields_decl(typ):
-if typ.name in struct_fields_seen:
+def gen_visit_members_decl(typ):
+if typ.name in object_members_seen:
 return ''
-struct_fields_seen.add(typ.name)
+object_members_seen.add(typ.name)
 return mcgen('''
 
 static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error 
**errp);
@@ -41,18 +41,18 @@ static void visit_type_%(c_type)s_fields(Visitor *v, 
%(c_type)s *obj, Error **er
  c_type=typ.c_name())
 
 
-def gen_visit_struct_fields(name, base, members, variants):
+def gen_visit_object_members(name, base, members, variants):
 ret = ''
 
 if base:
-ret += gen_visit_fields_decl(base)
+ret += gen_visit_members_decl(base)
 if variants:
 for var in 

[Qemu-devel] [PULL v2 02/12] qapi-dealloc: Reduce use outside of generated code

[Markus Armbruster]

From: Eric Blake 

No need to roll our own use of the dealloc visitors when we can
just directly use the qapi_free_FOO() functions that do what we
want in one line.

In net.c, inline net_visit() into its remaining lone caller.

After this patch, test-visitor-serialization.c is the only
non-generated file that needs to use a dealloc visitor, because
it is testing low level aspects of the visitor interface.

Signed-off-by: Eric Blake 
Message-Id: <1456262075-3311-2-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 hw/acpi/core.c| 11 +--
 net/net.c | 31 ++-
 numa.c|  9 +
 tests/test-opts-visitor.c | 10 +-
 4 files changed, 13 insertions(+), 48 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 3a14e90..3d9e5c4 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -26,7 +26,6 @@
 #include "hw/nvram/fw_cfg.h"
 #include "qemu/config-file.h"
 #include "qapi/opts-visitor.h"
-#include "qapi/dealloc-visitor.h"
 #include "qapi-visit.h"
 #include "qapi-event.h"
 
@@ -297,15 +296,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
 out:
 g_free(blob);
 g_strfreev(pathnames);
-
-if (hdrs != NULL) {
-QapiDeallocVisitor *dv;
-
-dv = qapi_dealloc_visitor_new();
-visit_type_AcpiTableOptions(qapi_dealloc_get_visitor(dv), NULL, ,
-NULL);
-qapi_dealloc_visitor_cleanup(dv);
-}
+qapi_free_AcpiTableOptions(hdrs);
 
 error_propagate(errp, err);
 }
diff --git a/net/net.c b/net/net.c
index aebf753..b0c832e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -42,7 +42,6 @@
 #include "qemu/main-loop.h"
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
-#include "qapi/dealloc-visitor.h"
 #include "sysemu/sysemu.h"
 #include "net/filter.h"
 #include "qapi/string-output-visitor.h"
@@ -1043,38 +1042,28 @@ static int net_client_init1(const void *object, int 
is_netdev, Error **errp)
 }
 
 
-static void net_visit(Visitor *v, int is_netdev, void **object, Error **errp)
-{
-if (is_netdev) {
-visit_type_Netdev(v, NULL, (Netdev **)object, errp);
-} else {
-visit_type_NetLegacy(v, NULL, (NetLegacy **)object, errp);
-}
-}
-
-
 int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
 {
 void *object = NULL;
 Error *err = NULL;
 int ret = -1;
+OptsVisitor *ov = opts_visitor_new(opts);
+Visitor *v = opts_get_visitor(ov);
 
-{
-OptsVisitor *ov = opts_visitor_new(opts);
-
-net_visit(opts_get_visitor(ov), is_netdev, , );
-opts_visitor_cleanup(ov);
+if (is_netdev) {
+visit_type_Netdev(v, NULL, (Netdev **), );
+} else {
+visit_type_NetLegacy(v, NULL, (NetLegacy **), );
 }
 
 if (!err) {
 ret = net_client_init1(object, is_netdev, );
 }
 
-if (object) {
-QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
-
-net_visit(qapi_dealloc_get_visitor(dv), is_netdev, , NULL);
-qapi_dealloc_visitor_cleanup(dv);
+if (is_netdev) {
+qapi_free_Netdev(object);
+} else {
+qapi_free_NetLegacy(object);
 }
 
 error_propagate(errp, err);
diff --git a/numa.c b/numa.c
index 4c4f7f5..da27bf8 100644
--- a/numa.c
+++ b/numa.c
@@ -31,7 +31,6 @@
 #include "include/exec/cpu-common.h" /* for RAM_ADDR_FMT */
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
-#include "qapi/dealloc-visitor.h"
 #include "hw/boards.h"
 #include "sysemu/hostmem.h"
 #include "qmp-commands.h"
@@ -243,13 +242,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error 
**errp)
 
 error:
 error_report_err(err);
-
-if (object) {
-QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
-visit_type_NumaOptions(qapi_dealloc_get_visitor(dv), NULL, ,
-   NULL);
-qapi_dealloc_visitor_cleanup(dv);
-}
+qapi_free_NumaOptions(object);
 
 return -1;
 }
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index b7acf7d..297a02d 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -17,7 +17,6 @@
 #include "qemu/option.h"  /* qemu_opts_parse() */
 #include "qapi/opts-visitor.h"/* opts_visitor_new() */
 #include "test-qapi-visit.h"  /* visit_type_UserDefOptions() */
-#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */
 
 static QemuOptsList userdef_opts = {
 .name = "userdef",
@@ -55,14 +54,7 @@ setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
 static void
 teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data)
 {
-if (f->userdef != NULL) {
-QapiDeallocVisitor *dv;
-
-dv = qapi_dealloc_visitor_new();
-visit_type_UserDefOptions(qapi_dealloc_get_visitor(dv), NULL,
-  >userdef, NULL);
-qapi_dealloc_visitor_cleanup(dv);
-  

[Qemu-devel] [PULL v2 07/12] chardev: Shorten references into ChardevBackend

[Markus Armbruster]

From: Eric Blake 

An upcoming patch will alter how simple unions, like ChardevBackend,
are laid out, which will impact all lines of the form 'backend->u.XXX'
(expanding it to the longer 'backend->u.XXX.data').  For better
legibility in that patch, and less need for line wrapping, it's better
to use a temporary variable to reduce the effect of a layout change to
just the variable initializations, rather than every reference within
a ChardevBackend.  It doesn't hurt that this also makes the code more
consistent: some clients touched here already had a temporary variable
but weren't using it.

Signed-off-by: Eric Blake 
Reviewed-By: Daniel P. Berrange 
Message-Id: <1457021813-10704-6-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 qemu-char.c | 122 
 1 file changed, 66 insertions(+), 56 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index fc8ffda..5ea1d34 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -724,7 +724,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
 ChardevMux *mux = backend->u.mux;
 CharDriverState *chr, *drv;
 MuxDriver *d;
-ChardevCommon *common = qapi_ChardevMux_base(backend->u.mux);
+ChardevCommon *common = qapi_ChardevMux_base(mux);
 
 drv = qemu_chr_find(mux->chardev);
 if (drv == NULL) {
@@ -1043,7 +1043,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
 char *filename_in;
 char *filename_out;
 const char *filename = opts->device;
-ChardevCommon *common = qapi_ChardevHostdev_base(backend->u.pipe);
+ChardevCommon *common = qapi_ChardevHostdev_base(opts);
 
 
 filename_in = g_strdup_printf("%s.in", filename);
@@ -1123,7 +1123,7 @@ static CharDriverState *qemu_chr_open_stdio(const char 
*id,
 ChardevStdio *opts = backend->u.stdio;
 CharDriverState *chr;
 struct sigaction act;
-ChardevCommon *common = qapi_ChardevStdio_base(backend->u.stdio);
+ChardevCommon *common = qapi_ChardevStdio_base(opts);
 
 if (is_daemonized()) {
 error_setg(errp, "cannot use stdio with -daemonize");
@@ -2141,7 +2141,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
 const char *filename = opts->device;
 CharDriverState *chr;
 WinCharState *s;
-ChardevCommon *common = qapi_ChardevHostdev_base(backend->u.pipe);
+ChardevCommon *common = qapi_ChardevHostdev_base(opts);
 
 chr = qemu_chr_alloc(common, errp);
 if (!chr) {
@@ -3216,7 +3216,7 @@ static CharDriverState *qemu_chr_open_ringbuf(const char 
*id,
   Error **errp)
 {
 ChardevRingbuf *opts = backend->u.ringbuf;
-ChardevCommon *common = qapi_ChardevRingbuf_base(backend->u.ringbuf);
+ChardevCommon *common = qapi_ChardevRingbuf_base(opts);
 CharDriverState *chr;
 RingBufCharDriver *d;
 
@@ -3506,26 +3506,29 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, 
ChardevBackend *backend,
 Error **errp)
 {
 const char *path = qemu_opt_get(opts, "path");
+ChardevFile *file;
 
 if (path == NULL) {
 error_setg(errp, "chardev: file: no filename given");
 return;
 }
-backend->u.file = g_new0(ChardevFile, 1);
-qemu_chr_parse_common(opts, qapi_ChardevFile_base(backend->u.file));
-backend->u.file->out = g_strdup(path);
+file = backend->u.file = g_new0(ChardevFile, 1);
+qemu_chr_parse_common(opts, qapi_ChardevFile_base(file));
+file->out = g_strdup(path);
 
-backend->u.file->has_append = true;
-backend->u.file->append = qemu_opt_get_bool(opts, "append", false);
+file->has_append = true;
+file->append = qemu_opt_get_bool(opts, "append", false);
 }
 
 static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
  Error **errp)
 {
-backend->u.stdio = g_new0(ChardevStdio, 1);
-qemu_chr_parse_common(opts, qapi_ChardevStdio_base(backend->u.stdio));
-backend->u.stdio->has_signal = true;
-backend->u.stdio->signal = qemu_opt_get_bool(opts, "signal", true);
+ChardevStdio *stdio;
+
+stdio = backend->u.stdio = g_new0(ChardevStdio, 1);
+qemu_chr_parse_common(opts, qapi_ChardevStdio_base(stdio));
+stdio->has_signal = true;
+stdio->signal = qemu_opt_get_bool(opts, "signal", true);
 }
 
 #ifdef HAVE_CHARDEV_SERIAL
@@ -3533,14 +3536,15 @@ static void qemu_chr_parse_serial(QemuOpts *opts, 
ChardevBackend *backend,
   Error **errp)
 {
 const char *device = qemu_opt_get(opts, "path");
+ChardevHostdev *serial;
 
 if (device == NULL) {
 error_setg(errp, "chardev: serial/tty: no device path given");
 return;
 }
-backend->u.serial = g_new0(ChardevHostdev, 1);
-qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(backend->u.serial));
-backend->u.serial->device = 

[Qemu-devel] [PULL v2 00/12] QAPI patches for 2016-03-04

[Markus Armbruster]

v2: Missing S-o-b supplied

The following changes since commit 3c0f12df65da872d5fbccae469f2cb21ed1c03b7:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20160304' 
into staging (2016-03-04 11:46:32 +)

are available in the git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2016-03-04

for you to fetch changes up to 48eb62a74fc2d6b0ae9e5f414304a85cfbf33066:

  qapi: Drop useless 'data' member of unions (2016-03-05 10:42:06 +0100)


QAPI patches for 2016-03-04


Daniel P. Berrange (1):
  qmp-shell: fix pretty printing of JSON responses

Eric Blake (11):
  qapi-dealloc: Reduce use outside of generated code
  qapi: Rename 'fields' to 'members' in generator
  qapi: Rename 'fields' to 'members' in generated C code
  qapi-visit: Expose visit_type_FOO_members()
  qapi: Update docs to match recent generator changes
  chardev: Shorten references into ChardevBackend
  util: Shorten references into SocketAddress
  ui: Shorten references into InputEvent
  qapi: Avoid use of 'data' member of QAPI unions
  chardev: Drop useless ChardevDummy type
  qapi: Drop useless 'data' member of unions

 backends/baum.c |   2 +-
 backends/msmouse.c  |   2 +-
 block/nbd.c |  14 +-
 blockdev.c  |  31 +--
 docs/qapi-code-gen.txt  | 364 +---
 docs/qmp-spec.txt   |   4 +-
 hw/acpi/core.c  |  11 +-
 hw/char/escc.c  |  12 +-
 hw/input/hid.c  |  36 ++--
 hw/input/ps2.c  |  27 ++-
 hw/input/virtio-input-hid.c |  33 +--
 net/net.c   |  31 +--
 numa.c  |   9 +-
 qapi-schema.json|  15 +-
 qemu-char.c | 179 +---
 qemu-nbd.c  |   9 +-
 replay/replay-input.c   |  31 +--
 scripts/qapi-commands.py|   4 +-
 scripts/qapi-event.py   |   4 +-
 scripts/qapi-types.py   |  19 +-
 scripts/qapi-visit.py   |  51 ++---
 scripts/qapi.py |  20 +-
 scripts/qmp/qmp-shell   |  23 +-
 tests/Makefile  |   1 -
 tests/qapi-schema/qapi-schema-test.json |   2 +-
 tests/qapi-schema/union-clash-data.err  |   0
 tests/qapi-schema/union-clash-data.exit |   1 -
 tests/qapi-schema/union-clash-data.json |   7 -
 tests/qapi-schema/union-clash-data.out  |   9 -
 tests/test-io-channel-socket.c  |  34 +--
 tests/test-opts-visitor.c   |  10 +-
 ui/input-legacy.c   |  25 ++-
 ui/input.c  |  56 ++---
 ui/vnc.c|  39 ++--
 util/qemu-sockets.c |  11 +-
 35 files changed, 564 insertions(+), 562 deletions(-)
 delete mode 100644 tests/qapi-schema/union-clash-data.err
 delete mode 100644 tests/qapi-schema/union-clash-data.exit
 delete mode 100644 tests/qapi-schema/union-clash-data.json
 delete mode 100644 tests/qapi-schema/union-clash-data.out

-- 
2.4.3