From: "Michael S. Tsirkin" <mst@redhat.com>

Allowing arbitary file names on command line is setting us up for
failure: future guests will look for a specific QEMU-specified name and
will get confused finding a user file there.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
[Change "warning" to "error" in the error report.]
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 vl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 1ad1c04..fae9300 100644
--- a/vl.c
+++ b/vl.c
@@ -2400,8 +2400,9 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
         return -1;
     }
     if (strncmp(name, "opt/", 4) != 0) {
-        warn_report("externally provided fw_cfg item names "
-                    "should be prefixed with \"opt/\"");
+        error_report("error: externally provided fw_cfg item names "
+                     "should be prefixed with \"opt/\"");
+        return -1;
     }
     if (nonempty_str(str)) {
         size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
-- 
2.7.4

Hi

On Thu, Dec 7, 2017 at 10:30 PM,  <minyard@acm.org> wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> Allowing arbitary file names on command line is setting us up for
> failure: future guests will look for a specific QEMU-specified name and
> will get confused finding a user file there.
>

Why is this part of a IPMI series?
There was a lengthy discussion on this patch earlier, could you
summarize the conclusion?

thanks

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> [Change "warning" to "error" in the error report.]
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  vl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 1ad1c04..fae9300 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2400,8 +2400,9 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>          return -1;
>      }
>      if (strncmp(name, "opt/", 4) != 0) {
> -        warn_report("externally provided fw_cfg item names "
> -                    "should be prefixed with \"opt/\"");
> +        error_report("error: externally provided fw_cfg item names "
> +                     "should be prefixed with \"opt/\"");
> +        return -1;
>      }
>      if (nonempty_str(str)) {
>          size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
> --
> 2.7.4
>
>



-- 
Marc-André Lureau

On 01/12/2018 11:06 AM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Dec 7, 2017 at 10:30 PM,  <minyard@acm.org> wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>
>> Allowing arbitary file names on command line is setting us up for
>> failure: future guests will look for a specific QEMU-specified name and
>> will get confused finding a user file there.
>>
> Why is this part of a IPMI series?

It's been so long I don't remember any more.  I think I suggested a 
simpler fix when
Michael posted his, and it ended up in my queue, because at one point 
the IPMI code
was messing around in fw_cfg.  But you are right, it doesn't belong here.

> There was a lengthy discussion on this patch earlier, could you
> summarize the conclusion?

I think there was no conclusion.  This patch should just be dropped.

Thanks for this and your other comments.

-corey

>
> thanks
>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> [Change "warning" to "error" in the error report.]
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   vl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 1ad1c04..fae9300 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2400,8 +2400,9 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>>           return -1;
>>       }
>>       if (strncmp(name, "opt/", 4) != 0) {
>> -        warn_report("externally provided fw_cfg item names "
>> -                    "should be prefixed with \"opt/\"");
>> +        error_report("error: externally provided fw_cfg item names "
>> +                     "should be prefixed with \"opt/\"");
>> +        return -1;
>>       }
>>       if (nonempty_str(str)) {
>>           size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
>> --
>> 2.7.4
>>
>>
>
>