Re: [Xen-devel] [PATCH V2] libxl: enabling upstream qemu as pv backend

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [Xen-devel] [PATCH V2] libxl: enabling upstream qemu as pv 
backend"):
> Resend.

Thanks.

> Gmail is really bad for submitting patch, but it is the only choice
> for the moment...

The patch seems mostly OK to me but I have two small complaints:


Firstly we have three copies of essentially this:

+    /* parse extra args for qemu, common to both pv, fv */
+    if (!xlu_cfg_get_list(config, "device_model_args", &dmargs, &nr_dmargs, 0))
+    {
+        int i;
+        dm_info->extra = xmalloc(sizeof(char *) * (nr_dmargs + 1));
+        dm_info->extra[nr_dmargs] = NULL;
+        for (i=0; i<nr_dmargs; i++) {
+            const char *a = xlu_cfg_get_listitem(dmargs, i);
+            dm_info->extra[i] = a ? strdup(a) : NULL;
+        }
+    }

Can you please find a way to do this that doesn't involve open-coding
the same 13 lines for each case ?  A small helper function or a macro
would do.


Secondly, there are still some overly long lines.  Can you please,
before you resend, check that there are no added lines in your diff
which are over 80 (preferably, 75) characters ?  This includes lines
containing message strings.  Long lines look like this to me:

+                "WARNING: ignoring device_model directive.\n"
+                "WARNING: Use \"device_model_override\" instead if you really 
want a non-default device_model\n");
+        if (strstr(buf, "stubdom-dm")) {
+            if (c_info->hvm == 1)
+                fprintf(stderr, "WARNING: Or use \"device_model_stubdomain_ovr
eride\" if you want to enable stubdomains\n");
+            else
+                fprintf(stderr, "WARNING: ignoring \"device_model_stubdomain_o
verride\" directive for pv guest\n");
+        }
+    }

This makes it very hard to see the structure.


Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel