Re: [Qemu-devel] [PATCH] fw_cfg: RQFN rules, documentation

[Laszlo Ersek]

On 04/04/16 12:51, Michael S. Tsirkin wrote:
> This requires that all -fw_cfg command line users use names of the form
> opt/RQFN/: such names are compatible with QEMU 2.4 and 2.5 as well as
> future QEMU versions.

I think "RQFN" is a typo; shouldn't it be RFQDN (reverse fully qualified
domain name)? In the subject too.

> 
> As ability to insert fw_cfg entries in QEMU root is useful for
> firmware development, add a special prefix: unsupported/root/ that
> allows that, while making sure users are aware it's unsupported.
> 
> Cc: Gerd Hoffmann <address@hidden>
> Cc: Gabriel L. Somlo <address@hidden>
> Cc: Laszlo Ersek <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
> 
> This summarizes the discussions we had on list.
> 
>  vl.c                  | 40 ++++++++++++++++++++++++++++++++++++----
>  docs/specs/fw_cfg.txt | 34 +++++++++++++++++-----------------
>  qemu-options.hx       | 38 +++++++++++++++++++++++++++++++++-----
>  3 files changed, 86 insertions(+), 26 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 2200e62..af9c9d6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2296,8 +2296,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>  {
>      gchar *buf;
>      size_t size;
> -    const char *name, *file, *str;
> +    const char *name, *file, *str, *slash, *dot;
>      FWCfgState *fw_cfg = (FWCfgState *) opaque;
> +    const char qemu_prefix[] = "opt/org.qemu";
> +    const char unsupported_root_prefix[] = "unsupported/root/";

Side point: "static" for these two?

>  
>      if (fw_cfg == NULL) {
>          error_report("fw_cfg device not available");
> @@ -2320,9 +2322,39 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>          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/\"");
> +    /*
> +     * Look for and strip unsupported_root_prefix, which is useful for 
> firmware
> +     * development, but warn users.
> +     */
> +    if (!strncmp(name, unsupported_root_prefix, 
> strlen(unsupported_root_prefix))) {

Side point: how about using sizeof ... -1 instead of strlen() here?

> +        error_report("warning: removing prefix \"%s\". "
> +                     "Guest or QEMU may crash. "
> +                     "Names must be prefixed with \"opt/RQFN/\"",
> +                     unsupported_root_prefix);

RFQDN

> +        name += strlen(unsupported_root_prefix);

sizeof?

> +        if (!(nonempty_str(name))) {

Does the QEMU coding style suggest

  !(nonempty_str(name))

over

  !nonempty_str(name)

?

> +            error_report("invalid argument(s)");
> +            return -1;

Hmm, not sure about two separate error_report() calls. Could be okay;
I'm getting confused by the hint thing. Markus?

> +        }
> +    } else {
> +        /*
> +         * Don't attempt to validate a valid RQFN in name, as that's not 
> easy:

RFQDN

> +         * we do validate that it includes '.' .
> +         */
> +        if (strncmp(name, "opt/", 4) ||
> +            !((dot = strchr(name + 4, '.'))) ||
> +            !((slash = strchr(name + 4, '/'))) ||

I don't think the double parens are necessary. Does gcc complain
otherwise? (Sigh...)

> +            dot > slash) {
> +            error_report("error: externally provided fw_cfg item names "
> +                         "must be prefixed with \"opt/RQFN/\"");

RFQDN

> +            return -1;
> +        }
> +        if (!strncmp(name, qemu_prefix, strlen(qemu_prefix))) {

sizeof?

> +            error_report("error: externally provided fw_cfg item names "
> +                         "must not use the reserved prefix \"%s\"",
> +                         qemu_prefix);
> +            return -1;
> +        }
>      }

I believe these checks will reject the historical "opt/ovmf/" prefix,
documented below. (Specifically, the dot check.)

>      if (nonempty_str(str)) {
>          size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5414140..83b5e80 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -210,29 +210,29 @@ 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.
> -
> -Small enough items may be provided directly as strings on the command
> -line, using the syntax:
> +Or
>  
>      -fw_cfg [name=]<item_name>,string=<string>
>  
> -The terminating NUL character of the content <string> will NOT be
> -included as part of the fw_cfg item data, which is consistent with
> -the absence of a NUL terminator for items inserted via the file option.
> +See QEMU man page for more documentation.
>  
> -Both <item_name> and, if applicable, the content <string> are passed
> -through by QEMU without any interpretation, expansion, or further
> -processing. Any such processing (potentially performed e.g., by the shell)
> -is outside of QEMU's responsibility; as such, using plain ASCII characters
> -is recommended.
> +Using item_name with plain ASCII characters only is recommended.
>  
> -NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> +Users MUST choose item names beginning with the prefix "opt/RQFN/"

RFQDN

>  when using the "-fw_cfg" command line option, to avoid conflicting with
> -item names used internally by QEMU. For instance:
> +item names used internally by QEMU, or by firmware. For instance:
>  
> -    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> +    -fw_cfg name=opt/com.my_company/guestagent/guestblob,file=./my_blob.bin
>  
> -Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> +Similarly, QEMU developers MUST NOT use item names prefixed with
>  "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
> +
> +For historical reasons "opt/ovmf/" is reserved for use with the OVMF 
> firmware.
> +
> +To simplify guest firmware development, the prefix
> +unsupported/root/ is automatically stripped from paths, which
> +allows creating fw_cfg files in the root QEMU directory.  This interface is
> +strictly for use by developers, its use can cause guest or QEMU crashes, is
> +unsupported and can be removed at any point.
> +

Okay, so you listed three groups of people in the discussion:
- QEMU developers
- QEMU firmware developers
- users

QEMU developers shall use stuff outside of "opt/" (and in the future,
maybe under "opt/org.qemu/"). Okay.

Users shall use "opt/com.my_company/..." style stuff; okay as well.

QEMU firmware developers will use "unsupported/root/..." when they want
to mess with the firmware in connection with fw_cfg files that QEMU
itself may expose under some circumstances. Okay.

Going forward, QEMU firmware developers shall use -- talking specifics
now -- "opt/org.tianocore.edk2.ovmf/..." and "opt/org.seabios/..."
pathnames for genuine firmware settings that QEMU doesn't / shouldn't
populate itself, but users might want to. Is that right?

... My question is, do we need the "opt/" prefix at all (for the future,
i.e., the non-historical cases)? Looking at the last discussion, I
believe we converged on:

- QEMU devs (future filenames): org.qemu/...
- users:                        com.my_company/...
- QEMU fw devs (future names):  org.tianocore.edk2.ovmf/...
                                org.seabios/...
- QEMU fw devs hacking:         <root-prefix-to-strip>/...

Did you find something unsafe about this (necessitating "opt/")?

> +Any use of the prefix "opt/org.qemu/" is reserved for future use.

This does not exactly match the code; the code will also reject
"opt/org.qemu1234" and "opt/org.qemu.why.not/...".

I guess it's probably the code that reflects your intent, so the docs
should be adapted.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index a770086..1af28ac 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2860,18 +2860,46 @@ ETEXI
>  
>  DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
>      "-fw_cfg [name=]<name>,file=<file>\n"
> -    "                add named fw_cfg entry from file\n"
> +    "                add named fw_cfg entry with content from file\n"
>      "-fw_cfg [name=]<name>,string=<str>\n"
> -    "                add named fw_cfg entry from string\n",
> +    "                add named fw_cfg entry with content from string\n",
>      QEMU_ARCH_ALL)
>  STEXI
> +
>  @item -fw_cfg address@hidden,address@hidden
>  @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.
> +Add named fw_cfg entry with content from file @var{file}.
>  
>  @item -fw_cfg address@hidden,address@hidden
> -Add named fw_cfg entry from string.
> +Add named fw_cfg entry with content from string @var{str}, up to the first 
> NUL character.
> +
> +The terminating NUL character of the content @var{str} will NOT be
> +included as part of the fw_cfg item data. To insert content including
> +the NUL character, store it in file and insert the item via
> +the @var{file} option.
> +
> +Both the name and the content are passed by QEMU through to the guest, where:
> address@hidden @option
> address@hidden @var{name} determines the name of the entry in the fw_cfg file 
> directory exposed to the guest.

Isn't this line overlong? (Maybe it's required by @table, I don't know.)

> +
> address@hidden must be in the format opt/RQFN/<item_name>.

RFQDN

> +
> +Any processing of @var{name} values (potentially performed e.g., by the 
> shell)
> +is outside of QEMU's responsibility; as such, using plain ASCII characters is
> +recommended.
> address@hidden table
> +
> +Example:
> address@hidden
> +    -fw_cfg opt/com.mycompany/guestagent/guestblob,file=./my_blob.bin

The example doesn't exactly match the one in the specs file ("name=" and
the underscore in "my_company" are missing). Not too important, but the
"guestagent" string suggests we might want to be consistent here.

> address@hidden example
> +
> +To simplify guest firmware development, the prefix
> +unsupported/root/ is automatically stripped from paths, which
> +allows creating fw_cfg files in the root QEMU directory.  This interface is
> +strictly for use by developers, its use can cause Guest or QEMU crashes, is

s/Guest/guest/?

> +unsupported and can be removed at any point.
> +
>  ETEXI
>  
>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
> 

Thanks
Laszlo