Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek <lersek@redhat.com>]

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 <somlo@cmu.edu>
>> Reviewed-by: Laszlo Ersek <lersek@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

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