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

Re: [Xen-devel] [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest



On 17/04/13 10:54, Ian Campbell wrote:
On Wed, 2013-04-17 at 10:36 +0100, George Dunlap wrote:
+/*
+ * Just use plain numbers for now.  Replace these with strings at some point.
+ */
+#define protocol_to_str(gc, t) libxl__sprintf((gc), "%d", (t))
+#define str_to_protocol(s) atoi((s))
+#define type_to_str(gc, t) libxl__sprintf((gc), "%d", (t))
GCSPRINTF, although this macros are so simple that you might also
replace them entirely with GCSPRINTF (altough it might make the code
harder to understand).
I might do that -- my original plan had actually been to actually print
"pv" for LIBXL_DEVICE_USB_BACKEND_PV, "dm" for _DEVICEMODEL, and so on.
Doing all the extra parsing was more work than I wanted to try to get
done for the code freeze, but I left the functions so I could easily
replaced them later.
These symbols should be coming from the IDL, in which case it will have
provided the parsing functions already, please use them instead of
inventing your own.

e.g. The libxl_domain_type enum in the idl generates
libxl_domain_type_from_string and libxl_domain_type_to_string (and
_to_json if that floats your boat). These implement the mapping:
        LIBXL_DOMAIN_TYPE_INVALID <=> "invalid"
        LIBXL_DOMAIN_TYPE_HVM     <=> "hvm"
        LIBXL_DOMAIN_TYPE_PV      <=> "pv"
(the from function is case insensitive)

"Replace these with strings at some point.", is that an API change,
either for the libxl user or the driver side?

This is just an internal thing for libxl; it doesn't persist across reboots.

Let me take a look at the functions you mentioned -- if they're really easy I'll just drop them in. Otherwise, the numeric values will be just fine, even if for some reason they end up being permanent.


+    switch (usbdev->protocol) {
+    case LIBXL_USB_PROTOCOL_DEVICEMODEL:
+        switch (libxl__device_model_version_running(gc, domid)) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            LOG(ERROR, "usb-remove not yet implemented for qemu-traditional");
+            return ERROR_INVAL;
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            if ( (rc = libxl__qmp_usb_remove(gc, domid, usbdev)) < 0 )
Spaces
How important are the spaces?  Most of the time I think it makes it
easier to read, in this case particularly so.
Everyone has their own opinions about these thing but the main purpose
of having a Coding Style is consistency and libxl's style is to not
include those spaces.

If you want to make it easier to read I would suggest removing the
assignment from the condition.

Ack.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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