"Gabriel L. Somlo" <gso...@gmail.com> writes:
> 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.
Yup.
>> (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.
error_report() prints the "current location". This is actually the top
of a location stack. The stack initially contains just one LOC_NONE
entry, which makes error_report() print no location. main()'s option
processing puts the current option's location on the stack. So do
qemu_config_parse() and qemu_opt_foreach().
If you need to report an error later on, you have to do it yourself
(unless you're using qemu_opt_foreach()). Sadly, we too often can't be
bothered.
The general pattern is
push a location in one of the several ways
do stuff, maybe call error_report()
pop the location with loc_pop()
To find concrete examples, search for loc_pop().
I can see two involving QemuOpts: scsi_bus_legacy_handle_cmdline() and
pc_system_flash_init(). The former is easier to understand, because it
does much less.
A helper error_report_for_opts() could save you the monkeying around
with the location stack. Maybe when we have more than two potential
users.
For a non-QemuOpts example, see foreach_device_config().
[...]
