On 05/27/2014 08:28 AM, Jeff Cody wrote: > Now that active layer block-commit is supported, the 'top' argument > no longer needs to be mandatory. > > Change it to optional, with the default being the active layer in the > device chain. > > Reviewed-by: Eric Blake <ebl...@redhat.com> > Reviewed-by: Benoit Canet <ben...@irqsave.net> > Signed-off-by: Jeff Cody <jc...@redhat.com> > --- > blockdev.c | 3 ++- > qapi-schema.json | 7 ++++--- > qmp-commands.hx | 5 +++-- > tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 9a9bdec..b37ace7 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1912,7 +1912,8 @@ void qmp_block_stream(const char *device, bool has_base, > } > > void qmp_block_commit(const char *device, > - bool has_base, const char *base, const char *top, > + bool has_base, const char *base, > + bool has_top, const char *top, > bool has_speed, int64_t speed, > Error **errp) > {
Hmm. Later in this function we have: if (top) { if (strcmp(bs->filename, top) != 0) { top_bs = bdrv_find_backing_image(bs, top); } } which is not ideal; it means we are DEPENDING on qapi to NULL-initialize a pointer when has_top is false. Although we have (finally!) made that guarantee (see commit fc13d937), I worry that backporting this patch but not that one may cause a use of undefined memory, unless you add an explicit: if (!has_top) { top = NULL; } In other words, I'm a bit reluctant to use default initialization values without documenting and testing that we can rely on them. On the other hand, prior to this commit, the 'if (top)' condition was dead code (since QMP doesn't allow "arguments":{"top":null}, there was previously no way for a caller to pass a NULL 'top' parameter - so the fact that the code was already checking for a NULL top implies that we had planned ages ago to make top a conditional parameter). So on that grounds, we are merely doing what we'd been planning all along. I can live with keeping my R-b without a respin, even though it feels a bit dirty to not be checking has_top. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature