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

[Eduardo Habkost]

On Mon, Aug 04, 2014 at 12:00:59PM +0200, Markus Armbruster wrote:
> "Gonglei (Arei)" <address@hidden> writes:
> 
> > Hi,
> >
> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command
> >> >>
> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, 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
> >> >> > +#
> >> >> > +# @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'} }
> >> >> > +
> >> >> > +##
> >> >>
> >> >> I wonder if we could simply use qom-set for that. How many devices
> >> >> actually support having multiple bootindex entries with different
> >> >> suffixes?
> >> >>
> >> > AFAICT, the floppy device support two bootindex with different suffixes.
> >> 
> >> Block device models commonly a single block device.  By convention,
> >> property "drive" is the backend, and property "bootindex" the bootindex,
> >> if the device model supports that.
> >> 
> >> The device ID suffices to identify a block device for such device
> >> models.
> >> 
> >> The floppy device model is an exception.  It folds multiple real-world
> >> objects into one: the controller and the actual drives.  Have a look at
> >> -device isa-fdc,help:
> >> 
> >>     isa-fdc.iobase=uint32
> >>     isa-fdc.irq=uint32
> >>     isa-fdc.dma=uint32
> >>     isa-fdc.driveA=drive
> >>     isa-fdc.driveB=drive
> >>     isa-fdc.bootindexA=int32
> >>     isa-fdc.bootindexB=int32
> >>     isa-fdc.check_media_rate=on/off
> >> 
> >> The properties ending with 'A' or 'B' apply to the first and the second
> >> drive, the others to the controller.
> >> 
> >> Unfortunately, this means the device ID by itself doesn't identify the
> >> floppy device.
> >> 
> > Yes.
> >
> >> Arguably, this is lousy modeling --- we really should model separate
> >> real-world objects as separate objects.  But making floppies pretty (or
> >> even sane) isn't anyone's priority nowadays.
> >> 
> > Hmm... Agreed.
> >
> >> Since qom-set works on *properties*, I can't see why it couldn't be used
> >> for changing bootindexes.  There is no ambiguity even with floppy.
> >
> > Sorry? 
> >
> >> You either set bootindexA or bootindexB.  No need to fuzz around with
> >> suffixes.  Or am I missing something?
> >
> > Your mean that we just need to think about change one bootindex? Either 
> > set bootindexA or bootindexB, do not need pass suffix for identifying?
> 
> Eduardo suggested to think about using qom-set to update the bootindex.
> 
> Could look like
> 
>     { "execute": "qom-set",
>       "arguments": { "path": "/machine/unattached/device[15]",
>                      "property": "bootindexA", "value": 1 } }
> 
> Demonstrates an existing, well-defined way to identify the bootindex to
> change.  Do we really want to invent another one, based on suffix?
> 
> The value of "path" is the QOM path.  I can't remember offhand how to go
> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
> anyway.
> 
> I also don't remember whether there's a nicer QOM path than this one.

The "path" argument may be a "partial path". From qapi-schema.json:

    Partial paths look like relative filenames.  They do not begin
    with a prefix.  The matching rules for partial paths are subtle but
    designed to make specifying objects easy.  At each level of the
    composition tree, the partial path is matched as an absolute path.
    The first match is not returned.  At least two matches are searched
    for.  A successful result is only returned if only one match is
    found.  If more than one match is found, a flag is return to
    indicate that the match was ambiguous.

I haven't tested this. Is the qdev ID visible in device paths somewhere,
allowing it to be used for partial path matching?

-- 
Eduardo