| Message ID | 1505375436-28439-3-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | QMP: out-of-band (OOB) execution support | expand |
On 09/14/2017 02:50 AM, Peter Xu wrote: > Then I can get NULL rather than crash when calling things like: > > qstring_get_str(qobject_to_qstring(object)); > > when key does not exist. Right now, qdict_get_str() is documented as: * This function assumes that 'key' exists and it stores a * QString object. Your code changes that, by making it now return NULL instead of crashing on what used to be usage in violation of the contract; compared to qdict_get_try_str() which gracefully returns NULL, but which could use your new semantics for doing so in fewer lines of code. I'm not necessarily opposed to the change, but I worry that it has subtle ramifications that we haven't thought about, as well as consistency with the rest of the QObject APIs. It may be better to just introduce qstring_get_try_str(), which gracefully handles NULL input, and leave the existing function alone; and if you do introduce a new helper, it may be worth converting existing clients (perhaps with help from Coccinelle) to take advantage of the helper. > +++ b/qobject/qstring.c > @@ -125,7 +125,7 @@ QString *qobject_to_qstring(const QObject *obj) > */ > const char *qstring_get_str(const QString *qstring) > { > - return qstring->string; > + return qstring ? qstring->string : NULL; > } > > /** >
On Tue, Sep 19, 2017 at 03:48:35PM -0500, Eric Blake wrote: > On 09/14/2017 02:50 AM, Peter Xu wrote: > > Then I can get NULL rather than crash when calling things like: > > > > qstring_get_str(qobject_to_qstring(object)); > > > > when key does not exist. > > Right now, qdict_get_str() is documented as: > > * This function assumes that 'key' exists and it stores a > * QString object. > > Your code changes that, by making it now return NULL instead of crashing > on what used to be usage in violation of the contract; compared to > qdict_get_try_str() which gracefully returns NULL, but which could use > your new semantics for doing so in fewer lines of code. > > I'm not necessarily opposed to the change, but I worry that it has > subtle ramifications that we haven't thought about, as well as > consistency with the rest of the QObject APIs. It may be better to just > introduce qstring_get_try_str(), which gracefully handles NULL input, > and leave the existing function alone; and if you do introduce a new > helper, it may be worth converting existing clients (perhaps with help > from Coccinelle) to take advantage of the helper. I'll take your suggestion. Though I didn't see much existing codes that can use the new qstring_get_try_str(). One I found is object_property_get_str(). I can add one more patch to convert its usage. Thanks, > > > +++ b/qobject/qstring.c > > @@ -125,7 +125,7 @@ QString *qobject_to_qstring(const QObject *obj) > > */ > > const char *qstring_get_str(const QString *qstring) > > { > > - return qstring->string; > > + return qstring ? qstring->string : NULL; > > } > > > > /** > > > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
diff --git a/qobject/qstring.c b/qobject/qstring.c index 5da7b5f..c499fec 100644 --- a/qobject/qstring.c +++ b/qobject/qstring.c @@ -125,7 +125,7 @@ QString *qobject_to_qstring(const QObject *obj) */ const char *qstring_get_str(const QString *qstring) { - return qstring->string; + return qstring ? qstring->string : NULL; } /**
Then I can get NULL rather than crash when calling things like: qstring_get_str(qobject_to_qstring(object)); when key does not exist. CC: Markus Armbruster <armbru@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- qobject/qstring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)