From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest Date: Wed, 17 Apr 2013 10:59:17 +0100 Message-ID: <516E7275.9030406@eu.citrix.com> References: <1365706317-5368-1-git-send-email-george.dunlap@eu.citrix.com> <516D91AE.4030600@citrix.com> <516E6D0F.4090606@eu.citrix.com> <1366192463.8399.219.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1366192463.8399.219.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "sstanisi@cbnco.com" , "xen-devel@lists.xen.org" , Ian Jackson , Roger Pau Monne List-Id: xen-devel@lists.xenproject.org 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