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.jus...@intel.com> > > Suggested-by: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Gabriel Somlo <so...@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