| Message ID | 20180612125821.4229-13-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Configuration fixes and rbd authentication | expand |
Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > When you mix scalar and non-scalar keys, whether you get an "already > set as scalar" or an "already set as dict" error depends on qdict > iteration order. Neither message makes much sense. Replace by > ""Cannot mix scalar and non-scalar keys". This is similar to the > message we get for mixing list and non-list keys. > > I find qdict_crumple()'s first loop hard to understand. Rearrange it > and add a comment. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> To be honest, I found the old version of the loop more obvious. > qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > index a4e1c8d08f..35e9052816 100644 > --- a/qobject/block-qdict.c > +++ b/qobject/block-qdict.c > @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > QObject *qdict_crumple(const QDict *src, Error **errp) > { > const QDictEntry *ent; > - QDict *two_level, *multi_level = NULL; > + QDict *two_level, *multi_level = NULL, *child_dict; > QObject *dst = NULL, *child; > size_t i; > char *prefix = NULL; > @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > } > > qdict_split_flat_key(ent->key, &prefix, &suffix); > - > child = qdict_get(two_level, prefix); > + child_dict = qobject_to(QDict, child); > + > + if (child) { > + /* > + * An existing child must be a dict and @ent must be a > + * dict member (i.e. suffix not null), or else @ent > + * clashes. > + */ > + if (!child_dict || !suffix) { > + error_setg(errp, > + "Cannot mix scalar and non-scalar keys"); > + goto error; > + } > + } else if (suffix) { > + child_dict = qdict_new(); > + qdict_put(two_level, prefix, child_dict); > + } else { > + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > + } At least, can you please move the else branch to the if below so that the addition of the new entry is symmetrical for both scalars and dicts? As the code is, it mixes the conflict check, creation of the child dict and addition of scalars (but not to the child dict) in a weird way in a single if block. Or maybe organise it like this: if (child && !(child_dict && suffix)) { error } if (suffix) { if (!child_dict) { create it add it to two_level } add entry to child_dict } else { add entry to two_level } Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> When you mix scalar and non-scalar keys, whether you get an "already >> set as scalar" or an "already set as dict" error depends on qdict >> iteration order. Neither message makes much sense. Replace by >> ""Cannot mix scalar and non-scalar keys". This is similar to the >> message we get for mixing list and non-list keys. >> >> I find qdict_crumple()'s first loop hard to understand. Rearrange it >> and add a comment. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > To be honest, I found the old version of the loop more obvious. > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- >> 1 file changed, 21 insertions(+), 21 deletions(-) >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c >> index a4e1c8d08f..35e9052816 100644 >> --- a/qobject/block-qdict.c >> +++ b/qobject/block-qdict.c >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) >> QObject *qdict_crumple(const QDict *src, Error **errp) >> { >> const QDictEntry *ent; >> - QDict *two_level, *multi_level = NULL; >> + QDict *two_level, *multi_level = NULL, *child_dict; >> QObject *dst = NULL, *child; >> size_t i; >> char *prefix = NULL; >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) >> } >> >> qdict_split_flat_key(ent->key, &prefix, &suffix); >> - >> child = qdict_get(two_level, prefix); >> + child_dict = qobject_to(QDict, child); >> + >> + if (child) { >> + /* >> + * An existing child must be a dict and @ent must be a >> + * dict member (i.e. suffix not null), or else @ent >> + * clashes. >> + */ >> + if (!child_dict || !suffix) { >> + error_setg(errp, >> + "Cannot mix scalar and non-scalar keys"); >> + goto error; >> + } >> + } else if (suffix) { >> + child_dict = qdict_new(); >> + qdict_put(two_level, prefix, child_dict); >> + } else { >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> + } > > At least, can you please move the else branch to the if below so that > the addition of the new entry is symmetrical for both scalars and dicts? > As the code is, it mixes the conflict check, creation of the child dict > and addition of scalars (but not to the child dict) in a weird way in a > single if block. > > Or maybe organise it like this: > > if (child && !(child_dict && suffix)) { > error > } > > if (suffix) { > if (!child_dict) { > create it > add it to two_level > } > add entry to child_dict > } else { > add entry to two_level > } Fleshing out... if (child && !child_dict) { /* * @prefix already exists and it's a non-dictionary, * i.e. we've seen a scalar with key @prefix. The same * key can't occur twice, therefore suffix must be * non-null. */ assert(suffix); /* * @ent has key @prefix.@suffix, but we've already seen * key @prefix: clash. */ error_setg(errp, "Cannot mix scalar and non-scalar keys"); goto error; } if (suffix) { if (!child_dict) { child_dict = qdict_new(); qdict_put(two_level, prefix, child_dict); } qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); } else { assert(!child); qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); } Okay?
Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > >> When you mix scalar and non-scalar keys, whether you get an "already > >> set as scalar" or an "already set as dict" error depends on qdict > >> iteration order. Neither message makes much sense. Replace by > >> ""Cannot mix scalar and non-scalar keys". This is similar to the > >> message we get for mixing list and non-list keys. > >> > >> I find qdict_crumple()'s first loop hard to understand. Rearrange it > >> and add a comment. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > To be honest, I found the old version of the loop more obvious. > > > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > >> 1 file changed, 21 insertions(+), 21 deletions(-) > >> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > >> index a4e1c8d08f..35e9052816 100644 > >> --- a/qobject/block-qdict.c > >> +++ b/qobject/block-qdict.c > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > >> QObject *qdict_crumple(const QDict *src, Error **errp) > >> { > >> const QDictEntry *ent; > >> - QDict *two_level, *multi_level = NULL; > >> + QDict *two_level, *multi_level = NULL, *child_dict; > >> QObject *dst = NULL, *child; > >> size_t i; > >> char *prefix = NULL; > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > >> } > >> > >> qdict_split_flat_key(ent->key, &prefix, &suffix); > >> - > >> child = qdict_get(two_level, prefix); > >> + child_dict = qobject_to(QDict, child); > >> + > >> + if (child) { > >> + /* > >> + * An existing child must be a dict and @ent must be a > >> + * dict member (i.e. suffix not null), or else @ent > >> + * clashes. > >> + */ > >> + if (!child_dict || !suffix) { > >> + error_setg(errp, > >> + "Cannot mix scalar and non-scalar keys"); > >> + goto error; > >> + } > >> + } else if (suffix) { > >> + child_dict = qdict_new(); > >> + qdict_put(two_level, prefix, child_dict); > >> + } else { > >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > >> + } > > > > At least, can you please move the else branch to the if below so that > > the addition of the new entry is symmetrical for both scalars and dicts? > > As the code is, it mixes the conflict check, creation of the child dict > > and addition of scalars (but not to the child dict) in a weird way in a > > single if block. > > > > Or maybe organise it like this: > > > > if (child && !(child_dict && suffix)) { > > error > > } > > > > if (suffix) { > > if (!child_dict) { > > create it > > add it to two_level > > } > > add entry to child_dict > > } else { > > add entry to two_level > > } > > Fleshing out... > > if (child && !child_dict) { > /* > * @prefix already exists and it's a non-dictionary, > * i.e. we've seen a scalar with key @prefix. The same > * key can't occur twice, therefore suffix must be > * non-null. > */ > assert(suffix); > /* > * @ent has key @prefix.@suffix, but we've already seen > * key @prefix: clash. > */ > error_setg(errp, "Cannot mix scalar and non-scalar keys"); > goto error; > } This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > if (suffix) { > if (!child_dict) { > child_dict = qdict_new(); > qdict_put(two_level, prefix, child_dict); > } > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > } else { > assert(!child); So "foo" after "foo.bar" would fail this assertion. > qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > } > > Okay? Kevin
On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote: > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: > > Kevin Wolf <kwolf@redhat.com> writes: > > > > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > > >> When you mix scalar and non-scalar keys, whether you get an "already > > >> set as scalar" or an "already set as dict" error depends on qdict > > >> iteration order. Neither message makes much sense. Replace by > > >> ""Cannot mix scalar and non-scalar keys". This is similar to the > > >> message we get for mixing list and non-list keys. > > >> > > >> I find qdict_crumple()'s first loop hard to understand. Rearrange it > > >> and add a comment. > > >> > > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > > > To be honest, I found the old version of the loop more obvious. > > > > > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > > >> 1 file changed, 21 insertions(+), 21 deletions(-) > > >> > > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > > >> index a4e1c8d08f..35e9052816 100644 > > >> --- a/qobject/block-qdict.c > > >> +++ b/qobject/block-qdict.c > > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > > >> QObject *qdict_crumple(const QDict *src, Error **errp) > > >> { > > >> const QDictEntry *ent; > > >> - QDict *two_level, *multi_level = NULL; > > >> + QDict *two_level, *multi_level = NULL, *child_dict; > > >> QObject *dst = NULL, *child; > > >> size_t i; > > >> char *prefix = NULL; > > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > > >> } > > >> > > >> qdict_split_flat_key(ent->key, &prefix, &suffix); > > >> - > > >> child = qdict_get(two_level, prefix); > > >> + child_dict = qobject_to(QDict, child); > > >> + > > >> + if (child) { > > >> + /* > > >> + * An existing child must be a dict and @ent must be a > > >> + * dict member (i.e. suffix not null), or else @ent > > >> + * clashes. > > >> + */ > > >> + if (!child_dict || !suffix) { > > >> + error_setg(errp, > > >> + "Cannot mix scalar and non-scalar keys"); > > >> + goto error; > > >> + } > > >> + } else if (suffix) { > > >> + child_dict = qdict_new(); > > >> + qdict_put(two_level, prefix, child_dict); > > >> + } else { > > >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > > >> + } > > > > > > At least, can you please move the else branch to the if below so that > > > the addition of the new entry is symmetrical for both scalars and dicts? > > > As the code is, it mixes the conflict check, creation of the child dict > > > and addition of scalars (but not to the child dict) in a weird way in a > > > single if block. > > > > > > Or maybe organise it like this: > > > > > > if (child && !(child_dict && suffix)) { > > > error > > > } > > > > > > if (suffix) { > > > if (!child_dict) { > > > create it > > > add it to two_level > > > } > > > add entry to child_dict > > > } else { > > > add entry to two_level > > > } > > > > Fleshing out... > > > > if (child && !child_dict) { > > /* > > * @prefix already exists and it's a non-dictionary, > > * i.e. we've seen a scalar with key @prefix. The same > > * key can't occur twice, therefore suffix must be > > * non-null. > > */ > > assert(suffix); > > /* > > * @ent has key @prefix.@suffix, but we've already seen > > * key @prefix: clash. > > */ > > error_setg(errp, "Cannot mix scalar and non-scalar keys"); > > goto error; > > } > > This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > > > if (suffix) { > > if (!child_dict) { > > child_dict = qdict_new(); > > qdict_put(two_level, prefix, child_dict); > > } > > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > > } else { > > assert(!child); > > So "foo" after "foo.bar" would fail this assertion. We got coverage of bad inputs in tests/check-qdict.c If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios we should extend it to make sure we detect them during testing Regards, Daniel
Kevin Wolf <kwolf@redhat.com> writes: > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> >> When you mix scalar and non-scalar keys, whether you get an "already >> >> set as scalar" or an "already set as dict" error depends on qdict >> >> iteration order. Neither message makes much sense. Replace by >> >> ""Cannot mix scalar and non-scalar keys". This is similar to the >> >> message we get for mixing list and non-list keys. >> >> >> >> I find qdict_crumple()'s first loop hard to understand. Rearrange it >> >> and add a comment. >> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > >> > To be honest, I found the old version of the loop more obvious. >> > >> >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- >> >> 1 file changed, 21 insertions(+), 21 deletions(-) >> >> >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c >> >> index a4e1c8d08f..35e9052816 100644 >> >> --- a/qobject/block-qdict.c >> >> +++ b/qobject/block-qdict.c >> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) >> >> QObject *qdict_crumple(const QDict *src, Error **errp) >> >> { >> >> const QDictEntry *ent; >> >> - QDict *two_level, *multi_level = NULL; >> >> + QDict *two_level, *multi_level = NULL, *child_dict; >> >> QObject *dst = NULL, *child; >> >> size_t i; >> >> char *prefix = NULL; >> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) >> >> } >> >> >> >> qdict_split_flat_key(ent->key, &prefix, &suffix); >> >> - >> >> child = qdict_get(two_level, prefix); >> >> + child_dict = qobject_to(QDict, child); >> >> + >> >> + if (child) { >> >> + /* >> >> + * An existing child must be a dict and @ent must be a >> >> + * dict member (i.e. suffix not null), or else @ent >> >> + * clashes. >> >> + */ >> >> + if (!child_dict || !suffix) { >> >> + error_setg(errp, >> >> + "Cannot mix scalar and non-scalar keys"); >> >> + goto error; >> >> + } >> >> + } else if (suffix) { >> >> + child_dict = qdict_new(); >> >> + qdict_put(two_level, prefix, child_dict); >> >> + } else { >> >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> >> + } >> > >> > At least, can you please move the else branch to the if below so that >> > the addition of the new entry is symmetrical for both scalars and dicts? >> > As the code is, it mixes the conflict check, creation of the child dict >> > and addition of scalars (but not to the child dict) in a weird way in a >> > single if block. >> > >> > Or maybe organise it like this: >> > >> > if (child && !(child_dict && suffix)) { >> > error >> > } >> > >> > if (suffix) { >> > if (!child_dict) { >> > create it >> > add it to two_level >> > } >> > add entry to child_dict >> > } else { >> > add entry to two_level >> > } >> >> Fleshing out... >> >> if (child && !child_dict) { >> /* >> * @prefix already exists and it's a non-dictionary, >> * i.e. we've seen a scalar with key @prefix. The same >> * key can't occur twice, therefore suffix must be >> * non-null. >> */ >> assert(suffix); >> /* >> * @ent has key @prefix.@suffix, but we've already seen >> * key @prefix: clash. >> */ >> error_setg(errp, "Cannot mix scalar and non-scalar keys"); >> goto error; >> } > > This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > >> if (suffix) { >> if (!child_dict) { >> child_dict = qdict_new(); >> qdict_put(two_level, prefix, child_dict); >> } >> qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); >> } else { >> assert(!child); > > So "foo" after "foo.bar" would fail this assertion. > >> qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> } >> >> Okay? > > Kevin I feel dumb. Next try: if (child) { /* * If @child_dict, then all previous keys with this prefix * had a suffix. If @suffix, this one has one as well, * and we're good, else there's a clash. */ if (!child_dict || !suffix) { error_setg(errp, "Cannot mix scalar and non-scalar keys"); goto error; } } if (suffix) { if (!child_dict) { child_dict = qdict_new(); qdict_put(two_level, prefix, child_dict); } qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); } else { qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); }
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote: >> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: >> > Kevin Wolf <kwolf@redhat.com> writes: >> > >> > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: >> > >> When you mix scalar and non-scalar keys, whether you get an "already >> > >> set as scalar" or an "already set as dict" error depends on qdict >> > >> iteration order. Neither message makes much sense. Replace by >> > >> ""Cannot mix scalar and non-scalar keys". This is similar to the >> > >> message we get for mixing list and non-list keys. >> > >> >> > >> I find qdict_crumple()'s first loop hard to understand. Rearrange it >> > >> and add a comment. >> > >> >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > > >> > > To be honest, I found the old version of the loop more obvious. >> > > >> > >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- >> > >> 1 file changed, 21 insertions(+), 21 deletions(-) >> > >> >> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c >> > >> index a4e1c8d08f..35e9052816 100644 >> > >> --- a/qobject/block-qdict.c >> > >> +++ b/qobject/block-qdict.c >> > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) >> > >> QObject *qdict_crumple(const QDict *src, Error **errp) >> > >> { >> > >> const QDictEntry *ent; >> > >> - QDict *two_level, *multi_level = NULL; >> > >> + QDict *two_level, *multi_level = NULL, *child_dict; >> > >> QObject *dst = NULL, *child; >> > >> size_t i; >> > >> char *prefix = NULL; >> > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) >> > >> } >> > >> >> > >> qdict_split_flat_key(ent->key, &prefix, &suffix); >> > >> - >> > >> child = qdict_get(two_level, prefix); >> > >> + child_dict = qobject_to(QDict, child); >> > >> + >> > >> + if (child) { >> > >> + /* >> > >> + * An existing child must be a dict and @ent must be a >> > >> + * dict member (i.e. suffix not null), or else @ent >> > >> + * clashes. >> > >> + */ >> > >> + if (!child_dict || !suffix) { >> > >> + error_setg(errp, >> > >> + "Cannot mix scalar and non-scalar keys"); >> > >> + goto error; >> > >> + } >> > >> + } else if (suffix) { >> > >> + child_dict = qdict_new(); >> > >> + qdict_put(two_level, prefix, child_dict); >> > >> + } else { >> > >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); >> > >> + } >> > > >> > > At least, can you please move the else branch to the if below so that >> > > the addition of the new entry is symmetrical for both scalars and dicts? >> > > As the code is, it mixes the conflict check, creation of the child dict >> > > and addition of scalars (but not to the child dict) in a weird way in a >> > > single if block. >> > > >> > > Or maybe organise it like this: >> > > >> > > if (child && !(child_dict && suffix)) { >> > > error >> > > } >> > > >> > > if (suffix) { >> > > if (!child_dict) { >> > > create it >> > > add it to two_level >> > > } >> > > add entry to child_dict >> > > } else { >> > > add entry to two_level >> > > } >> > >> > Fleshing out... >> > >> > if (child && !child_dict) { >> > /* >> > * @prefix already exists and it's a non-dictionary, >> > * i.e. we've seen a scalar with key @prefix. The same >> > * key can't occur twice, therefore suffix must be >> > * non-null. >> > */ >> > assert(suffix); >> > /* >> > * @ent has key @prefix.@suffix, but we've already seen >> > * key @prefix: clash. >> > */ >> > error_setg(errp, "Cannot mix scalar and non-scalar keys"); >> > goto error; >> > } >> >> This catches "foo.bar" after "foo", but not "foo" after "foo.bar". >> >> > if (suffix) { >> > if (!child_dict) { >> > child_dict = qdict_new(); >> > qdict_put(two_level, prefix, child_dict); >> > } >> > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); >> > } else { >> > assert(!child); >> >> So "foo" after "foo.bar" would fail this assertion. > > We got coverage of bad inputs in tests/check-qdict.c > > If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios > we should extend it to make sure we detect them during testing Depends on qdict_first() / qdict_next() traversal order, which makes it awkward.
Am 14.06.2018 um 13:52 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > >> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben: > >> >> When you mix scalar and non-scalar keys, whether you get an "already > >> >> set as scalar" or an "already set as dict" error depends on qdict > >> >> iteration order. Neither message makes much sense. Replace by > >> >> ""Cannot mix scalar and non-scalar keys". This is similar to the > >> >> message we get for mixing list and non-list keys. > >> >> > >> >> I find qdict_crumple()'s first loop hard to understand. Rearrange it > >> >> and add a comment. > >> >> > >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> > > >> > To be honest, I found the old version of the loop more obvious. > >> > > >> >> qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- > >> >> 1 file changed, 21 insertions(+), 21 deletions(-) > >> >> > >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c > >> >> index a4e1c8d08f..35e9052816 100644 > >> >> --- a/qobject/block-qdict.c > >> >> +++ b/qobject/block-qdict.c > >> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) > >> >> QObject *qdict_crumple(const QDict *src, Error **errp) > >> >> { > >> >> const QDictEntry *ent; > >> >> - QDict *two_level, *multi_level = NULL; > >> >> + QDict *two_level, *multi_level = NULL, *child_dict; > >> >> QObject *dst = NULL, *child; > >> >> size_t i; > >> >> char *prefix = NULL; > >> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) > >> >> } > >> >> > >> >> qdict_split_flat_key(ent->key, &prefix, &suffix); > >> >> - > >> >> child = qdict_get(two_level, prefix); > >> >> + child_dict = qobject_to(QDict, child); > >> >> + > >> >> + if (child) { > >> >> + /* > >> >> + * An existing child must be a dict and @ent must be a > >> >> + * dict member (i.e. suffix not null), or else @ent > >> >> + * clashes. > >> >> + */ > >> >> + if (!child_dict || !suffix) { > >> >> + error_setg(errp, > >> >> + "Cannot mix scalar and non-scalar keys"); > >> >> + goto error; > >> >> + } > >> >> + } else if (suffix) { > >> >> + child_dict = qdict_new(); > >> >> + qdict_put(two_level, prefix, child_dict); > >> >> + } else { > >> >> + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > >> >> + } > >> > > >> > At least, can you please move the else branch to the if below so that > >> > the addition of the new entry is symmetrical for both scalars and dicts? > >> > As the code is, it mixes the conflict check, creation of the child dict > >> > and addition of scalars (but not to the child dict) in a weird way in a > >> > single if block. > >> > > >> > Or maybe organise it like this: > >> > > >> > if (child && !(child_dict && suffix)) { > >> > error > >> > } > >> > > >> > if (suffix) { > >> > if (!child_dict) { > >> > create it > >> > add it to two_level > >> > } > >> > add entry to child_dict > >> > } else { > >> > add entry to two_level > >> > } > >> > >> Fleshing out... > >> > >> if (child && !child_dict) { > >> /* > >> * @prefix already exists and it's a non-dictionary, > >> * i.e. we've seen a scalar with key @prefix. The same > >> * key can't occur twice, therefore suffix must be > >> * non-null. > >> */ > >> assert(suffix); > >> /* > >> * @ent has key @prefix.@suffix, but we've already seen > >> * key @prefix: clash. > >> */ > >> error_setg(errp, "Cannot mix scalar and non-scalar keys"); > >> goto error; > >> } > > > > This catches "foo.bar" after "foo", but not "foo" after "foo.bar". > > > >> if (suffix) { > >> if (!child_dict) { > >> child_dict = qdict_new(); > >> qdict_put(two_level, prefix, child_dict); > >> } > >> qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > >> } else { > >> assert(!child); > > > > So "foo" after "foo.bar" would fail this assertion. > > > >> qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > >> } > >> > >> Okay? > > > > Kevin > > I feel dumb. Next try: > > if (child) { > /* > * If @child_dict, then all previous keys with this prefix > * had a suffix. If @suffix, this one has one as well, > * and we're good, else there's a clash. > */ > if (!child_dict || !suffix) { > error_setg(errp, "Cannot mix scalar and non-scalar keys"); > goto error; > } > } > > if (suffix) { > if (!child_dict) { > child_dict = qdict_new(); > qdict_put(two_level, prefix, child_dict); > } > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); > } else { > qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); > } This one looks good to me. (Though I feel your comments explains the !(child_dict && suffix) form that I chose rather than what you actually have in the code. But I don't mind.) Kevin
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c index a4e1c8d08f..35e9052816 100644 --- a/qobject/block-qdict.c +++ b/qobject/block-qdict.c @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp) QObject *qdict_crumple(const QDict *src, Error **errp) { const QDictEntry *ent; - QDict *two_level, *multi_level = NULL; + QDict *two_level, *multi_level = NULL, *child_dict; QObject *dst = NULL, *child; size_t i; char *prefix = NULL; @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error **errp) } qdict_split_flat_key(ent->key, &prefix, &suffix); - child = qdict_get(two_level, prefix); + child_dict = qobject_to(QDict, child); + + if (child) { + /* + * An existing child must be a dict and @ent must be a + * dict member (i.e. suffix not null), or else @ent + * clashes. + */ + if (!child_dict || !suffix) { + error_setg(errp, + "Cannot mix scalar and non-scalar keys"); + goto error; + } + } else if (suffix) { + child_dict = qdict_new(); + qdict_put(two_level, prefix, child_dict); + } else { + qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); + } + if (suffix) { - QDict *child_dict = qobject_to(QDict, child); - if (!child_dict) { - if (child) { - error_setg(errp, "Key %s prefix is already set as a scalar", - prefix); - goto error; - } - - child_dict = qdict_new(); - qdict_put_obj(two_level, prefix, QOBJECT(child_dict)); - } - qdict_put_obj(child_dict, suffix, qobject_ref(ent->value)); - } else { - if (child) { - error_setg(errp, "Key %s prefix is already set as a dict", - prefix); - goto error; - } - qdict_put_obj(two_level, prefix, qobject_ref(ent->value)); } g_free(prefix);
When you mix scalar and non-scalar keys, whether you get an "already set as scalar" or an "already set as dict" error depends on qdict iteration order. Neither message makes much sense. Replace by ""Cannot mix scalar and non-scalar keys". This is similar to the message we get for mixing list and non-list keys. I find qdict_crumple()'s first loop hard to understand. Rearrange it and add a comment. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qobject/block-qdict.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)