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
