Re: [Qemu-devel] [PATCH v3 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Eric Blake [mailto:address@hidden
> Sent: Thursday, July 31, 2014 6:37 AM
> Subject: Re: [PATCH v3 5/7] qmp: add set-bootindex command
> 
> On 07/25/2014 10:45 PM, address@hidden wrote:
> > From: Gonglei <address@hidden>
> >
> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> >
> > Example QMP command:
> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1", 
> > "bootindex":
> 1, "suffix": "/address@hidden"}}
> > <- { "return": {} }
> >
> > Signed-off-by: Gonglei <address@hidden>
> > Signed-off-by: Chenliang <address@hidden>
> > ---
> >  qapi-schema.json | 16 ++++++++++++++++
> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >  qmp.c            | 17 +++++++++++++++++
> >  3 files changed, 57 insertions(+)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index b11aad2..a9ef0be 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1704,6 +1704,22 @@
> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >
> >  ##
> > +# @set-bootindex:
> > +#
> > +# set bootindex of a devcie
> 
> s/devcie/device/
> 
OK.

> Just to make sure this command is not write-only, it would be nice to
> mention which query-* command can be used to learn a device's current
> bootindex.
> 
Yes. I am thinking about introducing a query-bootindex command, which
can show a device's current bootindex and bootindex's suffix.

> > +#
> > +# @id: the name of the device
> > +# @bootindex: the bootindex of the device
> > +# @suffix: #optional a suffix of the device
> > +#
> > +# Returns: Nothing on success
> > +#          If @id is not a valid device, DeviceNotFound
> > +#
> > +# Since: 2.2
> > +##
> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex': 'int', 
> > '*suffix':
> 'str'} }
> 
> Long line; wrap it to stay in 80 columns.
> 
OK.

> 
> > +
> > +SQMP
> > +set-bootindex
> > +--------------------
> 
> Match the ---- length to the command name.
> 
OK.

> > +++ b/qmp.c
> > @@ -684,6 +684,23 @@ void qmp_object_del(const char *id, Error **errp)
> >      object_unparent(obj);
> >  }
> >
> > +void qmp_set_bootindex(const char *id, int64_t bootindex,
> > +                     bool has_suffix, const char *suffix, Error **errp)
> 
> Indentation is off.
> 
OK.

> > +{
> > +    DeviceState *dev;
> > +
> > +    dev = qdev_find_recursive(sysbus_get_default(), id);
> > +    if (NULL == dev) {
> 
> Code like Yoda we do not.  This is more idiomatically written 'if
> (!dev)' or 'if (dev == NULL)'.
> 
OK.

Thanks for your careful reviewing, Eric. 
I will fix those problems in the next version.

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

Best regards,
-Gonglei