[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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


> 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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.