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

[Gabriel L. Somlo]

On Tue, Mar 17, 2015 at 12:28:20PM +0100, Laszlo Ersek wrote:
> > +
> > +void fw_cfg_option_add(QemuOpts *opts)
> > +{
> > +    const char *name = qemu_opt_get(opts, "name");
> > +    const char *file = qemu_opt_get(opts, "file");
> > +
> > +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> > +        error_report("invalid argument value");
> > +        exit(1);
> > +    }
> 
> Okay, I don't recall the details of what I'm going to recommend. :)
> 
> Please use the location API to tie the error message to the offending
> QemuOpts. I've done that only once before, but it didn't turn out to be
> a catastrophe, so now I'm recommending it to you as well. See commit
> 637a5acb; specifically the code around the "Location" object.

I don't think that's applicable here. fw_cfg_option_add() is called
from v.c:

@@ -3420,6 +3440,13 @@ 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, 0);
+                if (opts == NULL) {
+                    exit(1);
+                }
+                fw_cfg_option_add(opts);
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=kvm", 0);

...and, as such, I'm exactly in the appropriate location for
error_report() to work as expected. In fact, in an earlier reply to
Matt, I quoted an example of what the error message looks like:

qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value

...which shows it works the way we'd want it to.

> (CC'ing Markus.)
> 
> > +static void fw_cfg_options_insert(FWCfgState *s)
> > +{
> > +    int i, filesize;
> > +    const char *filename;
> > +    void *filebuf;
> > +
> > +    for (i = 0; i < fw_cfg_num_options; i++) {
> > +        filename = fw_cfg_options[i].file;
> > +        filesize = get_image_size(filename);
> > +        if (filesize == -1) {
> > +            error_report("Cannot read fw_cfg file %s", filename);
> > +            exit(1);
> > +        }
> > +        filebuf = g_malloc(filesize);
> > +        if (load_image(filename, filebuf) != filesize) {
> > +            error_report("Failed to load fw_cfg file %s", filename);

Now, *this* is where I could use the stashed copy of 'QemuOpts *opts'
from fw_cfg_add_option() to tie the error report back to the "bad"
command line component :) That would work if we decided it's OK to
delay everything, including parsing '*opts' with qemu_opt_get(), until
this point.

> > +            exit(1);
> > +        }
> > +        fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize);
> > +    }
> > +}

Guess I'm going to wait on some feedback on whether to stash or not to
stash QemuOpts first...

Thanks much,
--Gabriel