| Message ID | 1457420446-25276-3-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
Peter Xu <peterx@redhat.com> writes: > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: qemu-block@nongnu.org > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > block/qapi.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/block/qapi.c b/block/qapi.c > index db2d3fb..687e577 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, > QType type = qobject_type(entry->value); > bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); > const char *format = composite ? "%*s%s:\n" : "%*s%s: "; Unrelated to your patch: ugh! Printf formats should be literals whenever possible, to make it easy for the compiler to warn you when you screw up. It's trivially possible here! Instead of func_fprintf(f, format, indentation * 4, "", key); do func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, composite ? '\n', ' ');; > - char key[strlen(entry->key) + 1]; > +#define __KEY_LEN (256) > + char key[__KEY_LEN]; > int i; > > + assert(strlen(entry->key) + 1 <= __KEY_LEN); > +#undef __KEY_LEN > /* replace dashes with spaces in key (variable) names */ > for (i = 0; entry->key[i]; i++) { > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; I'm afraid this isn't a good idea. It relies on the non-local argument that nobody will ever put a key longer than 255 into a qdict that gets dumped. That may even be the case, but you need to *prove* it, not just assert it. The weakest acceptable proof might be assertions in every place that put keys into a dict that might get dumped. I suspect that's practical and maintainable only if there's a single place that does it. If this was a good idea, I'd recommend to avoid the awkward macro: char key[256]; int i; assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); There are several other ways to limit the stack usage: 1. Move the array from stack to heap. Fine unless it's on a hot path. As far as I can tell, this dumping business is for HMP and qemu-io, i.e. not hot. 2. Use a stack array for short strings, switch to the heap for large strings. More complex, but may be worth it on a hot path where most strings are short. 3. Instead of creating a new string with '-' replaced for printing, print characters. Can be okay with buffered I/O, but obviously not on a hot path. 4. Like 3., but print substrings not containing '-'.
On Tue, 03/08 09:12, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > CC: Markus Armbruster <armbru@redhat.com> > > CC: Kevin Wolf <kwolf@redhat.com> > > CC: qemu-block@nongnu.org > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > block/qapi.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/block/qapi.c b/block/qapi.c > > index db2d3fb..687e577 100644 > > --- a/block/qapi.c > > +++ b/block/qapi.c > > @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, > > QType type = qobject_type(entry->value); > > bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); > > const char *format = composite ? "%*s%s:\n" : "%*s%s: "; > > Unrelated to your patch: ugh! > > Printf formats should be literals whenever possible, to make it easy for > the compiler to warn you when you screw up. It's trivially possible > here! Instead of > > func_fprintf(f, format, indentation * 4, "", key); > > do > > func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, > composite ? '\n', ' ');; > > > - char key[strlen(entry->key) + 1]; > > +#define __KEY_LEN (256) > > + char key[__KEY_LEN]; > > int i; > > > > + assert(strlen(entry->key) + 1 <= __KEY_LEN); > > +#undef __KEY_LEN > > /* replace dashes with spaces in key (variable) names */ > > for (i = 0; entry->key[i]; i++) { > > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; > > I'm afraid this isn't a good idea. It relies on the non-local argument > that nobody will ever put a key longer than 255 into a qdict that gets > dumped. That may even be the case, but you need to *prove* it, not just > assert it. The weakest acceptable proof might be assertions in every > place that put keys into a dict that might get dumped. I suspect that's > practical and maintainable only if there's a single place that does it. > > If this was a good idea, I'd recommend to avoid the awkward macro: Also I think the double underscore identifiers are considered reserved in C, no? > > char key[256]; > int i; > > assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); > <...> Fam
On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > const char *format = composite ? "%*s%s:\n" : "%*s%s: "; > > Unrelated to your patch: ugh! > > Printf formats should be literals whenever possible, to make it easy for > the compiler to warn you when you screw up. It's trivially possible > here! Instead of > > func_fprintf(f, format, indentation * 4, "", key); > > do > > func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, > composite ? '\n', ' ');; Yes, I can do that too. Do you think I should split the patchset into several smaller ones possibly? So that I can use a 2/2 for this specific function, one for the printf() issue, one for the stack allocation issue. > > > - char key[strlen(entry->key) + 1]; > > +#define __KEY_LEN (256) > > + char key[__KEY_LEN]; > > int i; > > > > + assert(strlen(entry->key) + 1 <= __KEY_LEN); > > +#undef __KEY_LEN > > /* replace dashes with spaces in key (variable) names */ > > for (i = 0; entry->key[i]; i++) { > > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; > > I'm afraid this isn't a good idea. It relies on the non-local argument > that nobody will ever put a key longer than 255 into a qdict that gets > dumped. That may even be the case, but you need to *prove* it, not just > assert it. The weakest acceptable proof might be assertions in every > place that put keys into a dict that might get dumped. I suspect that's > practical and maintainable only if there's a single place that does it. Yes. Actually I doubted whether I should do this change... since I am not experienced enough to estimate a value which will be 100% working all the time. Here I just assumed 256 is big enough for qdict keys, which indeed is lack of proof. > > If this was a good idea, I'd recommend to avoid the awkward macro: > > char key[256]; > int i; > > assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out. > > There are several other ways to limit the stack usage: > > 1. Move the array from stack to heap. Fine unless it's on a hot path. > As far as I can tell, this dumping business is for HMP and qemu-io, > i.e. not hot. > > 2. Use a stack array for short strings, switch to the heap for large > strings. More complex, but may be worth it on a hot path where most > strings are short. > > 3. Instead of creating a new string with '-' replaced for printing, > print characters. Can be okay with buffered I/O, but obviously not > on a hot path. > > 4. Like 3., but print substrings not containing '-'. Thanks for all the suggestions and ideas. To avoid the limitation of 256 (and also consider this path is not hot path), I'd like to choose (1) if you would not mind, in a split patch maybe. Thanks. Peter
On 08/03/2016 09:12, Markus Armbruster wrote: > I'm afraid this isn't a good idea. It relies on the non-local argument > that nobody will ever put a key longer than 255 into a qdict that gets > dumped. That may even be the case, but you need to *prove* it, not just > assert it. The weakest acceptable proof might be assertions in every > place that put keys into a dict that might get dumped. I suspect that's > practical and maintainable only if there's a single place that does it. > > If this was a good idea, I'd recommend to avoid the awkward macro: > > char key[256]; > int i; > > assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); > > There are several other ways to limit the stack usage: > > 1. Move the array from stack to heap. Fine unless it's on a hot path. > As far as I can tell, this dumping business is for HMP and qemu-io, > i.e. not hot. I think this is the best. You can just g_strdup, modify in place, print and free. Paolo
Fam Zheng <famz@redhat.com> writes: > On Tue, 03/08 09:12, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> > CC: Markus Armbruster <armbru@redhat.com> >> > CC: Kevin Wolf <kwolf@redhat.com> >> > CC: qemu-block@nongnu.org >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > block/qapi.c | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/block/qapi.c b/block/qapi.c >> > index db2d3fb..687e577 100644 >> > --- a/block/qapi.c >> > +++ b/block/qapi.c >> > @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, >> > QType type = qobject_type(entry->value); >> > bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); >> > const char *format = composite ? "%*s%s:\n" : "%*s%s: "; >> >> Unrelated to your patch: ugh! >> >> Printf formats should be literals whenever possible, to make it easy for >> the compiler to warn you when you screw up. It's trivially possible >> here! Instead of >> >> func_fprintf(f, format, indentation * 4, "", key); >> >> do >> >> func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, >> composite ? '\n', ' ');; >> >> > - char key[strlen(entry->key) + 1]; >> > +#define __KEY_LEN (256) >> > + char key[__KEY_LEN]; >> > int i; >> > >> > + assert(strlen(entry->key) + 1 <= __KEY_LEN); >> > +#undef __KEY_LEN >> > /* replace dashes with spaces in key (variable) names */ >> > for (i = 0; entry->key[i]; i++) { >> > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; >> >> I'm afraid this isn't a good idea. It relies on the non-local argument >> that nobody will ever put a key longer than 255 into a qdict that gets >> dumped. That may even be the case, but you need to *prove* it, not just >> assert it. The weakest acceptable proof might be assertions in every >> place that put keys into a dict that might get dumped. I suspect that's >> practical and maintainable only if there's a single place that does it. >> >> If this was a good idea, I'd recommend to avoid the awkward macro: > > Also I think the double underscore identifiers are considered reserved in C, > no? Correct. C99 7.1.3 Reserved identifiers: All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. [...]
Peter Xu <peterx@redhat.com> writes: > On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> > const char *format = composite ? "%*s%s:\n" : "%*s%s: "; >> >> Unrelated to your patch: ugh! >> >> Printf formats should be literals whenever possible, to make it easy for >> the compiler to warn you when you screw up. It's trivially possible >> here! Instead of >> >> func_fprintf(f, format, indentation * 4, "", key); >> >> do >> >> func_fprintf(f, "%*s%s:%c", indentation * 4, "", key, >> composite ? '\n', ' ');; > > Yes, I can do that too. Do you think I should split the patchset > into several smaller ones possibly? So that I can use a 2/2 for this > specific function, one for the printf() issue, one for the stack > allocation issue. Since the format string cleanup and the unbounded stack issue aren't related, separate patches make sense. >> >> > - char key[strlen(entry->key) + 1]; >> > +#define __KEY_LEN (256) >> > + char key[__KEY_LEN]; >> > int i; >> > >> > + assert(strlen(entry->key) + 1 <= __KEY_LEN); >> > +#undef __KEY_LEN >> > /* replace dashes with spaces in key (variable) names */ >> > for (i = 0; entry->key[i]; i++) { >> > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i]; >> >> I'm afraid this isn't a good idea. It relies on the non-local argument >> that nobody will ever put a key longer than 255 into a qdict that gets >> dumped. That may even be the case, but you need to *prove* it, not just >> assert it. The weakest acceptable proof might be assertions in every >> place that put keys into a dict that might get dumped. I suspect that's >> practical and maintainable only if there's a single place that does it. > > Yes. Actually I doubted whether I should do this change... since I > am not experienced enough to estimate a value which will be 100% > working all the time. Here I just assumed 256 is big enough for > qdict keys, which indeed is lack of proof. > >> >> If this was a good idea, I'd recommend to avoid the awkward macro: >> >> char key[256]; >> int i; >> >> assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); > > Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out. > >> >> There are several other ways to limit the stack usage: >> >> 1. Move the array from stack to heap. Fine unless it's on a hot path. >> As far as I can tell, this dumping business is for HMP and qemu-io, >> i.e. not hot. >> >> 2. Use a stack array for short strings, switch to the heap for large >> strings. More complex, but may be worth it on a hot path where most >> strings are short. >> >> 3. Instead of creating a new string with '-' replaced for printing, >> print characters. Can be okay with buffered I/O, but obviously not >> on a hot path. >> >> 4. Like 3., but print substrings not containing '-'. > > Thanks for all the suggestions and ideas. > > To avoid the limitation of 256 (and also consider this path is not > hot path), I'd like to choose (1) if you would not mind, in a split > patch maybe. Sounds good to me.
On Tue, Mar 08, 2016 at 02:47:31PM +0100, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > > Also I think the double underscore identifiers are considered reserved in C, > > no? > > Correct. C99 7.1.3 Reserved identifiers: All identifiers that begin > with an underscore and either an uppercase letter or another underscore > are always reserved for any use. > > [...] Fam, Markus, Thanks to point out. Will fix. Peter
On Tue, Mar 08, 2016 at 01:17:03PM +0100, Paolo Bonzini wrote: > > > On 08/03/2016 09:12, Markus Armbruster wrote: > > I'm afraid this isn't a good idea. It relies on the non-local argument > > that nobody will ever put a key longer than 255 into a qdict that gets > > dumped. That may even be the case, but you need to *prove* it, not just > > assert it. The weakest acceptable proof might be assertions in every > > place that put keys into a dict that might get dumped. I suspect that's > > practical and maintainable only if there's a single place that does it. > > > > If this was a good idea, I'd recommend to avoid the awkward macro: > > > > char key[256]; > > int i; > > > > assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key)); > > > > There are several other ways to limit the stack usage: > > > > 1. Move the array from stack to heap. Fine unless it's on a hot path. > > As far as I can tell, this dumping business is for HMP and qemu-io, > > i.e. not hot. > > I think this is the best. You can just g_strdup, modify in place, print > and free. g_strdup() will bring one more loop? One to copy the strings, one for replacing "-" to " ". Though I will first need to replace g_malloc0() with g_malloc(), which seems more suitable here. :) Thanks! Peter
diff --git a/block/qapi.c b/block/qapi.c index db2d3fb..687e577 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation, QType type = qobject_type(entry->value); bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); const char *format = composite ? "%*s%s:\n" : "%*s%s: "; - char key[strlen(entry->key) + 1]; +#define __KEY_LEN (256) + char key[__KEY_LEN]; int i; + assert(strlen(entry->key) + 1 <= __KEY_LEN); +#undef __KEY_LEN /* replace dashes with spaces in key (variable) names */ for (i = 0; entry->key[i]; i++) { key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> CC: qemu-block@nongnu.org Signed-off-by: Peter Xu <peterx@redhat.com> --- block/qapi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)