On Mon, Jun 01, 2015 at 09:06:19AM +0200, Laszlo Ersek wrote:
> On 05/31/15 20:10, Michael S. Tsirkin wrote:
> > On Wed, Apr 29, 2015 at 11:21:53AM -0400, Gabriel L. Somlo wrote:
> >> Allow user supplied files to be inserted into the fw_cfg
> >> device before starting the guest. Since fw_cfg_add_file()
> >> already disallows duplicate fw_cfg file names, qemu will
> >> exit with an error message if the user supplies multiple
> >> blobs with the same fw_cfg file name, or if a blob name
> >> collides with a fw_cfg name programmatically added from
> >> within the QEMU source code. A warning message will be
> >> printed if the fw_cfg item name does not begin with the
> >> prefix "opt/", which is recommended for external, user
> >> provided blobs.
> >>
> >> Signed-off-by: Gabriel Somlo <so...@cmu.edu>
> >> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> >
> > Would it make sense to make an illegal prefix a hard failure
> > instead?
> > If we don't, we'll be unable to add new file names without
> > fear of conflicts in the future.
>
> We discussed this earlier. The idea was "mechanism, not policy", and
> that if a user violates the convention (not rule) / ignores the warning
> at some point, he's on his own when it breaks in the next version.
>
> I don't feel overly strongly about it; just "mechanism, not policy"
> looks like a good tradition (well, good excuse anyway).
>
> Thanks
> Laszlo
Most users never see warnings. We ship it, we support it.
If we don't want to support it, let's not ship it.
> >> ---
> >> docs/specs/fw_cfg.txt | 21 +++++++++++++++++
> >> qemu-options.hx | 11 +++++++++
> >> vl.c | 63
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 95 insertions(+)
> >>
> >> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> >> index 6accd92..74351dd 100644
> >> --- a/docs/specs/fw_cfg.txt
> >> +++ b/docs/specs/fw_cfg.txt
> >> @@ -203,3 +203,24 @@ completes fully overwriting the item's data.
> >>
> >> NOTE: This function is deprecated, and will be completely removed
> >> starting with QEMU v2.4.
> >> +
> >> +== Externally Provided Items ==
> >> +
> >> +As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
> >> +FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
> >> +directory structure) may be inserted via the QEMU command line, using
> >> +the following syntax:
> >> +
> >> + -fw_cfg [name=]<item_name>,file=<path>
> >> +
> >> +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.
> >> +
> >> +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:
> >> +
> >> + -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> >> +
> >> +Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> >> +"opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 319d971..aa386b4 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -2668,6 +2668,17 @@ STEXI
> >> @table @option
> >> ETEXI
> >>
> >> +DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> >> + "-fw_cfg [name=]<name>,file=<file>\n"
> >> + " add named fw_cfg entry from file\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.
> >> +ETEXI
> >> +
> >> DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
> >> "-serial dev redirect the serial port to char device 'dev'\n",
> >> QEMU_ARCH_ALL)
> >> diff --git a/vl.c b/vl.c
> >> index 74c2681..b02b2d4 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
> >> },
> >> };
> >>
> >> +static QemuOptsList qemu_fw_cfg_opts = {
> >> + .name = "fw_cfg",
> >> + .implied_opt_name = "name",
> >> + .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
> >> + .desc = {
> >> + {
> >> + .name = "name",
> >> + .type = QEMU_OPT_STRING,
> >> + .help = "Sets the fw_cfg name of the blob to be inserted",
> >> + }, {
> >> + .name = "file",
> >> + .type = QEMU_OPT_STRING,
> >> + .help = "Sets the name of the file from which\n"
> >> + "the fw_cfg blob will be loaded",
> >> + },
> >> + { /* end of list */ }
> >> + },
> >> +};
> >> +
> >> /**
> >> * Get machine options
> >> *
> >> @@ -2118,6 +2137,38 @@ char *qemu_find_file(int type, const char *name)
> >> return NULL;
> >> }
> >>
> >> +static int parse_fw_cfg(QemuOpts *opts, void *opaque)
> >> +{
> >> + gchar *buf;
> >> + size_t size;
> >> + const char *name, *file;
> >> +
> >> + if (opaque == NULL) {
> >> + error_report("fw_cfg device not available");
> >> + return -1;
> >> + }
> >> + 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");
> >> + return -1;
> >> + }
> >> + if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> >> + error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH
> >> - 1);
> >> + return -1;
> >> + }
> >> + if (strncmp(name, "opt/", 4) != 0) {
> >> + 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;
> >> + }
> >> + fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
> >> + return 0;
> >> +}
> >> +
> >> static int device_help_func(QemuOpts *opts, void *opaque)
> >> {
> >> return qdev_device_help(opts);
> >> @@ -2806,6 +2857,7 @@ int main(int argc, char **argv, char **envp)
> >> qemu_add_opts(&qemu_numa_opts);
> >> qemu_add_opts(&qemu_icount_opts);
> >> qemu_add_opts(&qemu_semihosting_config_opts);
> >> + qemu_add_opts(&qemu_fw_cfg_opts);
> >>
> >> runstate_init();
> >>
> >> @@ -3422,6 +3474,12 @@ int main(int argc, char **argv, char **envp)
> >> }
> >> do_smbios_option(opts);
> >> break;
> >> + case QEMU_OPTION_fwcfg:
> >> + opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg,
> >> 1);
> >> + if (opts == NULL) {
> >> + exit(1);
> >> + }
> >> + break;
> >> case QEMU_OPTION_enable_kvm:
> >> olist = qemu_find_opts("machine");
> >> qemu_opts_parse(olist, "accel=kvm", 0);
> >> @@ -4233,6 +4291,11 @@ int main(int argc, char **argv, char **envp)
> >>
> >> numa_post_machine_init();
> >>
> >> + if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> >> + parse_fw_cfg, fw_cfg_find(), 1) != 0) {
> >> + exit(1);
> >> + }
> >> +
> >> /* init USB devices */
> >> if (usb_enabled()) {
> >> if (foreach_device_config(DEV_USB, usb_parse) < 0)
> >> --
> >> 2.1.0
> >>
