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

[Markus Armbruster]

Interface and doc review only.  Sorry for the delay, I was on vacation.

"Michael S. Tsirkin" <address@hidden> writes:

> This requires that all -fw_cfg command line users use names of the form

What's "this"?  Do you mean "Require that ..."?

> opt/RFQDN/: such names are compatible with QEMU 2.4 and 2.5 as well as
> future QEMU versions.

I'd reserve names with dots before the first '/' for users, and use
RFQDN/ without opt/.  But I can live with opt/.

>
> 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>
> Reviewed-by: Laszlo Ersek <address@hidden>
> ---
>
> I put this in my tree for now. If anyone has an issue with this,
> please speak up!
[...]
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5414140..41ce9ca 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.
>  

My first reaction was "wait a second, why are you throwing away useful
information?"  I had to read on to realize you're *moving* it, to
qemu-options.hx.  That's fine.  Announcing such moves in the commit
message makes review easier.

> -NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> +Users MUST choose item names beginning with the prefix "opt/RFQDN/"

Preexisting: no need to shout.

>  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.mycompany/guestagent/guestblob,file=./my_blob.bin

Neglects to spell out the obvious: RFQDN is a reverse fully qualified
domain name which you control.

I'm not sure I agree with the change from "should" to "must".  What
happens when a user uses a conflicting name?  Can you categorically rule
out that specifying a conflicting name could ever be useful, e.g. for
debugging or testing purposes?  If not, then "must" is wrong.

> -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().

Here, "must not" is appropriate.

> +
> +For historical reasons "opt/ovmf/" is reserved for use with the OVMF 
> firmware.

Comma after reasons.

Here's how I'd skin this cat:

    Names beginning with "opt/" are reserved for users.  QEMU will never
    create entries with such names unless explicitly ordered by the
    user.

    To avoid clashes among different users, it is strongly recommended
    that you use names beginning with opt/RFQDN/, where RFQDN is a
    reverse fully qualified domain name you control.  For instance, if
    SeaBIOS wanted to define additional names, prefix "opt/org.seabios/"
    would be appropriate.

    For historical reasons, "opt/ovmf/" is reserved for 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.

What exactly does making users say "unsupported/root/FOO" instead of
"FOO" buy us?

We can warn on "-fw_cfg FOO" just as easily as on "-fw_cfg
/unsupported/root/FOO".

>                                                            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.

We should tell users that messing with stuff outside /opt is
unsupported.  But telling them that only developers get to do
unsupported things is futile.

My take:

    Use of names not beginning with "opt/" is potentially dangerous and
    entirely unsupported.  QEMU will warn if you try.

> +
> +Any use of the prefix "opt/org.qemu" is reserved for future use.
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 587de8f..2109a4f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2864,18 +2864,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"

Not a native speaker, but I'd say "contents" rather than "content".
More of the same below.

>      "-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.

Isn't "up to first NUL" self-evident for strings, at least in the
context of the QEMU command line?

> +
> +The terminating NUL character of the content @var{str} will NOT be

of @var{str}

> +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

To insert contents with embedded NUL characters, ...

> +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.
> +
> address@hidden must be in the format opt/RFQDN/<item_name>.
> +
> +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

Sure your markup as table of one element is appropriate?  The resulting
qemu.1 looks bad.  Try "man ./qemu.1" to see.

I understand you're merely moving stuff, but here goes anyway: "any
processing of @var{name} values (potentially performed e.g., by the
shell) is outside of QEMU's responsibility" is confusing, and once you
understand it, trivial.  Shell meta-characters getting interpreted by
the shell isn't news, and not specific to -fw_cfg...

> +
> +Example:
> address@hidden
> +    -fw_cfg name=opt/com.mycompany/guestagent/guestblob,file=./my_blob.bin
> 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
> +unsupported and can be removed at any point.
> +
>  ETEXI
>  
>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \