Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

On Tue, Mar 17, 2015 at 12:49:50PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Perhaps the suggestion to move the file loading from fw_cfg_init1() --
> > ie. device initialization -- to the earlier option parsing phase will
> > appease Gerd too :) But, admittedly, I don't know what the "existing
> > fw_cfg init order issues" that he referenced are.
> 
> Basically fw_cfg init picks up information from a bunch of places,
> instead of the having the places where the information is generated
> store the information in fw_cfg.  Which would be nice as it would make
> the fw_cfg dependency more visible in the code.
> 
> Due to parsing and using command line switches being separated (via
> QemuOpts) this doesn't create that many init order issues though.
> 
> Which reminds me:  Going for QemuOpts would be very useful (gives
> -readconfig support), and it would solve the init order issue too.
> Instead of having your custom storage you just let QemuOpts parse and
> store the command line switch, then process them after fw_cfg device
> initialization.

+static struct FWCfgOption {
+    const char *name;
+    const char *file;
+} *fw_cfg_options;
+static size_t fw_cfg_num_options;
+
+void fw_cfg_option_add(QemuOpts *opts)
+{
+    const char *name = qemu_opt_get(opts, "name");
+    const char *file = qemu_opt_get(opts, "file");
+
+    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
+        error_report("invalid argument value");
+        exit(1);
+    }
+
+    fw_cfg_options = g_realloc(fw_cfg_options, sizeof(struct FWCfgOption) *
+                                               (fw_cfg_num_options + 1));
+    fw_cfg_options[fw_cfg_num_options].name = name;
+    fw_cfg_options[fw_cfg_num_options].file = file;
+    fw_cfg_num_options++;
+}

So basically, you're saying "don't process 'opts' here with qemu_opt_get(),
just stash them as is, then process them later" ?  (e.g. during
fw_cfg_options_insert() below) ?

Is there an existing example where that's currently done that I use
for inspiration?

Laszlo's suggestion was almost the opposite of this, i.e. allocate
memory for all the blobs during option processing, them just call
fw_cfg_add_file() during fw_cfg_options_insert().


+static void fw_cfg_options_insert(FWCfgState *s)
+{
+    size_t i;
+    int filesize;
+    const char *filename;
+    void *filebuf;
+
+    for (i = 0; i < fw_cfg_num_options; i++) {
+        filename = fw_cfg_options[i].file;
+        filesize = get_image_size(filename);
+        if (filesize == -1) {
+            error_report("Cannot read fw_cfg file %s", filename);
+            exit(1);
+        }
+        filebuf = g_malloc(filesize);
+        if (load_image(filename, filebuf) != filesize) {
+            error_report("Failed to load fw_cfg file %s", filename);
+            exit(1);
+        }
+        fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize);
+    }
+}
+
 
 static void fw_cfg_init1(DeviceState *dev)
 {
@@ -584,6 +631,8 @@ static void fw_cfg_init1(DeviceState *dev)
 
     qdev_init_nofail(dev);
 
+    fw_cfg_options_insert(s);
+
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == 
DT_NOGRAPHIC));


Personally, I don't think there are *any* init order issues at all.
It's an error to insert a dupe file name either way, whether the
user's command line blob goes in first and we error out at the
programmatically inserted dupe, or the other way around.


BTW, regarding that suggestion (to allocate blobs during option_add
and just call add_file() from options_insert(): I did think of that,
but decided against it because users could pathologically provide the
same fw_cfg file name multiple times on the command line, and I didn't
like the idea of either allocating RAM for each one blindly, nor did I
like the idea of adding checks for dupes within user-supplied blobs,
so delaying everything until the moment fw_cfg_add_file would enforce
uniqueness on my behalf sounded like the least amount of trouble.

Of course, that's an unlikely scenario, and so what if we allocate
lots of RAM only to exit with an error shortly after :)

Anyway, I'm going to try and figure out the bits about stashing
QemuOpts from fw_cfg_option_add(), then probably fall back to
preallocating blobs during the same function.

If I (very likely) misunderstood something, please let me know :)

Thanks,
--Gabriel