| Message ID | 1412078370-3555-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/30/2014 05:59 AM, Markus Armbruster wrote: > IDs have long spread beyond QemuOpts: not everything with an ID > necessarily goes through QemuOpts. Commit 9aebf3b is about such a > case: block layer names are meant to be well-formed IDs, but some of > them don't go through QemuOpts, and thus weren't checked. The commit > fixed that the straightforward way: rename the internal QemuOpts > helper id_wellformed() to qemu_opts_id_wellformed() and give it > external linkage. > > Instead of using it directly in block.c, the commit adds wrapper > bdrv_is_valid_name(), probably to hide the connection to QemuOpts. > > Go one logical step further: emancipate IDs from QemuOpts. Rename the > function back to id_wellformed(), and put it in another file. While > there, clean up its value to bool. Peel off the bdrv_is_valid_name() > wrapper. > --- > block.c | 9 ++------- > include/qemu-common.h | 3 +++ > include/qemu/option.h | 1 - > util/Makefile.objs | 1 + > util/id.c | 28 ++++++++++++++++++++++++++++ > util/qemu-option.c | 17 +---------------- > 6 files changed, 35 insertions(+), 24 deletions(-) > create mode 100644 util/id.c Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster <armbru@redhat.com> writes: > IDs have long spread beyond QemuOpts: not everything with an ID > necessarily goes through QemuOpts. Commit 9aebf3b is about such a > case: block layer names are meant to be well-formed IDs, but some of > them don't go through QemuOpts, and thus weren't checked. The commit > fixed that the straightforward way: rename the internal QemuOpts > helper id_wellformed() to qemu_opts_id_wellformed() and give it > external linkage. > > Instead of using it directly in block.c, the commit adds wrapper > bdrv_is_valid_name(), probably to hide the connection to QemuOpts. > > Go one logical step further: emancipate IDs from QemuOpts. Rename the > function back to id_wellformed(), and put it in another file. While > there, clean up its value to bool. Peel off the bdrv_is_valid_name() > wrapper. Thinking about IDs has led me to QOM and down a rabbit hole. Here goes. QemuOpts IDs need to be well-formed: consist of letters, digits, '-', '.', '_', starting with a letter. See id_wellformed(), commit b560a9a "qemu-option: Reject anti-social IDs". This restriction has served us well. The commit message above is a case where we started with QemuOpts, then added QMP interfaces bypassing QemuOpts, which also bypassed its ID well-formedness rule. The bypass is a bug, and we fixed it. The patch cleans up the fix. There might be more of the same. QOM is the other way round: we started with a C API, and later added external interfaces, namely -object and object-add. QMP object-add has no need for QemuOpts. See qmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and object_add (HMP) command". Parameter 'id' is an arbitrary string. qmp_object_add() passes it to object_add(), which runs object_property_add_child(container_get(object_get_root(), "/objects"), id, obj, &local_err); On an abstract level, this puts the newly created object in container /objects/ with name id. Down & concrete, the container object has a property named name pointing the the new object. Property names are arbitrary strings; object_property_add() doesn't care. However, we also have the concept of a path, which are sequences of names separated by '/'. See object_resolve_path_type(), container_get(). What if a name contains '/'? Or other funny characters? Actually, "property names are arbitrary strings" has recently become untrue: commit 339659 "qom: Add automatic arrayification to object_property_add()" changed names to have embedded syntax. A property name ending with "[*]" makes the property special. Normal properties can be added just once, and an attempt to add it again is an error. These special properties, however, can be added any number of times, and each time the name "[*]" is silently rewritten to "[N]", where N counts from 0. Is mangling array-ness into the name really a good idea? Isn't this type matter, not name matter? Backtracking a bit... Unlike QMP object-add, -object ) and HMP object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add -object option to create QOM objects from the command line", and hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus bound by its well-formedness rule. Therefore, -object and HMP object-add only support a subset of the possible names. In particular, they do not permit "automatic arrayification". Should QOM names be (well-formed!) IDs? If not, is -object and HMP object-add restricting the names to well-formed IDs a bug?
On Tue, Sep 30, 2014 at 01:59:30PM +0200, Markus Armbruster wrote: > +bool id_wellformed(const char *id) > +{ > + int i; > + > + if (!qemu_isalpha(id[0])) { > + return 0; false would be a bit nicer since the other return cases use true/false.
On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: This discussion seems orthogonal to your patch. But I'm not applying it yet to give more time for discussion/review of the patch. > Is mangling array-ness into the name really a good idea? Isn't this > type matter, not name matter? I agree. It's nasty to hack the array selector into the name and will probably cause us pain down the line. > Backtracking a bit... Unlike QMP object-add, -object ) and HMP > object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add > -object option to create QOM objects from the command line", and > hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and > object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus > bound by its well-formedness rule. > > Therefore, -object and HMP object-add only support a subset of the > possible names. > > In particular, they do not permit "automatic arrayification". > > Should QOM names be (well-formed!) IDs? Yes, I think that is sane. Are there any invalid IDs used as QOM names today? Hopefully the answer is no and we can lock everything down using id_wellformed().
Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: > On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: > > This discussion seems orthogonal to your patch. But I'm not applying it > yet to give more time for discussion/review of the patch. > >> Is mangling array-ness into the name really a good idea? Isn't this >> type matter, not name matter? > > I agree. It's nasty to hack the array selector into the name and will > probably cause us pain down the line. > >> Backtracking a bit... Unlike QMP object-add, -object ) and HMP >> object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add >> -object option to create QOM objects from the command line", and >> hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and >> object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus >> bound by its well-formedness rule. >> >> Therefore, -object and HMP object-add only support a subset of the >> possible names. >> >> In particular, they do not permit "automatic arrayification". >> >> Should QOM names be (well-formed!) IDs? > > Yes, I think that is sane. > > Are there any invalid IDs used as QOM names today? > > Hopefully the answer is no and we can lock everything down using > id_wellformed(). On IRC I was arguing against that, preferring some more specific object_property_name_wellformed() or so. This could be called from object_property_add(), with invalid names returning an Error *. Only thing to check for would be '/'? Regards, Andreas
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Tue, Sep 30, 2014 at 01:59:30PM +0200, Markus Armbruster wrote: >> +bool id_wellformed(const char *id) >> +{ >> + int i; >> + >> + if (!qemu_isalpha(id[0])) { >> + return 0; > > false would be a bit nicer since the other return cases use true/false. Missed when I changed the value to bool. Would you be willing to fix this up on commit? If not, I'll respin.
On Thu, Oct 02, 2014 at 03:26:28PM +0200, Andreas Färber wrote: > Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: > > On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: > >> Markus Armbruster <armbru@redhat.com> writes: > > > > This discussion seems orthogonal to your patch. But I'm not applying it > > yet to give more time for discussion/review of the patch. > > > >> Is mangling array-ness into the name really a good idea? Isn't this > >> type matter, not name matter? > > > > I agree. It's nasty to hack the array selector into the name and will > > probably cause us pain down the line. > > > >> Backtracking a bit... Unlike QMP object-add, -object ) and HMP > >> object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add > >> -object option to create QOM objects from the command line", and > >> hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and > >> object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus > >> bound by its well-formedness rule. > >> > >> Therefore, -object and HMP object-add only support a subset of the > >> possible names. > >> > >> In particular, they do not permit "automatic arrayification". > >> > >> Should QOM names be (well-formed!) IDs? > > > > Yes, I think that is sane. > > > > Are there any invalid IDs used as QOM names today? > > > > Hopefully the answer is no and we can lock everything down using > > id_wellformed(). > > On IRC I was arguing against that, preferring some more specific > object_property_name_wellformed() or so. This could be called from > object_property_add(), with invalid names returning an Error *. > > Only thing to check for would be '/'? Why risk the inability to specify QOM names via QemuOpts? What is the benefit of allowing two different sets of valid names? Stefan
On Thu, Oct 02, 2014 at 03:42:21PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@gmail.com> writes: > > > On Tue, Sep 30, 2014 at 01:59:30PM +0200, Markus Armbruster wrote: > >> +bool id_wellformed(const char *id) > >> +{ > >> + int i; > >> + > >> + if (!qemu_isalpha(id[0])) { > >> + return 0; > > > > false would be a bit nicer since the other return cases use true/false. > > Missed when I changed the value to bool. Would you be willing to fix > this up on commit? If not, I'll respin. Yes, I'll fix it when merging tomorrow. Stefan
Andreas Färber <afaerber@suse.de> writes: > Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: >> On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: >>> Markus Armbruster <armbru@redhat.com> writes: >> >> This discussion seems orthogonal to your patch. But I'm not applying it >> yet to give more time for discussion/review of the patch. >> >>> Is mangling array-ness into the name really a good idea? Isn't this >>> type matter, not name matter? >> >> I agree. It's nasty to hack the array selector into the name and will >> probably cause us pain down the line. Andreas? >>> Backtracking a bit... Unlike QMP object-add, -object ) and HMP >>> object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add >>> -object option to create QOM objects from the command line", and >>> hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and >>> object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus >>> bound by its well-formedness rule. >>> >>> Therefore, -object and HMP object-add only support a subset of the >>> possible names. >>> >>> In particular, they do not permit "automatic arrayification". >>> >>> Should QOM names be (well-formed!) IDs? >> >> Yes, I think that is sane. >> >> Are there any invalid IDs used as QOM names today? >> >> Hopefully the answer is no and we can lock everything down using >> id_wellformed(). > > On IRC I was arguing against that, preferring some more specific > object_property_name_wellformed() or so. This could be called from > object_property_add(), with invalid names returning an Error *. > > Only thing to check for would be '/'? We obviously have to outlaw '/'. However, unless we outlaw just like id_wellformed(): >> If not, is -object and HMP object-add restricting the names to >> well-formed IDs a bug? Opinions? My opinion is to stick to id_wellformed() and call it a day.
Am 02.10.2014 um 16:21 schrieb Markus Armbruster: > Andreas Färber <afaerber@suse.de> writes: > >> Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: >>> On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: >>>> Markus Armbruster <armbru@redhat.com> writes: >>> >>> This discussion seems orthogonal to your patch. But I'm not applying it >>> yet to give more time for discussion/review of the patch. >>> >>>> Is mangling array-ness into the name really a good idea? Isn't this >>>> type matter, not name matter? >>> >>> I agree. It's nasty to hack the array selector into the name and will >>> probably cause us pain down the line. > > Andreas? -> Paolo ;) >>>> Backtracking a bit... Unlike QMP object-add, -object ) and HMP >>>> object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add >>>> -object option to create QOM objects from the command line", and >>>> hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and >>>> object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus >>>> bound by its well-formedness rule. >>>> >>>> Therefore, -object and HMP object-add only support a subset of the >>>> possible names. >>>> >>>> In particular, they do not permit "automatic arrayification". >>>> >>>> Should QOM names be (well-formed!) IDs? >>> >>> Yes, I think that is sane. >>> >>> Are there any invalid IDs used as QOM names today? >>> >>> Hopefully the answer is no and we can lock everything down using >>> id_wellformed(). >> >> On IRC I was arguing against that, preferring some more specific >> object_property_name_wellformed() or so. This could be called from >> object_property_add(), with invalid names returning an Error *. >> >> Only thing to check for would be '/'? > > We obviously have to outlaw '/'. However, unless we outlaw just like > id_wellformed(): > >>> If not, is -object and HMP object-add restricting the names to >>> well-formed IDs a bug? > > Opinions? > > My opinion is to stick to id_wellformed() and call it a day. We cannot use id_wellformed() in object_property_add(), as I assume it prohibits '.' as well, which we need for bus names. This would be easier to discuss if you gave us the exact list of outlawed characters for comparison. ',' is probably another? If you just want to call id_wellformed() for -object / object-add, I won't object to restricting it beyond the necessary, but it'll lead to two places doing validity checks for QOM. Regards, Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 02.10.2014 um 16:21 schrieb Markus Armbruster: >> Andreas Färber <afaerber@suse.de> writes: >> >>> Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: >>>> On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: >>>>> Markus Armbruster <armbru@redhat.com> writes: >>>> >>>> This discussion seems orthogonal to your patch. But I'm not applying it >>>> yet to give more time for discussion/review of the patch. >>>> >>>>> Is mangling array-ness into the name really a good idea? Isn't this >>>>> type matter, not name matter? >>>> >>>> I agree. It's nasty to hack the array selector into the name and will >>>> probably cause us pain down the line. >> >> Andreas? > > -> Paolo ;) Paolo? >>>>> Backtracking a bit... Unlike QMP object-add, -object ) and HMP >>>>> object-add use QemuOpts. See object_create(), commit 68d98d3 "vl: add >>>>> -object option to create QOM objects from the command line", and >>>>> hmp_object_add(), commit cff8b2c "monitor: add object-add (QMP) and >>>>> object_add (HMP) command". Parameter 'id' is the QemuOpts ID, thus >>>>> bound by its well-formedness rule. >>>>> >>>>> Therefore, -object and HMP object-add only support a subset of the >>>>> possible names. >>>>> >>>>> In particular, they do not permit "automatic arrayification". >>>>> >>>>> Should QOM names be (well-formed!) IDs? >>>> >>>> Yes, I think that is sane. >>>> >>>> Are there any invalid IDs used as QOM names today? >>>> >>>> Hopefully the answer is no and we can lock everything down using >>>> id_wellformed(). >>> >>> On IRC I was arguing against that, preferring some more specific >>> object_property_name_wellformed() or so. This could be called from >>> object_property_add(), with invalid names returning an Error *. >>> >>> Only thing to check for would be '/'? >> >> We obviously have to outlaw '/'. However, unless we outlaw just like >> id_wellformed(): >> >>>> If not, is -object and HMP object-add restricting the names to >>>> well-formed IDs a bug? >> >> Opinions? >> >> My opinion is to stick to id_wellformed() and call it a day. > > We cannot use id_wellformed() in object_property_add(), as I assume it > prohibits '.' as well, which we need for bus names. It doesn't. > This would be easier to discuss if you gave us the exact list of > outlawed characters for comparison. ',' is probably another? Quoting myself: QemuOpts IDs need to be well-formed: consist of letters, digits, '-', '.', '_', starting with a letter. See id_wellformed(), commit b560a9a "qemu-option: Reject anti-social IDs". This restriction has served us well. We can discuss permitting more characters. > If you just want to call id_wellformed() for -object / object-add, I > won't object to restricting it beyond the necessary, but it'll lead to > two places doing validity checks for QOM. id_wellformed() is *already* called for -object and HMP object-add. That's exactly my point! Please reread my explanation, and if it's insufficient, ask for clarification. Subject: IDs in QOM (was: [PATCH] util: Emancipate id_wellformed() from QemuOpts) Date: Wed, 01 Oct 2014 14:33:47 +0200 Message-ID: <87iok46kb8.fsf@blackfin.pond.sub.org>
Il 02/10/2014 16:59, Markus Armbruster ha scritto: > Andreas Färber <afaerber@suse.de> writes: > >> Am 02.10.2014 um 16:21 schrieb Markus Armbruster: >>> Andreas Färber <afaerber@suse.de> writes: >>> >>>> Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: >>>>> On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: >>>>>> Markus Armbruster <armbru@redhat.com> writes: >>>>> >>>>> This discussion seems orthogonal to your patch. But I'm not applying it >>>>> yet to give more time for discussion/review of the patch. >>>>> >>>>>> Is mangling array-ness into the name really a good idea? Isn't this >>>>>> type matter, not name matter? >>>>> >>>>> I agree. It's nasty to hack the array selector into the name and will >>>>> probably cause us pain down the line. >>> >>> Andreas? >> >> -> Paolo ;) > > Paolo? Uhm, I had written an answer but Thunderbird ate it. Oh well. I think foo[*] is not really a matter of typing, but a matter of grouping similar children. It does not really matter if foo[1] is deleted while foo[2] still exists, and it does not really matter if the next object created will be foo[1] or foo[3]. While we do not have any example, QOM could support true array properties, e.g. with type intList. Also note that while foo[*] was a generalization of MemoryRegion code, the same idea has also existed forever for devices. /machine/peripheral_anon and /machine/unattached do not currently use automatic array-ification via foo[*], but they could. >> If you just want to call id_wellformed() for -object / object-add, I >> won't object to restricting it beyond the necessary, but it'll lead to >> two places doing validity checks for QOM. > > id_wellformed() is *already* called for -object and HMP object-add. > That's exactly my point! > > Please reread my explanation, and if it's insufficient, ask for > clarification. > > Subject: IDs in QOM (was: [PATCH] util: Emancipate id_wellformed() from QemuOpts) > Date: Wed, 01 Oct 2014 14:33:47 +0200 > Message-ID: <87iok46kb8.fsf@blackfin.pond.sub.org> I think it is okay to keep calling id_wellformed(), since it covers the QOM-specific constraint that slashes must be outlawed. It just makes the children of /objects a bit more restricted in their naming, but that is harmless because object_del can only delete objects created with object_add. Paolo
On Tue, Sep 30, 2014 at 01:59:30PM +0200, Markus Armbruster wrote: > IDs have long spread beyond QemuOpts: not everything with an ID > necessarily goes through QemuOpts. Commit 9aebf3b is about such a > case: block layer names are meant to be well-formed IDs, but some of > them don't go through QemuOpts, and thus weren't checked. The commit > fixed that the straightforward way: rename the internal QemuOpts > helper id_wellformed() to qemu_opts_id_wellformed() and give it > external linkage. > > Instead of using it directly in block.c, the commit adds wrapper > bdrv_is_valid_name(), probably to hide the connection to QemuOpts. > > Go one logical step further: emancipate IDs from QemuOpts. Rename the > function back to id_wellformed(), and put it in another file. While > there, clean up its value to bool. Peel off the bdrv_is_valid_name() > wrapper. > --- > block.c | 9 ++------- > include/qemu-common.h | 3 +++ > include/qemu/option.h | 1 - > util/Makefile.objs | 1 + > util/id.c | 28 ++++++++++++++++++++++++++++ > util/qemu-option.c | 17 +---------------- > 6 files changed, 35 insertions(+), 24 deletions(-) > create mode 100644 util/id.c Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
[Quoted material edited somewhat to provide better context] Paolo Bonzini <pbonzini@redhat.com> writes: > Il 02/10/2014 16:59, Markus Armbruster ha scritto: >> Andreas Färber <afaerber@suse.de> writes: >> >>> Am 02.10.2014 um 16:21 schrieb Markus Armbruster: >>>> Andreas Färber <afaerber@suse.de> writes: >>>> >>>>> Am 02.10.2014 um 15:21 schrieb Stefan Hajnoczi: >>>>>> On Wed, Oct 01, 2014 at 02:33:47PM +0200, Markus Armbruster wrote: >>>>>>> Markus Armbruster <armbru@redhat.com> writes: >>>>>>> >>>>>>> Actually, "property names are arbitrary strings" has recently become >>>>>>> untrue: commit 339659 "qom: Add automatic arrayification to >>>>>>> object_property_add()" changed names to have embedded syntax. A >>>>>>> property name ending with "[*]" makes the property special. Normal >>>>>>> properties can be added just once, and an attempt to add it again is an >>>>>>> error. These special properties, however, can be added any number of >>>>>>> times, and each time the name "[*]" is silently rewritten to "[N]", >>>>>>> where N counts from 0. [...] >>>>>>> Is mangling array-ness into the name really a good idea? Isn't this >>>>>>> type matter, not name matter? >>>>>> >>>>>> I agree. It's nasty to hack the array selector into the name and will >>>>>> probably cause us pain down the line. >>>> >>>> Andreas? >>> >>> -> Paolo ;) >> >> Paolo? > > Uhm, I had written an answer but Thunderbird ate it. Oh well. > > I think foo[*] is not really a matter of typing, but a matter of > grouping similar children. It does not really matter if foo[1] is > deleted while foo[2] still exists, and it does not really matter if the > next object created will be foo[1] or foo[3]. > > While we do not have any example, QOM could support true array > properties, e.g. with type intList. > > Also note that while foo[*] was a generalization of MemoryRegion code, > the same idea has also existed forever for devices. > /machine/peripheral_anon and /machine/unattached do not currently use > automatic array-ification via foo[*], but they could. Let me try to paraphrase / further explain your view, to make sure I got it. "Automatic arrayification" isn't about array-valued properties, it's a convenience feature for creating a bunch of properties with a common type, accessors and so forth, named in a peculiar way: "foo[0]", "foo[1]", ... The feature saves the caller the trouble of generating the names. That's all there is to it. Once created, QOM assumes no particular relation between the properties. Weird: if you create a "foo[2]", then three "foo[*]", the three become "foo[0]", "foo[1]", "foo[3]". Correct so far? If yes, then I retract my "isn't this type matter" remark: it isn't, it's just a fancy way to generate names. However, I now have a different one: should we really bake fancy ways to generate names into object_property_add()? Wouldn't having a separate name generator be cleaner? And yet another one: the special treatment of names ending with "[*]" appears to be entirely undocumented! Review fail. >>> If you just want to call id_wellformed() for -object / object-add, I >>> won't object to restricting it beyond the necessary, but it'll lead to >>> two places doing validity checks for QOM. >> >> id_wellformed() is *already* called for -object and HMP object-add. >> That's exactly my point! >> >> Please reread my explanation, and if it's insufficient, ask for >> clarification. >> >> Subject: IDs in QOM (was: [PATCH] util: Emancipate id_wellformed() >> from QemuOpts) >> Date: Wed, 01 Oct 2014 14:33:47 +0200 >> Message-ID: <87iok46kb8.fsf@blackfin.pond.sub.org> > > I think it is okay to keep calling id_wellformed(), since it covers the > QOM-specific constraint that slashes must be outlawed. It just makes > the children of /objects a bit more restricted in their naming, but that > is harmless because object_del can only delete objects created with > object_add. 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? Are -object and object_add the only ways to create children of /objects? Hmm, I'm afraid my working definition of the loser one is incorrect. It's actually "anything but '\0' and '/' not ending with '[*]'.
Il 07/10/2014 10:01, Markus Armbruster ha scritto: > "Automatic arrayification" isn't about array-valued properties, it's a > convenience feature for creating a bunch of properties with a common > type, accessors and so forth, named in a peculiar way: "foo[0]", > "foo[1]", ... > > The feature saves the caller the trouble of generating the names. > That's all there is to it. > > Once created, QOM assumes no particular relation between the properties. > > Weird: if you create a "foo[2]", then three "foo[*]", the three become > "foo[0]", "foo[1]", "foo[3]". > > Correct so far? > > If yes, then I retract my "isn't this type matter" remark: it isn't, > it's just a fancy way to generate names. Exactly. Regarding the "weird part", it is really a case of "if it hurts, do not do it". :) For example, most memory regions are created at or before realize time, and live until the parent device is hot-unplugged or QEMU exits. Unattached devices are created statically at or before machine creation, and live until they are hot-unplugged or QEMU exits. > However, I now have a different one: should we really bake fancy ways to > generate names into object_property_add()? > > Wouldn't having a separate name generator be cleaner? 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 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. > Are -object and object_add the only ways to create children of /objects? Yes (of course you could do that programmatically in C, but I don't see why you should/would do that). > Hmm, I'm afraid my working definition of the loser one is incorrect. > It's actually "anything but '\0' and '/' not ending with '[*]'. True. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 07/10/2014 10:01, Markus Armbruster ha scritto: >> "Automatic arrayification" isn't about array-valued properties, it's a >> convenience feature for creating a bunch of properties with a common >> type, accessors and so forth, named in a peculiar way: "foo[0]", >> "foo[1]", ... >> >> The feature saves the caller the trouble of generating the names. >> That's all there is to it. >> >> Once created, QOM assumes no particular relation between the properties. >> >> Weird: if you create a "foo[2]", then three "foo[*]", the three become >> "foo[0]", "foo[1]", "foo[3]". >> >> Correct so far? >> >> If yes, then I retract my "isn't this type matter" remark: it isn't, >> it's just a fancy way to generate names. > > Exactly. Regarding the "weird part", it is really a case of "if it > hurts, do not do it". :) For example, most memory regions are created > at or before realize time, and live until the parent device is > hot-unplugged or QEMU exits. Unattached devices are created statically > at or before machine creation, and live until they are hot-unplugged or > QEMU exits. > >> However, I now have a different one: should we really bake fancy ways to >> generate names into object_property_add()? >> >> Wouldn't having a separate name generator be cleaner? > > 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() >> 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? 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). >> Are -object and object_add the only ways to create children of /objects? > > Yes (of course you could do that programmatically in C, but I don't see > why you should/would do that). > >> Hmm, I'm afraid my working definition of the loser one is incorrect. >> It's actually "anything but '\0' and '/' not ending with '[*]'. > > True. And ugly :) So the "automatic arrayification" convenience feature added a property name restriction. What makes us sure this is the last time we add name restrictions?
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. :) > > > 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. netdev-add and device-add have been doing (2) because they use QemuOpts under the hood. blockdev-add has been consciously doing (2) for node-name. chardev-add has been doing (1), and I'd argue that this is a bug in chardev-add. 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. > 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? Paolo
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
Am 07.10.2014 um 17:14 hat Paolo Bonzini geschrieben: > 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. :) > > > > > 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. > > netdev-add and device-add have been doing (2) because they use QemuOpts > under the hood. > > blockdev-add has been consciously doing (2) for node-name. > > chardev-add has been doing (1), and I'd argue that this is a bug in > chardev-add. Is there any way to add netdevs/chardevs/devices in a non-QemuOpts way? If not, (1) and (2) are the same thing, and the checks are always applied. I think always checking for the same allowed set of characters is the only sane way to do things. Otherwise you end up with names that can be used in one place, but not in another, e.g. you can create an object, but then not delete it again using HMP. Or you can use a name for hotplug, but not on the initial startup when the command line configures the device. That's definitely something to be avoided. > > 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? They were possible in QMP object-add, weren't they? Kevin
Il 07/10/2014 20:39, Markus Armbruster ha scritto: >>>> >> > 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. > Nevermind, I've consistently swapped (1) and (2). Paolo
Il 07/10/2014 20:41, Kevin Wolf ha scritto: > Is there any way to add netdevs/chardevs/devices in a non-QemuOpts way? For chardevs, yes. > I think always checking for the same allowed set of characters is the > only sane way to do things. Otherwise you end up with names that can be > used in one place, but not in another, e.g. you can create an object, > but then not delete it again using HMP. Note that deletion does not use QemuOpts, so device_del and netdev_del could delete things with funny names. > Or you can use a name for > hotplug, but not on the initial startup when the command line > configures the device. That's definitely something to be avoided. I agree. >>> > > 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? > They were possible in QMP object-add, weren't they? Yes. I think we agree that we're going to change that. Paolo
diff --git a/block.c b/block.c index c5a251c..d3aebeb 100644 --- a/block.c +++ b/block.c @@ -335,18 +335,13 @@ void bdrv_register(BlockDriver *bdrv) QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list); } -static bool bdrv_is_valid_name(const char *name) -{ - return qemu_opts_id_wellformed(name); -} - /* create a new block device (by default it is empty) */ BlockDriverState *bdrv_new(const char *device_name, Error **errp) { BlockDriverState *bs; int i; - if (*device_name && !bdrv_is_valid_name(device_name)) { + if (*device_name && !id_wellformed(device_name)) { error_setg(errp, "Invalid device name"); return NULL; } @@ -874,7 +869,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs, } /* Check for empty string or invalid characters */ - if (!bdrv_is_valid_name(node_name)) { + if (!id_wellformed(node_name)) { error_setg(errp, "Invalid node name"); return; } diff --git a/include/qemu-common.h b/include/qemu-common.h index dcb57ab..b87e9c2 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -190,6 +190,9 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end, /* used to print char* safely */ #define STR_OR_NULL(str) ((str) ? (str) : "null") +/* id.c */ +bool id_wellformed(const char *id); + /* path.c */ void init_paths(const char *prefix); const char *path(const char *pathname); diff --git a/include/qemu/option.h b/include/qemu/option.h index 945347c..59bea75 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -103,7 +103,6 @@ typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaq int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, int abort_on_failure); -int qemu_opts_id_wellformed(const char *id); QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id); QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists, Error **errp); diff --git a/util/Makefile.objs b/util/Makefile.objs index cb8862b..93007e2 100644 --- a/util/Makefile.objs +++ b/util/Makefile.objs @@ -8,6 +8,7 @@ util-obj-y += fifo8.o util-obj-y += acl.o util-obj-y += error.o qemu-error.o util-obj-$(CONFIG_POSIX) += compatfd.o +util-obj-y += id.o util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o uri.o notify.o util-obj-y += qemu-option.o qemu-progress.o util-obj-y += hexdump.o diff --git a/util/id.c b/util/id.c new file mode 100644 index 0000000..0f2c42a --- /dev/null +++ b/util/id.c @@ -0,0 +1,28 @@ +/* + * Dealing with identifiers + * + * Copyright (C) 2014 Red Hat, Inc. + * + * Authors: + * Markus Armbruster <armbru@redhat.com>, + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 + * or later. See the COPYING.LIB file in the top-level directory. + */ + +#include "qemu-common.h" + +bool id_wellformed(const char *id) +{ + int i; + + if (!qemu_isalpha(id[0])) { + return 0; + } + for (i = 1; id[i]; i++) { + if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) { + return false; + } + } + return true; +} diff --git a/util/qemu-option.c b/util/qemu-option.c index 0cf9960..5d10695 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -641,28 +641,13 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id) return NULL; } -int qemu_opts_id_wellformed(const char *id) -{ - int i; - - if (!qemu_isalpha(id[0])) { - return 0; - } - for (i = 1; id[i]; i++) { - if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) { - return 0; - } - } - return 1; -} - QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists, Error **errp) { QemuOpts *opts = NULL; if (id) { - if (!qemu_opts_id_wellformed(id)) { + if (!id_wellformed(id)) { error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier"); #if 0 /* conversion from qerror_report() to error_set() broke this: */ error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");