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. > 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