| Message ID | 1324036918-2405-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/16/2011 06:01 AM, Paolo Bonzini wrote: > QAPI currently cannot deal with no object pushed to the stack, > and dereferences a NULL pointer. This is visible with > > qom-get path=/i440fx/piix3 property=romfile > > after static non-string properties are introduced. I'm a bit confused about what's happening here. What's the significance of non-string properties? Regards, Anthony Liguori > > Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> > --- > qapi/qmp-output-visitor.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index f76d015..29575da 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -65,13 +65,13 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) > static QObject *qmp_output_first(QmpOutputVisitor *qov) > { > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > - return e->value; > + return e ? e->value : NULL; > } > > static QObject *qmp_output_last(QmpOutputVisitor *qov) > { > QStackEntry *e = QTAILQ_FIRST(&qov->stack); > - return e->value; > + return e ? e->value : NULL; > } > > static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
On 12/16/2011 02:55 PM, Anthony Liguori wrote: >> This is visible with >> >> qom-get path=/i440fx/piix3 property=romfile >> >> after static non-string properties are introduced. > > I'm a bit confused about what's happening here. What's the significance > of non-string properties? Should have been "static non-legacy properties". When you don't have a value for a property, legacy properties are visited as "<null>", while the new static properties do not pass anything to the visitor. I stole this from qdev_property_get_str: value = prop->get(dev, errp); if (value) { visit_type_str(v, &value, name, errp); g_free(value); } Paolo
On 12/16/2011 08:00 AM, Paolo Bonzini wrote: > On 12/16/2011 02:55 PM, Anthony Liguori wrote: >>> This is visible with >>> >>> qom-get path=/i440fx/piix3 property=romfile >>> >>> after static non-string properties are introduced. >> >> I'm a bit confused about what's happening here. What's the significance >> of non-string properties? > > Should have been "static non-legacy properties". > > When you don't have a value for a property, legacy properties are visited as > "<null>", while the new static properties do not pass anything to the visitor. > > I stole this from qdev_property_get_str: > > value = prop->get(dev, errp); > if (value) { > visit_type_str(v, &value, name, errp); > g_free(value); > } I should more clearly document it. NULL would be only return if errp was set. I just didn't want to introduce a local_err in order to be able to check whether the function failed. If a property function does not set the Error pointer, it must visit something. Regards, Anthony Liguori > > Paolo >
On 12/16/2011 03:10 PM, Anthony Liguori wrote: > If a property function does not set the Error pointer, it must visit > something. Hmm, then we have to introduce NULL into QJson and visitors. Paolo
On 12/16/2011 08:22 AM, Paolo Bonzini wrote: > On 12/16/2011 03:10 PM, Anthony Liguori wrote: >> If a property function does not set the Error pointer, it must visit >> something. > > Hmm, then we have to introduce NULL into QJson and visitors. Visitors assume that strings aren't nullable (which is actually true in JSON and in QString). I also think that string properties shouldn't be nullable. Regards, Anthony Liguori > > Paolo >
On 12/16/2011 03:46 PM, Anthony Liguori wrote: >> >> Hmm, then we have to introduce NULL into QJson and visitors. > > Visitors assume that strings aren't nullable (which is actually true in > JSON and in QString). > > I also think that string properties shouldn't be nullable. Unfortunately, qdev string properties are nullable and there might well be examples in which empty and NULL are different. I'd rather not risk. But JSON actually has NULL, so not all is lost. I can introduce a nullable_str type in visitors (restricting structs and arrays should be fine, though). Paolo
On 12/16/2011 08:49 AM, Paolo Bonzini wrote: > On 12/16/2011 03:46 PM, Anthony Liguori wrote: >>> >>> Hmm, then we have to introduce NULL into QJson and visitors. >> >> Visitors assume that strings aren't nullable (which is actually true in >> JSON and in QString). >> >> I also think that string properties shouldn't be nullable. > > Unfortunately, qdev string properties are nullable and there might well be > examples in which empty and NULL are different. I'd rather not risk. > > But JSON actually has NULL, so not all is lost. I can introduce a nullable_str > type in visitors (restricting structs and arrays should be fine, though). I'd really prefer to stick to non-nullable strings as there is no obvious way to specify NULL in command line options. What are the uses of null in qdev string properties? I know you can't set a string to null since parse() doesn't have a null syntax. So we're really just talking about an uninitialized state, right? Regards, Anthony Liguori > > Paolo >
On 12/16/2011 03:56 PM, Anthony Liguori wrote: > I'd really prefer to stick to non-nullable strings as there is no > obvious way to specify NULL in command line options. We can leave it as the default. A property with a non-null default is implicitly not nullable, which actually makes some sense. We can model this in get_string/set_string too. > What are the uses of null in qdev string properties? I know you can't > set a string to null since parse() doesn't have a null syntax. So we're > really just talking about an uninitialized state, right? Yes. No ROM BAR is an example of a NULL string property. Paolo
On 12/16/2011 09:03 AM, Paolo Bonzini wrote: > On 12/16/2011 03:56 PM, Anthony Liguori wrote: >> I'd really prefer to stick to non-nullable strings as there is no >> obvious way to specify NULL in command line options. > > We can leave it as the default. A property with a non-null default is implicitly > not nullable, which actually makes some sense. We can model this in > get_string/set_string too. > >> What are the uses of null in qdev string properties? I know you can't >> set a string to null since parse() doesn't have a null syntax. So we're >> really just talking about an uninitialized state, right? > > Yes. No ROM BAR is an example of a NULL string property. So it's really filenames and backend names, right? Can we just treat empty strings like NULL? I think all of the various bits handles both the same way. Regards, Anthony Liguori > > Paolo >
On 12/16/2011 04:05 PM, Anthony Liguori wrote: >> >> >>> What are the uses of null in qdev string properties? I know you can't >>> set a string to null since parse() doesn't have a null syntax. So we're >>> really just talking about an uninitialized state, right? >> >> Yes. No ROM BAR is an example of a NULL string property. > > So it's really filenames and backend names, right? Can we just treat > empty strings like NULL? I think all of the various bits handles both > the same way. The only case I can think of when it differs is serial numbers. An empty serial number is different from no serial number (which means do not expose one at all). However, DEFINE_PROP_STRING does not allow setting a default, and some device models rely on a NULL value as a default. I see two ways out: 1) add nullable strings; 2) merge without this patch, knowing qom-get is kind-of broken, and proceed adding a default to all DEFINE_PROP_STRING; this would be a merge nightmare for you. 3) merge without this patch, knowing it's kind-of broken, and later decide what to do. Let's do (3), and add nullable strings if the discussion stalls. Paolo
On 12/16/2011 09:13 AM, Paolo Bonzini wrote: > On 12/16/2011 04:05 PM, Anthony Liguori wrote: >>> >>> >>>> What are the uses of null in qdev string properties? I know you can't >>>> set a string to null since parse() doesn't have a null syntax. So we're >>>> really just talking about an uninitialized state, right? >>> >>> Yes. No ROM BAR is an example of a NULL string property. >> >> So it's really filenames and backend names, right? Can we just treat >> empty strings like NULL? I think all of the various bits handles both >> the same way. > > The only case I can think of when it differs is serial numbers. An empty serial > number is different from no serial number (which means do not expose one at all). > > However, DEFINE_PROP_STRING does not allow setting a default, and some device > models rely on a NULL value as a default. I see two ways out: 1) add nullable > strings; 2) merge without this patch, knowing qom-get is kind-of broken, and > proceed adding a default to all DEFINE_PROP_STRING; this would be a merge > nightmare for you. I thrive on pain it seems :-) > 3) merge without this patch, knowing it's kind-of broken, and > later decide what to do. Ok. I think nullable strings are not a good idea simply because it means that a property can have a state that cannot be set. Long term, I want to be able to do something like dump the current device graph to a config file, and then use that config file to recreate the same machine again. A nullable property without a null representation would not allow this. Regards, Anthony Liguori > Let's do (3), and add nullable strings if the discussion stalls. > > Paolo >
On 12/16/2011 04:23 PM, Anthony Liguori wrote: > Ok. I think nullable strings are not a good idea simply because it > means that a property can have a state that cannot be set. How is this different from NULL links? (Honest, not trick question :)). > Long term, I want to be able to do something like dump the current > device graph to a config file, and then use that config file to recreate > the same machine again. A nullable property without a null > representation would not allow this. JSON null is such a representation. Paolo
On 12/16/2011 09:42 AM, Paolo Bonzini wrote: > On 12/16/2011 04:23 PM, Anthony Liguori wrote: >> Ok. I think nullable strings are not a good idea simply because it >> means that a property can have a state that cannot be set. > > How is this different from NULL links? (Honest, not trick question :)). An empty string == NULL for links. If a pointer is NULL, an empty string is returned. So get/set is full symmetric. >> Long term, I want to be able to do something like dump the current >> device graph to a config file, and then use that config file to recreate >> the same machine again. A nullable property without a null >> representation would not allow this. > > JSON null is such a representation. For JSON, but it doesn't map to a config file easily nor does it map to command line syntax well. Regards, Anthony Liguori > > Paolo >
On 12/16/2011 04:54 PM, Anthony Liguori wrote: > On 12/16/2011 09:42 AM, Paolo Bonzini wrote: >> On 12/16/2011 04:23 PM, Anthony Liguori wrote: >>> Ok. I think nullable strings are not a good idea simply because it >>> means that a property can have a state that cannot be set. >> >> How is this different from NULL links? (Honest, not trick question :)). > > An empty string == NULL for links. > > If a pointer is NULL, an empty string is returned. So get/set is full > symmetric. Ok, so we can do the same for "pointer" properties. I'm just a bit worried about serial numbers, where "-device virtio-blk-pci,...,serial=" is different from no serial at all. One shows "" in info qtree, the other shows <null>. > For JSON, but it doesn't map to a config file easily nor does it map to > command line syntax well. You can force nullable properties to have a null default (which is now the case in qdev), but I agree that's not too nice. Paolo
Hi, >> What are the uses of null in qdev string properties? I know you can't >> set a string to null since parse() doesn't have a null syntax. So we're >> really just talking about an uninitialized state, right? > > Yes. No ROM BAR is an example of a NULL string property. Both NULL and zero-length string are valid for "empty" though, so you can use -device e1000,romfile=,mac=whatever to boot without pxe rom. Which underlines anthonys point ;) cheers, Gerd
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index f76d015..29575da 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -65,13 +65,13 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) static QObject *qmp_output_first(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); - return e->value; + return e ? e->value : NULL; } static QObject *qmp_output_last(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_FIRST(&qov->stack); - return e->value; + return e ? e->value : NULL; } static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
QAPI currently cannot deal with no object pushed to the stack, and dereferences a NULL pointer. This is visible with qom-get path=/i440fx/piix3 property=romfile after static non-string properties are introduced. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- qapi/qmp-output-visitor.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)