On 08/09/2017 04:06 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> We want -Wformat to catch blatant programming errors in format >> strings that we pass to qobject_from_jsonf(). But if someone >> were to pass a JSON string "'%s'" as the format string, gcc would >> insist that it be paired with a char* argument, even though our >> lexer does not recognize % sequences inside a JSON string. You can >> bypass -Wformat checking by passing the Unicode escape \u0025 for >> %, but who wants to remember to type that? So the solution is that >> anywhere we are relying on -Wformat checking, the caller should >> pass the usual printf %% escape sequence where a literal % is >> wanted on output. >>
>> + bool double_quote = *ptr++ == '"'; >> >> str = qstring_new(); >> - while (*ptr && >> - ((double_quote && *ptr != '"') || (!double_quote && *ptr != >> '\''))) { >> + while (*ptr && *ptr != "'\""[double_quote]) { > > Simpler: > > bool quote = *ptr++; > > and then > > while (*ptr && *ptr != quote) { Well, 'char quote' rather than 'bool quote', but yes, I like it. > > Have you considered splitting the patch into one to simplify, and one to > implement %%? Will split. >> @@ -455,13 +452,15 @@ static QObject *parse_escape(JSONParserContext *ctxt, >> va_list *ap) >> { >> JSONToken *token; >> >> - if (ap == NULL) { >> - return NULL; >> - } >> - >> token = parser_context_pop_token(ctxt); >> assert(token && token->type == JSON_ESCAPE); >> >> + if (ap == NULL) { >> + parse_error(ctxt, token, "escape parsing for '%s' not requested", >> + token->str); >> + return NULL; >> + } >> + > > When I manage to fat-finger a '%' into my QMP input, I now get this > error message instead of "Invalid JSON syntax". Makes me go "what is > escape parsing, and how do I request it?" Not an improvement, I'm > afraid. Pre-patch, I see: $ qemu-kvm -nodefaults -nographic -qmp stdio {"QMP": {"version": {"qemu": {"micro": 91, "minor": 9, "major": 2}, "package": "(qemu-2.10.0-0.1.rc1.fc26)"}, "capabilities": []}} {'execute':%s} {"error": {"class": "GenericError", "desc": "JSON parse error, Missing value in dict"}} {'execute':%%} {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}} {"error": {"class": "GenericError", "desc": "JSON parse error, expecting value"}} I find it odd that NOT calling parse_error() but still returning NULL changes the behavior on what error message eventually gets emitted; but I also agree that the QMP case should drive what error message (if any) is needed in parse_escape(). I'll play with it some more (the parser's error handling is weird). >> + /* In vararg form, %% must occur in strings */ >> + /* qobject_from_jsonf("%%"); */ >> + /* qobject_from_jsonf("{%%}"); */ >> + >> + /* In vararg form, strings must not use % except for %% */ >> + /* qobject_from_jsonf("'%s'", "unpaired"); */ > > Could use g_test_trap_subprocess(). Not sure it's worth the bother. I don't know - this is one case where proving we abort on invalid usage might actually be a good validation of the contract. > > I hate code in comments. Better: > > /* The following intentionally cause assertion failures */ > #if 0 > /* In vararg form, %% must occur in strings */ > qobject_from_jsonf("%%"); If I don't use the g_test_trap_subprocess() trick, then I can override checkpatch's complaints about #if 0. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature