[Qemu-devel] How to emit errors with nice location information (was: [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline)

[Markus Armbruster]

"Gabriel L. Somlo" <address@hidden> 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().

[...]