[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] qmp: Return 'user_creatable' & 'hotpluggable' fie
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [RFC] qmp: Return 'user_creatable' & 'hotpluggable' fields on qom-list-types |
Date: |
Mon, 22 May 2017 10:56:25 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, May 22, 2017 at 09:17:58AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
>
> > On Thu, May 18, 2017 at 01:59:53PM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost <address@hidden> writes:
> >>
> >> > Currently there's no way for QMP clients to get a list of device types
> >> > that are really usable with -device. This information would be useful
> >> > for management software and test scripts (like the device-crash-test
> >> > script I have submitted recently). Interestingly, the "info qdm" HMP
> >> > command provides this information, but no QMP command does.
> >> >
> >> > Add two new fields to the return value of qom-list-types:
> >> > "user-creatable-device" and "hotpluggable-device".
> >>
> >> Does the combination
> >>
> >> "user-creatable-device": false,
> >> "hotpluggable-device": true
> >>
> >> make any sense?
> >
> > It doesn't, and the code ensures this won't happen.
>
> Would a single variable with three states be clearer?
I don't think it would, in this specific case. I can't even see
how to document an enum in this case without being too verbose
and confusing.
If one day we add a different method to plug devices (e.g. by
building a composite QOM object first, and then plugging it), how
exactly would we extend the enum to indicate the new method is
supported by a device?
> I don't like
> entangled booleans much...
I tend to prefer booleans because each one describe a single fact
and nothing else.
>
> >> Exposing information on user-creatable/hot-pluggable via QMP makes
> >> sense. The question is how. This is a design question, and as so often
> >> with design questions, I need more words to make my case than I'd like
> >> to. Please bear with me.
> >>
> >> > Also, add extra
> >> > arguments for filtering the list of types based on the new fields.
> >>
> >> I consider the case for filtering weak. Let's ignore this part for now.
> >
> > I considered sending a version that didn't include filtering. I
> > will be happy to ignore it. :)
> >
> >>
> >> We have a number of commands to introspect static information,
> >> e.g. query-version, query-commands, query-qmp-schema, query-target,
> >> query-machines, query-cpu-definitions, query-chardev-backends,
> >> device-list-properties, qom-list-types.
> >>
> >> Aside: us abandoning the convention to name such commands query-FOO is
> >> regrettable.
> >>
> >> In its basic form, i.e. without arguments, qom-list-types does what its
> >> name suggests: it lists the QOM types.
> >>
> >> It also permits finding out whether a type is abstract, but only in a
> >> roundabout way: subtract the result of running it without arguments (or
> >> with 'abstract':false) from the result with 'abstract':true.
> >>
> >> It also permits finding the "implements" relation, but only in an even
> >> more roundabout way: run it with 'implements':T for every abstract T,
> >> then solve the resulting puzzle.
> >>
> >> Unless there's a direct way to find both (and I'm not aware of any),
> >> this is bad design. The obvious fix is to extend its return type to
> >> include the information.
> >
> > Agreed.
> >
> >>
> >> With qom-list-types fixed that way, there's precedence for exposing
> >> additional information on QOM types there.
> >>
> >> Note that both the "abstract" bit and the "implements directly" list
> >> apply to any QOM type, not just to certain subtypes. As proposed,
> >> "user-creatable-device' and "hotpluggable-device" apply only to the
> >> "device" subtype. There's no precedence for exposing information
> >> specific to certain subtypes.
> >>
> >> If we want to do it anyway, then qom-list-type should perhaps return a
> >> union. Taken to the logical conclusion, this becomes a nest of unions
> >> mirroring the "direct subtype of" relation. Hmm.
> >
> > I don't think that's the logical conclusion. The differences
> > between "object" and "device" are hardcoded in our interfaces: we
> > already have -object and -device. Modelling our data to take
> > care of thoes differences doesn't mean we will also have to treat
> > the differences between other QOM types (e.g. between
> > "pci-device" and "usb-device") the same way.
>
> *If* we make qom-list-type return a union to properly reflect which
> results apply to which devices, *then* the logical conclusion is a nest
> of unions mirroring the "direct subtype of" relation.
>
> "Hmm" expresses my less than enthusiastic reaction to the thought of
> such a nest.
>
> > Now, we could choose to encode that in different ways. We could
> > have a single command for all QOM types (like qom-list-types or a
> > new query-qom-type command) that return an union, or have
> > separate commands for object types and device types. I'm not
> > sure what's the better option here (see below for additional
> > comments on that).
> >
> >>
> >> However, we already have a command to introspect device types:
> >> device-list-properties. Can we expose the information as read-only
> >> property/ies of type "device" and be done?
> >
> > I don't think we should. device-list-properties has a very
> > specific purpose: listing QOM properties that can be read or
> > written using qom-get and qom-set, or set in -device. If there's
> > no use case for qom-get/qom-set on a property like "hotpluggable"
> > or "user-creatable", we shouldn't make them QOM properties (hence
> > they shouldn't appear on device-list-properties).
>
> I figure your argument boils down to "we should distinguish between
> information about a device instance and a device type, and properties
> are for the former, but ...
Indirectly, yes: device-list-properties takes a device type as
argument, but its purpose is to list the names of QOM properties
we can set/get on device instances.
>
> > But that doesn't mean we can't have something like a
> > "query-device-type" command that returns other information about
> > a given device type, in addition to the property list.
>
> ... we still need something for the latter."
True.
>
> > (In my specific use case (the device-crash-test script), I would
> > prefer to avoid having to fetch the full list of properties of
> > every single device type, just to find out which ones are
> > user-creatable. But this is not really performance-critical
> > code, so I can live with that.)
> >
> > So, I see a few options here:
> >
> > 1) Including DeviceClass::user_creatable and
> > DeviceClass::hotpluggable in qom-list-types output. Probably
> > using an union.
> >
> > 2) Adding a new "query-device-type" command, returning
> > DeviceClass::user_creatable and DeviceClass::hotpluggable, and
> > possibly other DeviceClass fields (like the list of
> > properties)
> >
> > 3) Adding a new "query-qom-type" command, returning extended
> > information about a QOM type. This might include the list of
> > properties supported by the type. This might include
> > device-specific data if the command return an union.
>
> That way's a nest of unions.
I still don't think it means a nest of unions, but I prefer
option 2 to option 3+union anyway. Having two separate commands
returning different structures sounds simpler and better than a
single command returning an union.
>
> > Those options are not mutually exclusive. e.g.: we might decide
> > that a small set of fields is useful if included in
> > qom-list-types too, even if we implement a query-device-type or
> > query-qom-type command.
>
> Yes. But let's start with just one.
I already submitted a RFC to extend qom-list-types. I will
probably submit another RFC to add a query-device-type command.
>
> >> But perhaps they aren't really specific to devices. There are other QOM
> >> types that can be created by users, e.g. with -object, so the
> >> "user-creatable" bit applies more widely. Are any of them only
> >> creatable during initial startup? If yes, then that applies more
> >> widely, too.
> >
> > I prefer to have very specific semantics like "this type can be
> > used with -device" than something more generic and prone to
> > confusion like "this can be user-creatable, but the method used
> > to create it can vary".
>
> Point taken.
>
> We have user-creatable objects and user-creatable devices. The latter
> are objects, but not user-creatable objects. Aesthetically displeasing.
Also, support for -object is represented using the
"user-creatable" interface name. So user-creatable devices do
not implement the "user-creatable" interface.
We need better names for all that. I suggest we call objects
supported by -object "user-creatable backend objects" instead of
just "user-creatable objects". Devices are objects, but they are
not backend objects.
>
> >> > I'm not sure about the naming of the new command arguments. Maybe the
> >> > names are too long, but I believe that
> >> > "user-creatable-devices-only=false"
> >> > would have more obvious semantics than "user-creatable-devices=false"
> >> > (the latter looks ambiguous: it could mean "return only
> >> > non-user-creatable devices" or "return all devices").
> >>
> >> I consider the filtering feature unnecessary complexity. The filtering
> >> we have got in against my objections. I won't veto additional filtering
> >> outright, but I will insist on test coverage.
> >>
> >> The unfiltered output of qom-list-types is less than 10 KiB. Even if we
> >> extend it some, the need to filter it client-side seems dubious. If a
> >> management application really wants to save resources here, it should
> >> cache the result and re-get it only when QEMU changes.
> >
> > I don't think we really need filtering, and I will be happy to
> > remove that feature on the next version.
>
> We can always add it later if we find a real need.
--
Eduardo