Re: [Qemu-devel] [PATCH v2] Sort the fw_cfg file list

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 07:56:36AM -0500, Corey Minyard wrote:
> On 03/15/2016 07:45 AM, Michael S. Tsirkin wrote:
> >On Tue, Mar 15, 2016 at 07:38:43AM -0500, Corey Minyard wrote:
> >>On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
> >>>On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
> >>>>>Depends on how you code it up.  We have a list, we look each file
> >>>>>there and sort accordingly.  Fine.
> >>>>>New devices will not be on this list, I guess you can just ignore them
> >>>>>and guests will not see them. OK but I think it is better to make old
> >>>>>machine types see them.
> >>>>Not a new fw_cfg file.
> >>>>
> >>>>It's existing smbios file which gets new records added by a new device.
> >>>>So when initializing it early (old order) it doesn't (yet) contain the
> >>>>new records.  When initializing it late it has them, but also has a
> >>>>different place in the fw_cfg directory.
> >>>>
> >>>>So old machine types initialize smbios early (for compatibility).
> >>>I see. So in this model, we'd have to somehow keep track of
> >>>the old initialization order forever, and
> >>>add hacks whenever we change it.
> >>>IMHO That would just be too hard to maintain. I have an alternative
> >>>proposal.
> >>>
> >>>
> >>>
> >>>>New machine types initialize smbios late (so guests see the new
> >>>>records).
> >>>So here is what I propose instead:
> >>>
> >>>- always initialize it late
> >>>- sort late, a machine done, not when inserting entries
> >>>- figure out what the order of existing entries is currently,
> >>>   and fill an array listing them in this order.
> >>>   for old machine types, insert the existing entries
> >>>   in this specific order by using a sorting function:
> >>>
> >>>qsort(....., fw_cfg_cmp);
> >>>
> >>>where:
> >>>
> >>>fw_cfg_find(a) {
> >>>     for (index = 0; index < fw_cfg_legacy_array_size; ++index)
> >>>         if (!strcmp(a, ...))
> >>>             break;
> >>>     return index;
> >>>}
> >>>
> >>>fw_cfg_cmp(a, b) {
> >>>     in cmp;
> >>>     if (legacy_fw_cfg_order) {
> >>>         int list1 = find(a);
> >>>         int list2 = find(b);
> >>>
> >>>         if (list1 < list2)
> >>>             return -1;
> >>>         if (list1 > list2)
> >>>             return 1;
> >>>     }
> >>>
> >>>     return strcmp(a, b);
> >>>}
> >>Last night I had an idea something like this.  Sorting by filename
> >>may not work because the user may pass in the file from the
> >>command line and you wouldn't be able to track the file name that
> >>way.
> >command line files must all have a consistent prefix,
> >so we can skip sorting them.
> >I'll need to look at the code - don't they already?
> >If not we IMHO absolutely must fix that before release
> >and give them consistent prefixes.
> 
> You get a warning if it doesn't start with "opt/", but
> that is not enforced.

Why don't we enforce it? It seems very dangerous to allow arbitrary
names as they may conflict with builtin ones down the road.
Here's a patch - pls feel free to include this in your patchset.

--->

vl.c: disallow command line fw cfg without opt/

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 <address@hidden>
---

 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/\"");
+        return -1;
     }
     if (nonempty_str(str)) {
         size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
-- 
MST