| Message ID | 20171110221329.24176-2-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/10/2017 04:13 PM, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/qapi/qmp/qdict.h | 1 + > qobject/qdict.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > Reviewed-by: Eric Blake <eblake@redhat.com>
On Fri 10 Nov 2017 11:13:27 PM CET, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/qapi/qmp/qdict.h | 1 + > qobject/qdict.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h > index fc218e7be6..c65ebfc748 100644 > --- a/include/qapi/qmp/qdict.h > +++ b/include/qapi/qmp/qdict.h > @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, > int64_t def_value); > bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); > const char *qdict_get_try_str(const QDict *qdict, const char *key); > +bool qdict_is_qnull(const QDict *qdict, const char *key); I found this name a bit confusing, how about something like qdict_entry_is_qnull() ? I'm fine if you want to keep it like this though, Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 2017-11-14 15:07, Alberto Garcia wrote: > On Fri 10 Nov 2017 11:13:27 PM CET, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> include/qapi/qmp/qdict.h | 1 + >> qobject/qdict.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h >> index fc218e7be6..c65ebfc748 100644 >> --- a/include/qapi/qmp/qdict.h >> +++ b/include/qapi/qmp/qdict.h >> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, >> int64_t def_value); >> bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); >> const char *qdict_get_try_str(const QDict *qdict, const char *key); >> +bool qdict_is_qnull(const QDict *qdict, const char *key); > > I found this name a bit confusing, how about something like > qdict_entry_is_qnull() ? Yes, that sounds better. Thanks! Max > I'm fine if you want to keep it like this though, > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > Berto >
Max Reitz <mreitz@redhat.com> writes: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/qapi/qmp/qdict.h | 1 + > qobject/qdict.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h > index fc218e7be6..c65ebfc748 100644 > --- a/include/qapi/qmp/qdict.h > +++ b/include/qapi/qmp/qdict.h > @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, > int64_t def_value); > bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); > const char *qdict_get_try_str(const QDict *qdict, const char *key); > +bool qdict_is_qnull(const QDict *qdict, const char *key); > > void qdict_copy_default(QDict *dst, QDict *src, const char *key); > void qdict_set_default_str(QDict *dst, const char *key, const char *val); > diff --git a/qobject/qdict.c b/qobject/qdict.c > index e8f15f1132..a032ea629a 100644 > --- a/qobject/qdict.c > +++ b/qobject/qdict.c > @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key) > } > > /** > + * qdict_is_qnull(): Return true if the value for 'key' is QNull > + */ > +bool qdict_is_qnull(const QDict *qdict, const char *key) > +{ > + QObject *value = qdict_get(qdict, key); > + > + return value && value->type == QTYPE_QNULL; > +} > + > +/** > * qdict_iter(): Iterate over all the dictionary's stored values. > * > * This function allows the user to provide an iterator, which will be As far as I can tell, the new helper function is going to be used just once, by bdrv_open_inherit() in PATCH 2: qdict_is_qnull(options, "backing") I dislike abstracting from just one concrete instance. Perhaps a more general helper could be more generally useful. Something like: qobject_is(qdict_get(options, "backing", QTYPE_QNULL)) There are numerous instances of !obj || qobject_type(obj) == T in the tree, which could then be replaced by qobject_is(obj, T) An alternative helper: macro qobject_dynamic_cast(obj, T) that returns (T *)obj if obj is a T, else null. Leads to something like qobject_dynamic_cast(qdict_get(options, "backing", QNull))
On 2017-11-14 15:57, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> include/qapi/qmp/qdict.h | 1 + >> qobject/qdict.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h >> index fc218e7be6..c65ebfc748 100644 >> --- a/include/qapi/qmp/qdict.h >> +++ b/include/qapi/qmp/qdict.h >> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, >> int64_t def_value); >> bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); >> const char *qdict_get_try_str(const QDict *qdict, const char *key); >> +bool qdict_is_qnull(const QDict *qdict, const char *key); >> >> void qdict_copy_default(QDict *dst, QDict *src, const char *key); >> void qdict_set_default_str(QDict *dst, const char *key, const char *val); >> diff --git a/qobject/qdict.c b/qobject/qdict.c >> index e8f15f1132..a032ea629a 100644 >> --- a/qobject/qdict.c >> +++ b/qobject/qdict.c >> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key) >> } >> >> /** >> + * qdict_is_qnull(): Return true if the value for 'key' is QNull >> + */ >> +bool qdict_is_qnull(const QDict *qdict, const char *key) >> +{ >> + QObject *value = qdict_get(qdict, key); >> + >> + return value && value->type == QTYPE_QNULL; >> +} >> + >> +/** >> * qdict_iter(): Iterate over all the dictionary's stored values. >> * >> * This function allows the user to provide an iterator, which will be > > As far as I can tell, the new helper function is going to be used just > once, by bdrv_open_inherit() in PATCH 2: > > qdict_is_qnull(options, "backing") > > I dislike abstracting from just one concrete instance. > > Perhaps a more general helper could be more generally useful. Something > like: > > qobject_is(qdict_get(options, "backing", QTYPE_QNULL)) > > There are numerous instances of > > !obj || qobject_type(obj) == T > > in the tree, which could then be replaced by > > qobject_is(obj, T) > > An alternative helper: macro qobject_dynamic_cast(obj, T) that returns > (T *)obj if obj is a T, else null. Leads to something like > > qobject_dynamic_cast(qdict_get(options, "backing", QNull)) If you think that's good, then that's good -- you know the QAPI code better then me, after all. To explain myself: I thought it would be the natural extension of the qdict_get_try_*() functions for the QNull type. Max
Max Reitz <mreitz@redhat.com> writes: > On 2017-11-14 15:57, Markus Armbruster wrote: >> Max Reitz <mreitz@redhat.com> writes: >> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> include/qapi/qmp/qdict.h | 1 + >>> qobject/qdict.c | 10 ++++++++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h >>> index fc218e7be6..c65ebfc748 100644 >>> --- a/include/qapi/qmp/qdict.h >>> +++ b/include/qapi/qmp/qdict.h >>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, >>> int64_t def_value); >>> bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); >>> const char *qdict_get_try_str(const QDict *qdict, const char *key); >>> +bool qdict_is_qnull(const QDict *qdict, const char *key); >>> >>> void qdict_copy_default(QDict *dst, QDict *src, const char *key); >>> void qdict_set_default_str(QDict *dst, const char *key, const char *val); >>> diff --git a/qobject/qdict.c b/qobject/qdict.c >>> index e8f15f1132..a032ea629a 100644 >>> --- a/qobject/qdict.c >>> +++ b/qobject/qdict.c >>> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key) >>> } >>> >>> /** >>> + * qdict_is_qnull(): Return true if the value for 'key' is QNull >>> + */ >>> +bool qdict_is_qnull(const QDict *qdict, const char *key) >>> +{ >>> + QObject *value = qdict_get(qdict, key); >>> + >>> + return value && value->type == QTYPE_QNULL; >>> +} >>> + >>> +/** >>> * qdict_iter(): Iterate over all the dictionary's stored values. >>> * >>> * This function allows the user to provide an iterator, which will be >> >> As far as I can tell, the new helper function is going to be used just >> once, by bdrv_open_inherit() in PATCH 2: >> >> qdict_is_qnull(options, "backing") >> >> I dislike abstracting from just one concrete instance. >> >> Perhaps a more general helper could be more generally useful. Something >> like: >> >> qobject_is(qdict_get(options, "backing", QTYPE_QNULL)) >> >> There are numerous instances of >> >> !obj || qobject_type(obj) == T >> >> in the tree, which could then be replaced by >> >> qobject_is(obj, T) >> >> An alternative helper: macro qobject_dynamic_cast(obj, T) that returns >> (T *)obj if obj is a T, else null. Leads to something like >> >> qobject_dynamic_cast(qdict_get(options, "backing", QNull)) > > If you think that's good, then that's good -- you know the QAPI code > better then me, after all. I'll play with it today. > To explain myself: I thought it would be the natural extension of the > qdict_get_try_*() functions for the QNull type. I see now. The name qdict_get_try_null() would make it obvious, but what to return on success...
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index fc218e7be6..c65ebfc748 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key, int64_t def_value); bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value); const char *qdict_get_try_str(const QDict *qdict, const char *key); +bool qdict_is_qnull(const QDict *qdict, const char *key); void qdict_copy_default(QDict *dst, QDict *src, const char *key); void qdict_set_default_str(QDict *dst, const char *key, const char *val); diff --git a/qobject/qdict.c b/qobject/qdict.c index e8f15f1132..a032ea629a 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key) } /** + * qdict_is_qnull(): Return true if the value for 'key' is QNull + */ +bool qdict_is_qnull(const QDict *qdict, const char *key) +{ + QObject *value = qdict_get(qdict, key); + + return value && value->type == QTYPE_QNULL; +} + +/** * qdict_iter(): Iterate over all the dictionary's stored values. * * This function allows the user to provide an iterator, which will be
Signed-off-by: Max Reitz <mreitz@redhat.com> --- include/qapi/qmp/qdict.h | 1 + qobject/qdict.c | 10 ++++++++++ 2 files changed, 11 insertions(+)