Re: [Qemu-devel] IDs in QOM

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 07/10/2014 14:16, Markus Armbruster ha scritto:
>>> > Possibly, except this would propagate all the way through the APIs.  For
>>> > example, right now [*] is added automatically to MemoryRegion
>>> > properties, but this can change in the future since many MemoryRegions
>>> > do not need array-like names.  Then you would have two sets of
>>> > MemoryRegion creation APIs, one that array-ifies names and one
>>> > that doesn't.
>> Why couldn't you have a separate name generator there as well?
>> 
>> QOM:
>> * object_property_add() takes a non-magical name argument
>> * object_gen_name() takes a base name and generates a stream of
>>   derived names suitable for object_property_add()
>> 
>> Memory:
>> * memory_region_init() takes a non-magical name argument
>> * memory_gen_name() takes a base name... you get the idea
>>   actually a wrapper around object_gen_name()
>
> I see what you mean; you could even reuse object_gen_name().  It looks
> sane, I guess one has to see a patch to judge if it also _is_ sane. :)

Yup.  Any takers?

>> > > Why is it a good idea have two separate restrictions on property names?
>> > > A loser one that applies always (anything but '\0' and '/'), and a
>> > > stricter one that applies sometimes (letters, digits, '-', '.', '_',
>> > > starting with a letter).
>> > > 
>> > > If yes, how is "sometimes" defined?
>> >
>> > It applies to objects created by the user (either in
>> > /machine/peripheral, or in /objects).  Why the restriction?  For
>> > -object, because creating the object involves QemuOpts.  You then have
>> > two ways to satisfy the principle of least astonishment:
>> >
>> > 1) always use the same restriction when a user creates objects;
>> >
>> > 2) do not introduce restrictions when a user is not using QemuOpts.
>> >
>> > We've been doing (2) so far; often it is just because QMP wrappers also
>> > used QemuOpts, but not necessarily.  So object_add just does the same.
>>
>> We've been doing (2) so far simply because we've never wasted a thought
>> on it!  Since we're wasting thoughts now: which one do we like better?
>
> User interfaces other than QOM have been doing (2) too.

>From an implementation point of view, doing nothing is doing (2).

>From an interface point of view, it's (2) only when interfaces bypassing
QemuOpts exist.

> netdev-add and device-add have been doing (2) because they use QemuOpts
> under the hood.

qdev_device_add() uses QemuOpts.  Until relatively recently, that was
the only way to create a qdev.  Nowadays, you could create one using QOM
directly, bypassing QemuOpts's ID restriction.

netdev-add is similar iirc.

> blockdev-add has been consciously doing (2) for node-name.

Actually, we consciously fixed it to do (1):

commit 9aebf3b89281a173d2dfeee379b800be5e3f363e
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Thu Sep 25 09:54:02 2014 +0200

    block: Validate node-name
    
    The device_name of a BlockDriverState is currently checked because it is
    always used as a QemuOpts ID and qemu_opts_create() checks whether such
    IDs are wellformed.
    
    node-name is supposed to share the same namespace, but it isn't checked
    currently. This patch adds explicit checks both for device_name and
    node-name so that the same rules will still apply even if QemuOpts won't
    be used any more at some point.
    
    qemu-img used to use names with spaces in them, which isn't allowed any
    more. Replace them with underscores.

> chardev-add has been doing (1), and I'd argue that this is a bug in
> chardev-add.

I'm not sure I got you here.

> QOM has two families of operations.
>
> One is -object/object-add/object-del.  This is a high-level operation
> that only works with specific QOM classes (those that implement
> UserCreatable) and only operate on a specific part of the QOM tree
> (/objects).
>
> The other is qom-get/qom-set.  This is a low-level operation that can
> explore all of the QOM tree.  It cannot _create_ new objects and
> properties, however, so the user cannot escape the naming sandbox that
> we put in place for him.
>
> I think it's fair to limit the high-level operations to the same id
> space, no matter if they're QemuOpts based or not.

Yes.

"QemuOpts-based" should never be a concern at the interface.  It's an
implementation detail.

>> Based on experience, I'd rather not make "user-created"
>> vs. "system-created" a hard boundary.  Once a system-created funny name
>> has become ABI, we can't ever let the user create it.  One reason for me
>> to prefer (1).
>
> Anything that is outside /objects is "funny", not just anything that has
> weird characters in its name.  The QOM API consists of "magic" object
> canonical paths and magic property names which, as far as I know, can be
> easily listed:
>
> * the aforementioned /machine.rtc-time that lets you detect missed
> RTC_CHANGE events
>
> * the /backend tree that includes info on the graphic consoles.  Not
> sure if this is considered stable, but it's there.
>
> * /machine/peripheral/foo lets you peek at run-time properties of
> -device id=foo - virtio-ballon has a couple of run-time properties,
> whose status I am not certain of.  Probably stable but undocumented.
>
> * /objects/bar lets you reconstruct the properties of -object id=bar -
> there are no such run-time properties with any promised stability.
>
>
> In other words, practically all of the QOM API is outside /objects.
>
> But not all hope is lost.  Were we to provide user access to the
> creation of graphic consoles, we could preserve the /backend API via
> aliases and links.  This way, anything that currently happens in
> /machine or /backend can tomorrow happen in /objects, without breaking
> backwards compatibility.
>
> Similarly, a QOMified block-backend could be either:
>
> * an object that QEMU creates for you when you give -device
> scsi-disk,id=disk,drive=foo.  The canonical path could be something like
> /machine/peripheral/disk/drive-backend, with a link in
> /machine/peripheral/disk/backend.
>
> * an object that you create with -object
> block-backend,id=bar,blockdev=myimg and reference with -device
> scsi-disk,backend=bar.  The canonical path would be of course
> /objects/bar, but the same link would exist in
> /machine/peripheral/disk/backend.
>
> In either case, you would be able to find the block-backend using the
> same QOM path and property.
>
>> So the "automatic arrayification" convenience feature added a property
>> name restriction.  What makes us sure this is the last time we add name
>> restrictions?
>
> Nothing.  However, does it matter, as long as the now-disallowed names
> were not possible at all in -object/object_add?

I think the simple argument struggling to get out of our conversation
could go something like this.

We have an internal and an external object creation interface.
Differerent rules apply.

The internal interface is not stable.  We can change its rules at any
time.  Therefore, getting them exactly right isn't terribly important.
However, the result of creating something with the internal interface
may be ABI.  So far our ABI promise in that area is still unclear[*].

The external interface is ABI.  It can only create well-formed names in
/objects/.


[*] Polite way to say "useless unless you can twist arms to make it
stick" ;-P