| Message ID | 20180208192328.16550-10-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-02-08 20:23, Kevin Wolf wrote: > A few block drivers will need to rename .bdrv_create options for their > QAPIfication, so let's have a helper function for that. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/qapi/qmp/qdict.h | 6 ++++++ > qobject/qdict.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) Hmmm, looks OK, but I wonder whether this is going to be any more efficient than if we simply had a qdict_rename_key() function that you call multiple times. ("efficient" both in terms of code readability and runtime efficiency) So, I'll give you a Reviewed-by: Max Reitz <mreitz@redhat.com> and let you decide. > > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h > index fc218e7be6..862441b9d3 100644 > --- a/include/qapi/qmp/qdict.h > +++ b/include/qapi/qmp/qdict.h > @@ -90,4 +90,10 @@ QObject *qdict_crumple(const QDict *src, Error **errp); > > void qdict_join(QDict *dest, QDict *src, bool overwrite); > > +typedef struct QDictRenames { > + const char *from; > + const char *to; > +} QDictRenames; > +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp); > + > #endif /* QDICT_H */ > diff --git a/qobject/qdict.c b/qobject/qdict.c > index e8f15f1132..07ae9489a7 100644 > --- a/qobject/qdict.c > +++ b/qobject/qdict.c > @@ -1051,3 +1051,33 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite) > entry = next; > } > } > + > +/** > + * qdict_rename_keys(): Rename keys in qdict according to the replacements > + * specified in the array renames. The array must be terminated by an entry > + * with from = NULL. > + * > + * Returns true for success, false in error cases. > + */ > +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) > +{ > + QObject *qobj; > + > + while (renames->from) { > + if (qdict_haskey(qdict, renames->from)) { > + if (qdict_haskey(qdict, renames->to)) { > + error_setg(errp, "'%s' and its alias '%s' can't be used at the " > + "same time", renames->to, renames->from); > + return false; > + } > + > + qobj = qdict_get(qdict, renames->from); > + qobject_incref(qobj); > + qdict_put_obj(qdict, renames->to, qobj); > + qdict_del(qdict, renames->from); > + } > + > + renames++; > + } > + return true; > +} >
On 2018-02-09 19:18, Max Reitz wrote: > On 2018-02-08 20:23, Kevin Wolf wrote: >> A few block drivers will need to rename .bdrv_create options for their >> QAPIfication, so let's have a helper function for that. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> include/qapi/qmp/qdict.h | 6 ++++++ >> qobject/qdict.c | 30 ++++++++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+) > > Hmmm, looks OK, but I wonder whether this is going to be any more > efficient than if we simply had a qdict_rename_key() function that you > call multiple times. > > ("efficient" both in terms of code readability and runtime efficiency) > > So, I'll give you a > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > and let you decide. Ah, I see, error handling is much easier this way, that's right. OK, R-b stands then. Max
On 02/08/2018 01:23 PM, Kevin Wolf wrote: > A few block drivers will need to rename .bdrv_create options for their > QAPIfication, so let's have a helper function for that. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/qapi/qmp/qdict.h | 6 ++++++ > qobject/qdict.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) Again, unit test coverage? > +/** > + * qdict_rename_keys(): Rename keys in qdict according to the replacements > + * specified in the array renames. The array must be terminated by an entry > + * with from = NULL. > + * > + * Returns true for success, false in error cases. > + */ > +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) > +{ > + QObject *qobj; > + > + while (renames->from) { > + if (qdict_haskey(qdict, renames->from)) { > + if (qdict_haskey(qdict, renames->to)) { Depending on how efficient qdict_haskey() is, this is a lot of looping. Good thing our lists aren't so large that we'd notice the effects of cubic scaling (I count O(m*n*n), where m is renames length, and n is worst-case performance of qdict_haskey). Definitely not worth the effort of more code to try and have a more efficient algorithm in large-term scaling but which hurts performance with increased overhead on small lists. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index fc218e7be6..862441b9d3 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -90,4 +90,10 @@ QObject *qdict_crumple(const QDict *src, Error **errp); void qdict_join(QDict *dest, QDict *src, bool overwrite); +typedef struct QDictRenames { + const char *from; + const char *to; +} QDictRenames; +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp); + #endif /* QDICT_H */ diff --git a/qobject/qdict.c b/qobject/qdict.c index e8f15f1132..07ae9489a7 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -1051,3 +1051,33 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite) entry = next; } } + +/** + * qdict_rename_keys(): Rename keys in qdict according to the replacements + * specified in the array renames. The array must be terminated by an entry + * with from = NULL. + * + * Returns true for success, false in error cases. + */ +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) +{ + QObject *qobj; + + while (renames->from) { + if (qdict_haskey(qdict, renames->from)) { + if (qdict_haskey(qdict, renames->to)) { + error_setg(errp, "'%s' and its alias '%s' can't be used at the " + "same time", renames->to, renames->from); + return false; + } + + qobj = qdict_get(qdict, renames->from); + qobject_incref(qobj); + qdict_put_obj(qdict, renames->to, qobj); + qdict_del(qdict, renames->from); + } + + renames++; + } + return true; +}
A few block drivers will need to rename .bdrv_create options for their QAPIfication, so let's have a helper function for that. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qapi/qmp/qdict.h | 6 ++++++ qobject/qdict.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)