Message ID | 20200528173141.17495-4-philmd@redhat.com |
---|---|
State | New |
Headers | show |
Series | fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites | expand |
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
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)) { + /* + * 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/\""); }
User-generated fw_cfg keys should be prefixed with "opt/". However FW_CFG_DATA_GENERATOR keys are generated by QEMU, 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(-)