[Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Michael S. Tsirkin]

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>
---
 vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vl.c b/vl.c
index 7a28982..5654af6 100644
--- a/vl.c
+++ b/vl.c
@@ -2321,6 +2321,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
     if (strncmp(name, "opt/", 4) != 0) {
-        error_report("warning: externally provided fw_cfg item names "
-                     "should be prefixed with \"opt/\"");
+        error_report("externally provided fw_cfg item names "
+                     "must be prefixed with \"opt/\"");
+        return -1;
     }
     if (nonempty_str(str)) {
         size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
-- 
MST

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Paolo Bonzini]

On 15/03/2016 14:55, Michael S. Tsirkin wrote:
> 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>

Too bad for the user.

I think we have already gone through this discussion.

Paolo

> ---
>  vl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/vl.c b/vl.c
> index 7a28982..5654af6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2321,6 +2321,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>      if (strncmp(name, "opt/", 4) != 0) {
> -        error_report("warning: externally provided fw_cfg item names "
> -                     "should be prefixed with \"opt/\"");
> +        error_report("externally provided fw_cfg item names "
> +                     "must be prefixed with \"opt/\"");
> +        return -1;
>      }
>      if (nonempty_str(str)) {
>          size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
> 

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Gerd Hoffmann]

On Di, 2016-03-15 at 15:14 +0100, Paolo Bonzini wrote:
> 
> On 15/03/2016 14:55, Michael S. Tsirkin wrote:
> > 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>
> 
> Too bad for the user.
> 
> I think we have already gone through this discussion.

Indeed, we discussed that before.

If users ignore the warning it's their fault.

Being able to set entries outside opt/ can be useful for debugging and
development purposes, this is the reason it is a warning only and not a
hard error.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 03:14:21PM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/03/2016 14:55, Michael S. Tsirkin wrote:
> > 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>
> 
> Too bad for the user.

Why, because we trapped the user by allowing this
and violating our own compatibility rules?
Because user did not notice the warning?
Do you look at all warnings all your software prints?
What if user uses some tool to build the command line?

consider mem-path - for years we printed a warning if
it's not on hugetlbfs. It worked and users just ignored it.

> I think we have already gone through this discussion.

So now Corey basically is prevented from sorting sanely
because command line might not start with opt/

This is interfering with development instead of enabling it.

If it is really super important for development, add some
option "x-unsupported-fw-cfg" for developers to play with.
Don't allow such things by default, it's a trap.


> Paolo
> 
> > ---
> >  vl.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7a28982..5654af6 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2321,6 +2321,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
> >      if (strncmp(name, "opt/", 4) != 0) {
> > -        error_report("warning: externally provided fw_cfg item names "
> > -                     "should be prefixed with \"opt/\"");
> > +        error_report("externally provided fw_cfg item names "
> > +                     "must be prefixed with \"opt/\"");
> > +        return -1;
> >      }
> >      if (nonempty_str(str)) {
> >          size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
> > 

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 03:25:01PM +0100, Gerd Hoffmann wrote:
> On Di, 2016-03-15 at 15:14 +0100, Paolo Bonzini wrote:
> > 
> > On 15/03/2016 14:55, Michael S. Tsirkin wrote:
> > > 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>
> > 
> > Too bad for the user.
> > 
> > I think we have already gone through this discussion.
> 
> Indeed, we discussed that before.
> 
> If users ignore the warning it's their fault.

Everyone ignores warnings. So it's user's fault for using QEMU.

> Being able to set entries outside opt/ can be useful for debugging and
> development purposes, this is the reason it is a warning only and not a
> hard error.
> 
> cheers,
>   Gerd

Add an unsupported flag there then. We have the rule that
flags starting with x- are for internal purposes, use that.
"x-unsupported-fw-cfg"? Don't trap users into
using a feature and then blaming them.

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Gerd Hoffmann]

  Hi,

> > I think we have already gone through this discussion.
> 
> So now Corey basically is prevented from sorting sanely
> because command line might not start with opt/

Hmm?  There are no guarantees whatsoever if the user used the command
line for entries outside /opt.

Beside that the "sort everything lexical for 2.6+" approach will work
fine.  I think it is more robust and I suspect we will have less hassle
with it long-term.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 03:46:35PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > I think we have already gone through this discussion.
> > 
> > So now Corey basically is prevented from sorting sanely
> > because command line might not start with opt/
> 
> Hmm?  There are no guarantees whatsoever if the user used the command
> line for entries outside /opt.
> 
> Beside that the "sort everything lexical for 2.6+" approach will work
> fine.  I think it is more robust and I suspect we will have less hassle
> with it long-term.
> 
> cheers,
>   Gerd

OK so use built-in order with fallback on lexical, make built-in order
empty for 2.6+.

Is that fine with you?

-- 
MST

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Paolo Bonzini]

On 15/03/2016 15:25, Michael S. Tsirkin wrote:
> Do you look at all warnings all your software prints?

Yes.  I also try to understand them, and if they seem useless I propose
a patch upstream.

> What if user uses some tool to build the command line?
> 
> consider mem-path - for years we printed a warning if
> it's not on hugetlbfs. It worked and users just ignored it.

That was a bad warning though.  There was no reason to warn.

It was also the smallest problem with the original hugetlbfs patches.

> So now Corey basically is prevented from sorting sanely
> because command line might not start with opt/

Can you explain?

Paolo

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 03:54:36PM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/03/2016 15:25, Michael S. Tsirkin wrote:
> > Do you look at all warnings all your software prints?
> 
> Yes.  I also try to understand them, and if they seem useless I propose
> a patch upstream.
> 
> > What if user uses some tool to build the command line?
> > 
> > consider mem-path - for years we printed a warning if
> > it's not on hugetlbfs. It worked and users just ignored it.
> 
> That was a bad warning though.  There was no reason to warn.
> 
> It was also the smallest problem with the original hugetlbfs patches.
> 
> > So now Corey basically is prevented from sorting sanely
> > because command line might not start with opt/
> 
> Can you explain?
> 
> Paolo

Ask Corey:
http://thread.gmane.org/gmane.comp.emulators.qemu/400540/focus=400799


-- 
MST

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Gerd Hoffmann]

On Di, 2016-03-15 at 16:54 +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 03:46:35PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > I think we have already gone through this discussion.
> > > 
> > > So now Corey basically is prevented from sorting sanely
> > > because command line might not start with opt/
> > 
> > Hmm?  There are no guarantees whatsoever if the user used the command
> > line for entries outside /opt.
> > 
> > Beside that the "sort everything lexical for 2.6+" approach will work
> > fine.  I think it is more robust and I suspect we will have less hassle
> > with it long-term.
> > 
> > cheers,
> >   Gerd
> 
> OK so use built-in order with fallback on lexical,

I would just call smbios init from the old location for old machine
types instead of adding code for the built-in sort order ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 04:03:23PM +0100, Gerd Hoffmann wrote:
> On Di, 2016-03-15 at 16:54 +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 15, 2016 at 03:46:35PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > I think we have already gone through this discussion.
> > > > 
> > > > So now Corey basically is prevented from sorting sanely
> > > > because command line might not start with opt/
> > > 
> > > Hmm?  There are no guarantees whatsoever if the user used the command
> > > line for entries outside /opt.
> > > 
> > > Beside that the "sort everything lexical for 2.6+" approach will work
> > > fine.  I think it is more robust and I suspect we will have less hassle
> > > with it long-term.
> > > 
> > > cheers,
> > >   Gerd
> > 
> > OK so use built-in order with fallback on lexical,
> 
> I would just call smbios init from the old location for old machine
> types instead of adding code for the built-in sort order ...
> 
> cheers,
>   Gerd

That's too fragile. This time I caught the code reordering
but I might not notice it the next time.

-- 
MST

Re: [Qemu-devel] [PATCH] vl.c: disallow command line fw cfg without opt/

[Corey Minyard]

On 03/15/2016 09:58 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 03:54:36PM +0100, Paolo Bonzini wrote:
>>
>> On 15/03/2016 15:25, Michael S. Tsirkin wrote:
>>> Do you look at all warnings all your software prints?
>> Yes.  I also try to understand them, and if they seem useless I propose
>> a patch upstream.
>>
>>> What if user uses some tool to build the command line?
>>>
>>> consider mem-path - for years we printed a warning if
>>> it's not on hugetlbfs. It worked and users just ignored it.
>> That was a bad warning though.  There was no reason to warn.
>>
>> It was also the smallest problem with the original hugetlbfs patches.
>>
>>> So now Corey basically is prevented from sorting sanely
>>> because command line might not start with opt/
>> Can you explain?
>>
>> Paolo
> Ask Corey:
> http://thread.gmane.org/gmane.comp.emulators.qemu/400540/focus=400799
>
>
I think what Paulo is saying is that if you don't use /opt, then all bets
are off whether it works properly.  You were warned and ignored it.

I can go either way with this, you can enforce it (which seems like
the safer route to me) or you can just say that it may not work right
if you don't use /opt.

With the new patch I just posted it won't actually make any difference.
It doesn't look at file names for some things, that ended up not working
because in at least one situation the same file name would end up
in different places depending on the situation, and the order of some
things vary depending on the order the user lists them.

(The one I found was that if you have a pc-0.11 or earlier, if you have
a PCI VGA card it ends up adding a fw_cfg item for the ROM when
VGA is initialized.  If you have an ISA VGA card, it will have the same
file name but will end up where device initialization occurs.)

The solution I ended up with was a fixed specified order, but in
certain sections (VGA, NIC, user added, and devices) they will be
ordered by when they were inserted.

-corey