Re: [Qemu-devel] [PATCH] fw_cfg: insert string blobs via qemu cmdline

[Laszlo Ersek <lersek@redhat.com>]

On 09/29/15 00:47, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 12:08:14AM +0200, Laszlo Ersek wrote:
>> On 09/28/15 23:45, Gabriel L. Somlo wrote:
>>> On Mon, Sep 28, 2015 at 11:05:32PM +0200, Laszlo Ersek wrote:
>>>> On 09/28/15 22:56, Laszlo Ersek wrote:
>>>>> On 09/28/15 22:00, Eric Blake wrote:
>>>>>> On 09/28/2015 01:51 PM, Gabriel L. Somlo wrote:
>>>>>>> On Mon, Sep 28, 2015 at 01:46:33PM -0600, Eric Blake wrote:
>>>>>>>> On 09/28/2015 07:30 AM, Laszlo Ersek wrote:
>>>>>>>>
>>>>>>>>>>  
>>>>>>>>>> +Small enough items may be provided directly as strings on the command
>>>>>>>>>> +line, using the syntax:
>>>>>>>>>> +
>>>>>>>>>> +    -fw_cfg [name=]<item_name>,content=<string>
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Please consider spelling out that these blobs will NOT be NUL-terminated
>>>>>>>>> when viewed on the guest. (It kinda follows from all the other fw_cfg
>>>>>>>>> things, but once we leave host-side files for qemu command line strings,
>>>>>>>>> it might become non-obvious to users.)
>>>>>>>>
>>>>>>>> Or else GUARANTEE that it will be NUL-terminated (and the only way to
>>>>>>>> get blobs that are not NUL terminated is to use files rather than content=).
>>>>>>>
>>>>>>> I went with the first suggestion (leave out the trailing '\0' from the
>>>>>>> blob payload, and say so in docs/specs/fw_cfg.txt) in v2 of the patch.
>>>>>>>
>>>>>>> Do you feel strongly about including the \0 ? Otherwise, we're already
>>>>>>> there :)
>>>>>>
>>>>>> I don't know what users are more likely to want to push through. A
>>>>>> trailing NUL implies a binary file (as text files cannot contain \0),
>>>>>> but even without a trailing NUL, a file is not a text file (per the
>>>>>> POSIX definition) unless it is either empty or ends in a newline.  Use
>>>>>> of content=... is unlikely to have users remembering to place a newline
>>>>>> there if examples don't suggest it.  And I don't know if guests are
>>>>>> expecting text data from these blobs, or are okay with binary blobs.
>>>>>
>>>>> fw_cfg blobs are considered binary, unless a specific selector key or
>>>>> fw_cfg file name makes different arrangements. (Described in QEMU docs,
>>>>> or elsewhere.) See more below.
>>>>>
>>>>>> That's a long-winded way of stating that omitting the NUL is fine by me,
>>>>>> as long as you document it, and as long as you are catering to the most
>>>>>> common user usage of the feature.
>>>>>
>>>>> The main consumer of the -fw_cfg switch is guest firmware (and, perhaps
>>>>> soon, the guest kernel too); the idea is to pass down firmware config
>>>>> items without QEMU being aware of their actual meaning. Therefore I'd
>>>>> like to see as little smarts as possible in QEMU wrt. the content passed
>>>>> down with -fw_cfg.
>>>>>
>>>>>>
>>>>>> Either that, or it's my way of dreaming about alternative 3: guarantee a
>>>>>> trailing newline (rather than NUL), so that 'content="abc"' on the
>>>>>> command line results in the 4-byte blob "abc\n" in the guest.
>>>>>>
>>>>>
>>>>> Please don't :)
>>>>>
>>>>> The current client code in OVMF (in effect for two specific fw_cfg file
>>>>> names) recognizes the following content pattern:
>>>>>
>>>>>   [0nN1yY](\n|\r\n)?
>>>>>
>>>>> E.g., QEMU may pass in a simple host-side file as an fw_cfg entry on a
>>>>> Windows host too. If you edited that file with "notepad.exe", it'll have
>>>>> \r\n, or maybe no line terminator at all. Other (really binary) blobs
>>>>> (passed in with file=...) may have embedded \0 characters.
>>>>>
>>>>> I think such flexibility is best left to the firmware, or else should be
>>>>> restricted in specifications living outside of QEMU, and QEMU should be
>>>>> dumb and transparent here, in accordance with the original goal of this
>>>>> feature.
>>>>>
>>>>> Re: policy vs. mechanism, the opt/ prefix is also strongly recommended
>>>>> (for the names), but we don't enforce it.
>>>>
>>>> ... This made me think of the following language that Gabriel added in
>>>> v2 (at my request, and to my acceptance):
>>>>
>>>>> Both <item_name> and, if applicable, the content <string> are assumed
>>>>> to consist exclusively of plain ASCII characters.
>>>>
>>>> Now I think that this could be improved. I think we should say "should
>>>> consist" rather than "are assumed to consist", because neither the QEMU
>>>> nor the firmware(s) "assume" anything in general here -- that would be
>>>> policy --, we just want to help the user avoid shooting himself in the
>>>> foot (and reporting a bug), lest he pass non-ASCII UTF-8 on the command
>>>> line, and the firmware do surprising things.
>>>>
>>>> Maybe I should even retract my request for spelling out ASCII... That's
>>>> really not a requirement, just a high-level recommendation for humans
>>>> who develop guest code for this interface, to save their sanity.
>>>
>>> Maybe something like this, then:
>>>
>>> Both <item_name> and, if applicable, the content <string> will be
>>> passed through by QEMU without any interpretation, expansion, or 
>>> further processing. Any such processing (potentially performed by
>>> e.g., the shell) is outside QEMU's responsibility; as such, using
>>> plain ASCII characters is recommended.
>>>
>>> Let me know what you think.
>>
>> Sounds good to me. Thanks for your patience. :)
> 
> Cool, v3 coming right up. Not 100% sure about etiquette, but I'm
> inclined to assume your earlier Reviewed-by is still valid :)

Sure.

> Thanks,
> --Gabriel
> 
> PS. You have a *very* *large* reserve of patience credits with me, so
> no worries on that :)
> 

Heh, thanks. :)
Laszlo