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