Re: [RFC PATCH v7 3/5] softmmu/vl: Allow -fw_cfg 'gen_id' option to use the 'etc/' namespace

[Laszlo Ersek]

On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> User-generated fw_cfg keys should be prefixed with "opt/".

(1) Please formulate this as follows:

'Names of user-provided fw_cfg items are supposed to start with "opt/".'

(Because we're really not "prefixing keys".)

> However FW_CFG_DATA_GENERATOR keys are generated by QEMU,

(2) s/keys/items/

> so allow the "etc/" namespace in this specific case.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v7: reword commit description and added comment in code
> ---
>  softmmu/vl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index cdb1d187ed..d5423eaf2b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2049,7 +2049,13 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>                     FW_CFG_MAX_FILE_PATH - 1);
>          return -1;
>      }
> -    if (strncmp(name, "opt/", 4) != 0) {
> +    if (!nonempty_str(gen_id)) {

(3) I think this condition should be inverted. We'd like to suppress the
warning when "gen_id" is specified. In that case, nonempty_str(gen_id)
returns "true".

In other words, please drop the "!" operator.

> +        /*
> +         * In this particular case where the content is populated
> +         * internally, the "etc/" namespace protection is relaxed,
> +         * so do not emit a warning.
> +         */
> +    } else if (strncmp(name, "opt/", 4) != 0) {
>          warn_report("externally provided fw_cfg item names "
>                      "should be prefixed with \"opt/\"");
>      }
>

(4) I think having this in a separate patch is nice; I agree we should
do this. But I'd like to request a small update to
"docs/specs/fw_cfg.txt" as well, in the same patch.

Namely, where the document says:

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

Please append:

"""
Use of names not beginning with "opt/" is tolerated with 'gen_id' (that
is, the warning is suppressed), but you must know exactly what you're
doing.
"""

Because this highlights that the user (or the management tool) *actively
participates* in connecting the content generated by QEMU with the
fw_cfg filename expected by the firmware.

With (1) through (4) fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo