| Message ID | 1457420446-25276-2-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cc: Kevin, because he added the array in question. Peter Xu <peterx@redhat.com> writes: > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > CC: Luiz Capitulino <lcapitulino@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > qobject/qdict.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/qobject/qdict.c b/qobject/qdict.c > index 9833bd0..eb602a7 100644 > --- a/qobject/qdict.c > +++ b/qobject/qdict.c > @@ -704,17 +704,19 @@ int qdict_array_entries(QDict *src, const char *subqdict) > for (i = 0; i < INT_MAX; i++) { > QObject *subqobj; > int subqdict_entries; > - size_t slen = 32 + subqdict_len; > - char indexstr[slen], prefix[slen]; > +#define __SLEN_MAX (128) > + char indexstr[__SLEN_MAX], prefix[__SLEN_MAX]; > size_t snprintf_ret; > > - snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i); > - assert(snprintf_ret < slen); > + assert(__SLEN_MAX >= 32 + subqdict_len); > + > + snprintf_ret = snprintf(indexstr, __SLEN_MAX, "%s%u", subqdict, i); > + assert(snprintf_ret < __SLEN_MAX); > > subqobj = qdict_get(src, indexstr); > > - snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i); > - assert(snprintf_ret < slen); > + snprintf_ret = snprintf(prefix, __SLEN_MAX, "%s%u.", subqdict, i); > + assert(snprintf_ret < __SLEN_MAX); > > subqdict_entries = qdict_count_prefixed_entries(src, prefix); > if (subqdict_entries < 0) { > @@ -745,6 +747,7 @@ int qdict_array_entries(QDict *src, const char *subqdict) > } > > return i; > +#undef __SLEN_MAX > } > > /** Same arguments as for PATCH 2, except here an argument on the maximum length of subqdict would probably be easier. Unrelated to your patch: I think we've pushed QDict use father than sensible. Encoding multiple keys in a string so you can use a flat associative array as your catch-all data structure is appropriate in AWK, but in C? Not so much...
Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: > Cc: Kevin, because he added the array in question. > > Peter Xu <peterx@redhat.com> writes: > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > CC: Luiz Capitulino <lcapitulino@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > qobject/qdict.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/qobject/qdict.c b/qobject/qdict.c > > index 9833bd0..eb602a7 100644 > > --- a/qobject/qdict.c > > +++ b/qobject/qdict.c > > @@ -704,17 +704,19 @@ int qdict_array_entries(QDict *src, const char *subqdict) > > for (i = 0; i < INT_MAX; i++) { > > QObject *subqobj; > > int subqdict_entries; > > - size_t slen = 32 + subqdict_len; > > - char indexstr[slen], prefix[slen]; > > +#define __SLEN_MAX (128) > > + char indexstr[__SLEN_MAX], prefix[__SLEN_MAX]; > > size_t snprintf_ret; > > > > - snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i); > > - assert(snprintf_ret < slen); > > + assert(__SLEN_MAX >= 32 + subqdict_len); > > + > > + snprintf_ret = snprintf(indexstr, __SLEN_MAX, "%s%u", subqdict, i); > > + assert(snprintf_ret < __SLEN_MAX); > > > > subqobj = qdict_get(src, indexstr); > > > > - snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i); > > - assert(snprintf_ret < slen); > > + snprintf_ret = snprintf(prefix, __SLEN_MAX, "%s%u.", subqdict, i); > > + assert(snprintf_ret < __SLEN_MAX); > > > > subqdict_entries = qdict_count_prefixed_entries(src, prefix); > > if (subqdict_entries < 0) { > > @@ -745,6 +747,7 @@ int qdict_array_entries(QDict *src, const char *subqdict) > > } > > > > return i; > > +#undef __SLEN_MAX > > } > > > > /** > > Same arguments as for PATCH 2, except here an argument on the maximum > length of subqdict would probably be easier. Yes, these are constant string literals in all callers, including the one non-test case in quorum. Let's simply assert a reasonable maximum for subqdict_length. The minimum we need to allow with the existing callers is 9, and I expect we'll never get keys longer than 16 characters. > Unrelated to your patch: I think we've pushed QDict use father than > sensible. Encoding multiple keys in a string so you can use a flat > associative array as your catch-all data structure is appropriate in > AWK, but in C? Not so much... We'll always to that, because it's the command line syntax. What you may criticise is that we convert QAPI objects to the command line representation instead of the other way around, but changing that (which I think would be far from trivial, for relatively little use) wouldn't get rid of this kind of key parsing, but just move it a bit closer to the command line handling. Kevin
On 03/08/2016 03:19 AM, Kevin Wolf wrote: > Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: >> Cc: Kevin, because he added the array in question. >> >> Peter Xu <peterx@redhat.com> writes: >> >> Unrelated to your patch: I think we've pushed QDict use father than >> sensible. Encoding multiple keys in a string so you can use a flat >> associative array as your catch-all data structure is appropriate in >> AWK, but in C? Not so much... > > We'll always to that, because it's the command line syntax. What you may > criticise is that we convert QAPI objects to the command line > representation instead of the other way around, but changing that (which > I think would be far from trivial, for relatively little use) wouldn't > get rid of this kind of key parsing, but just move it a bit closer to > the command line handling. I actually WANT us to try that conversion (a great GSoC project, if someone wants it) for 2.7. We already did it for SocketAddress, and it makes the code easier to maintain when you can just access foo->data instead of doing qdict_lookup(foo, "data") all over the place.
Am 08.03.2016 um 17:21 hat Eric Blake geschrieben: > On 03/08/2016 03:19 AM, Kevin Wolf wrote: > > Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: > >> Cc: Kevin, because he added the array in question. > >> > >> Peter Xu <peterx@redhat.com> writes: > >> > > >> Unrelated to your patch: I think we've pushed QDict use father than > >> sensible. Encoding multiple keys in a string so you can use a flat > >> associative array as your catch-all data structure is appropriate in > >> AWK, but in C? Not so much... > > > > We'll always to that, because it's the command line syntax. What you may > > criticise is that we convert QAPI objects to the command line > > representation instead of the other way around, but changing that (which > > I think would be far from trivial, for relatively little use) wouldn't > > get rid of this kind of key parsing, but just move it a bit closer to > > the command line handling. > > I actually WANT us to try that conversion (a great GSoC project, if > someone wants it) for 2.7. We already did it for SocketAddress, and it > makes the code easier to maintain when you can just access foo->data > instead of doing qdict_lookup(foo, "data") all over the place. I think you're underestimating the difference in difficulty between using SocketAddress everywhere and using BlockdevOptions everywhere, which is the reason why I never even put it on my todo list. Of course, I would be fine with your trying anyway, but it would probably be fairer if we not let a poor student fail with this. Kevin
On Tue, Mar 08, 2016 at 05:30:31PM +0100, Kevin Wolf wrote: > Am 08.03.2016 um 17:21 hat Eric Blake geschrieben: > > On 03/08/2016 03:19 AM, Kevin Wolf wrote: > > > Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: > > >> Cc: Kevin, because he added the array in question. > > >> > > >> Peter Xu <peterx@redhat.com> writes: > > >> > > > > >> Unrelated to your patch: I think we've pushed QDict use father than > > >> sensible. Encoding multiple keys in a string so you can use a flat > > >> associative array as your catch-all data structure is appropriate in > > >> AWK, but in C? Not so much... > > > > > > We'll always to that, because it's the command line syntax. What you may > > > criticise is that we convert QAPI objects to the command line > > > representation instead of the other way around, but changing that (which > > > I think would be far from trivial, for relatively little use) wouldn't > > > get rid of this kind of key parsing, but just move it a bit closer to > > > the command line handling. > > > > I actually WANT us to try that conversion (a great GSoC project, if > > someone wants it) for 2.7. We already did it for SocketAddress, and it > > makes the code easier to maintain when you can just access foo->data > > instead of doing qdict_lookup(foo, "data") all over the place. > > I think you're underestimating the difference in difficulty between > using SocketAddress everywhere and using BlockdevOptions everywhere, > which is the reason why I never even put it on my todo list. > > Of course, I would be fine with your trying anyway, but it would > probably be fairer if we not let a poor student fail with this. I think a more sensible/practical first step would be to change the block layer to using the nested QDicts as its primary representation, instead of the flat QDicts. In APIs which get a flat qdict (CLI QemuOpts & HMP), it should really immediately crumple it into a nested QDict before usage. Regards, Daniel
diff --git a/qobject/qdict.c b/qobject/qdict.c index 9833bd0..eb602a7 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -704,17 +704,19 @@ int qdict_array_entries(QDict *src, const char *subqdict) for (i = 0; i < INT_MAX; i++) { QObject *subqobj; int subqdict_entries; - size_t slen = 32 + subqdict_len; - char indexstr[slen], prefix[slen]; +#define __SLEN_MAX (128) + char indexstr[__SLEN_MAX], prefix[__SLEN_MAX]; size_t snprintf_ret; - snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i); - assert(snprintf_ret < slen); + assert(__SLEN_MAX >= 32 + subqdict_len); + + snprintf_ret = snprintf(indexstr, __SLEN_MAX, "%s%u", subqdict, i); + assert(snprintf_ret < __SLEN_MAX); subqobj = qdict_get(src, indexstr); - snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i); - assert(snprintf_ret < slen); + snprintf_ret = snprintf(prefix, __SLEN_MAX, "%s%u.", subqdict, i); + assert(snprintf_ret < __SLEN_MAX); subqdict_entries = qdict_count_prefixed_entries(src, prefix); if (subqdict_entries < 0) { @@ -745,6 +747,7 @@ int qdict_array_entries(QDict *src, const char *subqdict) } return i; +#undef __SLEN_MAX } /**
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> CC: Luiz Capitulino <lcapitulino@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- qobject/qdict.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)