| Message ID | 1443308129-19965-1-git-send-email-somlo@cmu.edu |
|---|---|
| State | New |
| Headers | show |
On 09/27/15 00:55, Gabriel L. Somlo wrote: > Allow users to provide custom fw_cfg blobs with ascii string > payloads specified directly on the qemu command line. > > Suggested-by: Jordan Justen <jordan.l.justen@intel.com> > Suggested-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > --- > docs/specs/fw_cfg.txt | 5 +++++ > qemu-options.hx | 7 ++++++- > vl.c | 30 ++++++++++++++++++++++++------ > 3 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt > index b5f4b5d..4d85701 100644 > --- a/docs/specs/fw_cfg.txt > +++ b/docs/specs/fw_cfg.txt > @@ -236,6 +236,11 @@ the following syntax: > where <item_name> is the fw_cfg item name, and <path> is the location > on the host file system of a file containing the data to be inserted. > > +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.) So something like, "... the content presented to the guest will not be NUL-terminated, same as if the string was written to a host-side file first". Please also stress that such content (and actually name too, but that's more generic) should be plain ASCII; let's sidestep any tricks a shell's locale settings could pull on us. > NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" > when using the "-fw_cfg" command line option, to avoid conflicting with > item names used internally by QEMU. For instance: > diff --git a/qemu-options.hx b/qemu-options.hx > index 328404c..0b6f5bd 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -2724,13 +2724,18 @@ ETEXI > > DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg, > "-fw_cfg [name=]<name>,file=<file>\n" > - " add named fw_cfg entry from file\n", > + " add named fw_cfg entry from file\n" > + "-fw_cfg [name=]<name>,content=<str>\n" > + " add named fw_cfg entry from string\n", Looks good. I got worried for a second that spelling out -fw_cfg twice is not consistent with the rest of the documentation, but there are many other such examples (-smbios, -netdev, -net, -chardev etc). > QEMU_ARCH_ALL) > STEXI > @item -fw_cfg [name=]@var{name},file=@var{file} > @findex -fw_cfg > Add named fw_cfg entry from file. @var{name} determines the name of > the entry in the fw_cfg file directory exposed to the guest. > + > +@item -fw_cfg [name=]@var{name},content=@var{str} > +Add named fw_cfg entry from string. > ETEXI > > DEF("serial", HAS_ARG, QEMU_OPTION_serial, \ Looks good. I checked with -chardev, and indeed @findex stands only after the first occurrence of the option. > diff --git a/vl.c b/vl.c > index e211f6a..7f35f40 100644 > --- a/vl.c > +++ b/vl.c > @@ -512,6 +512,10 @@ static QemuOptsList qemu_fw_cfg_opts = { > .type = QEMU_OPT_STRING, > .help = "Sets the name of the file from which\n" > "the fw_cfg blob will be loaded", > + }, { > + .name = "content", > + .type = QEMU_OPT_STRING, > + .help = "Sets the content of the fw_cfg blob to be inserted", > }, > { /* end of list */ } > }, > @@ -2236,7 +2240,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > { > gchar *buf; > size_t size; > - const char *name, *file; > + const char *name, *file, *content; > > if (opaque == NULL) { > error_report("fw_cfg device not available"); > @@ -2244,8 +2248,17 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > } > name = qemu_opt_get(opts, "name"); > file = qemu_opt_get(opts, "file"); > - if (name == NULL || *name == '\0' || file == NULL || *file == '\0') { > - error_report("invalid argument value"); > + content = qemu_opt_get(opts, "content"); > + > +#define HAVE_OPT(arg) ((arg) && (*arg)) Not sure if this is usual in QEMU, but if it is, please also #undef the macro after you are done using it. Also, I recommend renaming HAVE_OPT() to NONEMPTY_STR(). > + > + /* we need name and either a file or the content string */ > + if (!(HAVE_OPT(name) && (HAVE_OPT(file) || HAVE_OPT(content)))) { > + error_report("invalid argument(s)!"); Please drop the exclamation mark. > + return -1; > + } > + if (HAVE_OPT(file) && HAVE_OPT(content)) { > + error_report("file and content are mutually exclusive!"); Ditto. > return -1; > } > if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { > @@ -2256,9 +2269,14 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > error_report("WARNING: externally provided fw_cfg item names " > "should be prefixed with \"opt/\"!"); > } > - if (!g_file_get_contents(file, &buf, &size, NULL)) { > - error_report("can't load %s", file); > - return -1; > + if (HAVE_OPT(content)) { > + buf = g_strdup(content); > + size = strlen(buf); I wonder if we should really duplicate the content here. I think we could get away with just linking the option string into fw_cfg. But, for consistency with the other branch, I don't mind. :) Also, strlen() is okay (it will not expose the terminating NUL to the guest), but I think we shouldn't *allocate* / duplicate that NUL either. g_strdup() does though. How about calling strlen() first, and then replacing g_strdup() with g_memdup()? Not crazy important though. > + } else { > + if (!g_file_get_contents(file, &buf, &size, NULL)) { > + error_report("can't load %s", file); > + return -1; > + } > } > fw_cfg_add_file((FWCfgState *)opaque, name, buf, size); > return 0; > Just small nits; looks okay to me otherwise. Thank you for coding this up (and thanks Jordan for the suggestion), because this way we can insert "-fw_cfg name=opt/ovmf/PcdXXX,content=Y" in a libvirt domain XML directly, using <qemu:arg>, without referencing external files. Thanks! Laszlo
On Mon, Sep 28, 2015 at 03:30:28PM +0200, Laszlo Ersek wrote: > On 09/27/15 00:55, Gabriel L. Somlo wrote: > > Allow users to provide custom fw_cfg blobs with ascii string > > payloads specified directly on the qemu command line. > > > > Suggested-by: Jordan Justen <jordan.l.justen@intel.com> > > Suggested-by: Laszlo Ersek <lersek@redhat.com> > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > > --- > > docs/specs/fw_cfg.txt | 5 +++++ > > qemu-options.hx | 7 ++++++- > > vl.c | 30 ++++++++++++++++++++++++------ > > 3 files changed, 35 insertions(+), 7 deletions(-) > > > > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt > > index b5f4b5d..4d85701 100644 > > --- a/docs/specs/fw_cfg.txt > > +++ b/docs/specs/fw_cfg.txt > > @@ -236,6 +236,11 @@ the following syntax: > > where <item_name> is the fw_cfg item name, and <path> is the location > > on the host file system of a file containing the data to be inserted. > > > > +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.) > > So something like, "... the content presented to the guest will not be > NUL-terminated, same as if the string was written to a host-side file > first". > > Please also stress that such content (and actually name too, but that's > more generic) should be plain ASCII; let's sidestep any tricks a shell's > locale settings could pull on us. OK, will do. > > NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" > > when using the "-fw_cfg" command line option, to avoid conflicting with > > item names used internally by QEMU. For instance: > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 328404c..0b6f5bd 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -2724,13 +2724,18 @@ ETEXI > > > > DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg, > > "-fw_cfg [name=]<name>,file=<file>\n" > > - " add named fw_cfg entry from file\n", > > + " add named fw_cfg entry from file\n" > > + "-fw_cfg [name=]<name>,content=<str>\n" > > + " add named fw_cfg entry from string\n", > > Looks good. I got worried for a second that spelling out -fw_cfg twice > is not consistent with the rest of the documentation, but there are many > other such examples (-smbios, -netdev, -net, -chardev etc). > > > QEMU_ARCH_ALL) > > STEXI > > @item -fw_cfg [name=]@var{name},file=@var{file} > > @findex -fw_cfg > > Add named fw_cfg entry from file. @var{name} determines the name of > > the entry in the fw_cfg file directory exposed to the guest. > > + > > +@item -fw_cfg [name=]@var{name},content=@var{str} > > +Add named fw_cfg entry from string. > > ETEXI > > > > DEF("serial", HAS_ARG, QEMU_OPTION_serial, \ > > Looks good. I checked with -chardev, and indeed @findex stands only > after the first occurrence of the option. > > > diff --git a/vl.c b/vl.c > > index e211f6a..7f35f40 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -512,6 +512,10 @@ static QemuOptsList qemu_fw_cfg_opts = { > > .type = QEMU_OPT_STRING, > > .help = "Sets the name of the file from which\n" > > "the fw_cfg blob will be loaded", > > + }, { > > + .name = "content", > > + .type = QEMU_OPT_STRING, > > + .help = "Sets the content of the fw_cfg blob to be inserted", > > }, > > { /* end of list */ } > > }, > > @@ -2236,7 +2240,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > > { > > gchar *buf; > > size_t size; > > - const char *name, *file; > > + const char *name, *file, *content; > > > > if (opaque == NULL) { > > error_report("fw_cfg device not available"); > > @@ -2244,8 +2248,17 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > > } > > name = qemu_opt_get(opts, "name"); > > file = qemu_opt_get(opts, "file"); > > - if (name == NULL || *name == '\0' || file == NULL || *file == '\0') { > > - error_report("invalid argument value"); > > + content = qemu_opt_get(opts, "content"); > > + > > +#define HAVE_OPT(arg) ((arg) && (*arg)) > > Not sure if this is usual in QEMU, but if it is, please also #undef the > macro after you are done using it. > > Also, I recommend renaming HAVE_OPT() to NONEMPTY_STR(). I was trying to improve clarity by factoring this out before things get too crazy when I start checking for combinations of available options below... :) Maybe I should use an inline function instead, that'd get me out of the #define/#undef business... > > + > > + /* we need name and either a file or the content string */ > > + if (!(HAVE_OPT(name) && (HAVE_OPT(file) || HAVE_OPT(content)))) { > > + error_report("invalid argument(s)!"); > > Please drop the exclamation mark. > > > + return -1; > > + } > > + if (HAVE_OPT(file) && HAVE_OPT(content)) { > > + error_report("file and content are mutually exclusive!"); > > Ditto. > > > return -1; > > } > > if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { > > @@ -2256,9 +2269,14 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > > error_report("WARNING: externally provided fw_cfg item names " > > "should be prefixed with \"opt/\"!"); > > } > > - if (!g_file_get_contents(file, &buf, &size, NULL)) { > > - error_report("can't load %s", file); > > - return -1; > > + if (HAVE_OPT(content)) { > > + buf = g_strdup(content); > > + size = strlen(buf); > > I wonder if we should really duplicate the content here. I think we > could get away with just linking the option string into fw_cfg. > > But, for consistency with the other branch, I don't mind. :) > > Also, strlen() is okay (it will not expose the terminating NUL to the > guest), but I think we shouldn't *allocate* / duplicate that NUL either. > g_strdup() does though. > > How about calling strlen() first, and then replacing g_strdup() with > g_memdup()? > > Not crazy important though. Leaning toward dropping the strdup/memdup and using the original content string... That has the advantage of keeping things simple :) Thanks for pointing it out ! > > > + } else { > > + if (!g_file_get_contents(file, &buf, &size, NULL)) { > > + error_report("can't load %s", file); > > + return -1; > > + } > > } > > fw_cfg_add_file((FWCfgState *)opaque, name, buf, size); > > return 0; > > > > Just small nits; looks okay to me otherwise. Thank you for coding this > up (and thanks Jordan for the suggestion), because this way we can > insert "-fw_cfg name=opt/ovmf/PcdXXX,content=Y" in a libvirt domain XML > directly, using <qemu:arg>, without referencing external files. Thanks for the comments, I'll send out a v2 later today. --Gabriel
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=).
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 :) Thanks much, --Gabriel
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. 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. 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.
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. Thanks! Laszlo
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. Thanks Laszlo
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. Thanks, --Gabriel
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. :) Laszlo
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 :)
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
diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt index b5f4b5d..4d85701 100644 --- a/docs/specs/fw_cfg.txt +++ b/docs/specs/fw_cfg.txt @@ -236,6 +236,11 @@ the following syntax: where <item_name> is the fw_cfg item name, and <path> is the location on the host file system of a file containing the data to be inserted. +Small enough items may be provided directly as strings on the command +line, using the syntax: + + -fw_cfg [name=]<item_name>,content=<string> + NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" when using the "-fw_cfg" command line option, to avoid conflicting with item names used internally by QEMU. For instance: diff --git a/qemu-options.hx b/qemu-options.hx index 328404c..0b6f5bd 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2724,13 +2724,18 @@ ETEXI DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg, "-fw_cfg [name=]<name>,file=<file>\n" - " add named fw_cfg entry from file\n", + " add named fw_cfg entry from file\n" + "-fw_cfg [name=]<name>,content=<str>\n" + " add named fw_cfg entry from string\n", QEMU_ARCH_ALL) STEXI @item -fw_cfg [name=]@var{name},file=@var{file} @findex -fw_cfg Add named fw_cfg entry from file. @var{name} determines the name of the entry in the fw_cfg file directory exposed to the guest. + +@item -fw_cfg [name=]@var{name},content=@var{str} +Add named fw_cfg entry from string. ETEXI DEF("serial", HAS_ARG, QEMU_OPTION_serial, \ diff --git a/vl.c b/vl.c index e211f6a..7f35f40 100644 --- a/vl.c +++ b/vl.c @@ -512,6 +512,10 @@ static QemuOptsList qemu_fw_cfg_opts = { .type = QEMU_OPT_STRING, .help = "Sets the name of the file from which\n" "the fw_cfg blob will be loaded", + }, { + .name = "content", + .type = QEMU_OPT_STRING, + .help = "Sets the content of the fw_cfg blob to be inserted", }, { /* end of list */ } }, @@ -2236,7 +2240,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) { gchar *buf; size_t size; - const char *name, *file; + const char *name, *file, *content; if (opaque == NULL) { error_report("fw_cfg device not available"); @@ -2244,8 +2248,17 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) } name = qemu_opt_get(opts, "name"); file = qemu_opt_get(opts, "file"); - if (name == NULL || *name == '\0' || file == NULL || *file == '\0') { - error_report("invalid argument value"); + content = qemu_opt_get(opts, "content"); + +#define HAVE_OPT(arg) ((arg) && (*arg)) + + /* we need name and either a file or the content string */ + if (!(HAVE_OPT(name) && (HAVE_OPT(file) || HAVE_OPT(content)))) { + error_report("invalid argument(s)!"); + return -1; + } + if (HAVE_OPT(file) && HAVE_OPT(content)) { + error_report("file and content are mutually exclusive!"); return -1; } if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) { @@ -2256,9 +2269,14 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) error_report("WARNING: externally provided fw_cfg item names " "should be prefixed with \"opt/\"!"); } - if (!g_file_get_contents(file, &buf, &size, NULL)) { - error_report("can't load %s", file); - return -1; + if (HAVE_OPT(content)) { + buf = g_strdup(content); + size = strlen(buf); + } else { + if (!g_file_get_contents(file, &buf, &size, NULL)) { + error_report("can't load %s", file); + return -1; + } } fw_cfg_add_file((FWCfgState *)opaque, name, buf, size); return 0;
Allow users to provide custom fw_cfg blobs with ascii string payloads specified directly on the qemu command line. Suggested-by: Jordan Justen <jordan.l.justen@intel.com> Suggested-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- docs/specs/fw_cfg.txt | 5 +++++ qemu-options.hx | 7 ++++++- vl.c | 30 ++++++++++++++++++++++++------ 3 files changed, 35 insertions(+), 7 deletions(-)