All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: libvir-list@redhat.com, Paolo Bonzini <pbonzini@redhat.com>,
	John Snow <jsnow@redhat.com>,
	qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>
Subject: Re: A brief look at deprecating our JSON extensions over RFC 8259
Date: Tue, 23 Feb 2021 10:31:55 +0100	[thread overview]
Message-ID: <87o8gbf7uc.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <YDPv7Bk6/DNq/lCn@redhat.com> ("Daniel P. Berrangé"'s message of "Mon, 22 Feb 2021 17:54:52 +0000")

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Feb 22, 2021 at 06:47:30PM +0100, Paolo Bonzini wrote:
>> On 22/02/21 16:24, Daniel P. Berrangé wrote:
>> > This problem isn't unique to QEMU. Any app using JSON from the
>> > shell will have the tedium of quote escaping. JSON is incredibly
>> > widespread and no other apps felt it neccessary to introduce single
>> > quoting support, because the benefit doesn't outweigh the interop
>> > problem it introduces.
>> 
>> The quotes were introduced for C code (and especially qtest), not for the
>> shell.  We have something like
>> 
>>     response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
>>                    "'property': 'temperature' } }", id);
>> 
>> These are sent to QEMU as double-quoted strings (the single-quoted JSON is
>> parsed to get interpolation and printed back; commit 563890c7c7, "libqtest:
>> escape strings in QMP commands, fix leak", 2014-07-01). However, doing the
>> interpolation requires a parser that recognizes the single-quoted strings.
>
> IMHO this is the wrong solution to the problem.  Consider the equivalent
> libvirt code that uses a standard JSON library underneath and has a high
> level API to serialize args into the command
>
>       qemuMonitorJSONMakeCommand("qom-get",
>                                  "s:path", id,
> 				 "s:property", "temperature");
>
> Of course this example is reasonably easy since it is a flat set of
> arguments. Nested args get slightly more complicated, but still always
> preferrable to doing string interpolation IMHO.

Misunderstanding: our JSON interpolation feature is *not* string
interpolation!  It interpolates *objects* into the QObject built by the
parser.

Best explained with an example.  The qmp() call above internally builds
the following QObject:

    QDict mapping
        key "execute" to a QString for "qom-get"
        key "arguments" to a QDict mapping
            key "path" to a QString for @id (interpolation!)
            key "property" to a QString for "temperature"

Unlike string interpolation, this is safe for any valid C string @id.

If you think like a hacker, you might try shenanigans like

    "{'execute': '%s'}"

You will then find that somebody has thought like a hacker before
you[*].

The %-sequences are cleverly chosen to get some protection against
common mistakes from the compiler.

This interpolation has worked quite well for us, and I have no plans to
replace it.  Doesn't mean I'm against replacing it, only that I want to
see benefits exceeding the costs.

A bigger stumbling block for replacement is our need for a streaming
interface: we feed the parser characters, and expect to be called back
when an expression is complete.


[*] Commit 16a4859921 "json: Improve safety of
qobject_from_jsonf_nofail() & friends", fixed up in commit bbc0586ced
"json: Fix % handling when not interpolating".



  parent reply	other threads:[~2021-02-23  9:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-22 14:57 A brief look at deprecating our JSON extensions over RFC 8259 Markus Armbruster
2021-02-22 15:10 ` Peter Krempa
2021-02-22 15:24 ` Daniel P. Berrangé
2021-02-22 15:43   ` Liviu Ionescu
2021-02-22 17:47   ` Paolo Bonzini
2021-02-22 17:54     ` Daniel P. Berrangé
2021-02-22 18:06       ` Paolo Bonzini
2021-02-23  9:31       ` Markus Armbruster [this message]
2021-02-23  9:37         ` Markus Armbruster
2021-02-23 11:03         ` Peter Maydell
2021-02-23 12:43           ` Markus Armbruster
2021-02-23  9:06     ` Markus Armbruster
2021-02-23 10:59       ` Paolo Bonzini
2021-02-23 12:34         ` Markus Armbruster
2021-02-22 17:42 ` Paolo Bonzini
2021-02-22 18:01   ` Daniel P. Berrangé
2021-02-22 18:22   ` Peter Krempa
2021-02-22 18:25     ` Peter Krempa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o8gbf7uc.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=libvir-list@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.