From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V8 3/7] libxl: add pvusb API Date: Thu, 12 Nov 2015 17:27:59 +0000 Message-ID: References: <1445418510-19614-1-git-send-email-cyliu@suse.com> <1445418510-19614-4-git-send-email-cyliu@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1445418510-19614-4-git-send-email-cyliu@suse.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: Chunyan Liu Cc: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= , Wei Liu , Ian Campbell , Ian Jackson , "xen-devel@lists.xen.org" , Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu wrote: > +static int > +get_assigned_devices(libxl__gc *gc, > + libxl_device_usb **list, int *num) > +{ > + char **domlist; > + unsigned int nd = 0, i, j, k; > + int rc; > + > + *list = NULL; > + *num = 0; > + > + domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd); > + for (i = 0; i < nd; i++) { > + char *path, **ctrl_list; > + unsigned int nc = 0; > + > + path = GCSPRINTF("/local/domain/%s/device/vusb", domlist[i]); > + ctrl_list = libxl__xs_directory(gc, XBT_NULL, path, &nc); > + > + for (j = 0; j < nc; j++) { > + const char *be_path, *num_ports; > + int nport; > + > + rc = libxl__xs_read_checked(gc, XBT_NULL, > + GCSPRINTF("%s/%s/backend", path, ctrl_list[j]), > + &be_path); > + if (rc) goto out; > + > + rc = libxl__xs_read_checked(gc, XBT_NULL, > + GCSPRINTF("%s/num-ports", be_path), > + &num_ports); > + if (rc) goto out; > + > + nport = atoi(num_ports); > + for (k = 0; k < nport; k++) { > + libxl_device_usb *usb; > + const char *portpath, *busid; > + > + portpath = GCSPRINTF("%s/port/%d", be_path, k + 1); > + busid = libxl__xs_read(gc, XBT_NULL, portpath); > + /* If there is USB device attached, add it to list */ > + if (busid && strcmp(busid, "")) { > + GCREALLOC_ARRAY(*list, *num + 1); > + usb = *list + *num; > + (*num)++; > + libxl_device_usb_init(usb); > + usb->ctrl = atoi(ctrl_list[j]); > + usb->port = k + 1; > + rc = usb_busaddr_from_busid(gc, busid, > + &usb->u.hostdev.hostbus, > + &usb->u.hostdev.hostaddr); You should probably set usb->devtype to HOSTDEV here, even though this function is internal. > + if (rc) goto out; > + } > + } > + } > + } > + > + rc = 0; > + > +out: > + if (rc) { > + *list = NULL; > + *num = 0; > + } > + return rc; > +} > + > +static bool is_usbdev_in_array(libxl_device_usb *usbs, int num, > + libxl_device_usb *usb) > +{ > + int i; > + > + for (i = 0; i < num; i++) { > + if (usbs[i].u.hostdev.hostbus == usb->u.hostdev.hostbus && > + usbs[i].u.hostdev.hostaddr == usb->u.hostdev.hostaddr) > + return true; > + } > + > + return false; > +} > + > +/* check if USB device is already assigned to a domain */ > +/* check if USB device type is assignable */ > +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb) > +{ > + int classcode; > + char *filename; > + void *buf = NULL; > + char *busid = NULL; > + > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, > + usb->u.hostdev.hostaddr); > + if (!busid) return false; > + > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid); > + if (libxl__read_sysfs_file_contents(gc, filename, &buf, NULL)) > + return false; > + > + classcode = atoi(buf); > + return classcode != USBHUB_CLASS_CODE; > +} > + > +/* get usb devices under certain usb controller */ > +static int > +libxl__device_usb_list_for_usbctrl(libxl__gc *gc, uint32_t domid, > + libxl_devid usbctrl, > + libxl_device_usb **usbs, int *num) > +{ > + const char *fe_path, *be_path, *num_devs; > + int n, i, rc; > + > + *usbs = NULL; > + *num = 0; > + > + fe_path = GCSPRINTF("%s/device/vusb/%d", > + libxl__xs_get_dompath(gc, domid), usbctrl); > + > + rc = libxl__xs_read_checked(gc, XBT_NULL, > + GCSPRINTF("%s/backend", fe_path), > + &be_path); > + if (rc) return rc; > + > + rc = libxl__xs_read_checked(gc, XBT_NULL, > + GCSPRINTF("%s/num-ports", be_path), > + &num_devs); > + if (rc) return rc; > + > + n = atoi(num_devs); > + > + for (i = 0; i < n; i++) { > + char *busid; > + libxl_device_usb *usb; > + > + busid = libxl__xs_read(gc, XBT_NULL, > + GCSPRINTF("%s/port/%d", be_path, i + 1)); > + if (busid && strcmp(busid, "")) { > + GCREALLOC_ARRAY(*usbs, *num + 1); > + usb = *usbs + *num; > + (*num)++; > + libxl_device_usb_init(usb); > + usb->ctrl = usbctrl; > + usb->port = i + 1; > + rc = usb_busaddr_from_busid(gc, busid, > + &usb->u.hostdev.hostbus, > + &usb->u.hostdev.hostaddr); Same thing re devtype. > +int libxl_ctrlport_to_device_usb(libxl_ctx *ctx, > + uint32_t domid, > + int ctrl, > + int port, > + libxl_device_usb *usb) > +{ > + GC_INIT(ctx); > + const char *dompath, *be_path, *busid; > + int rc; > + > + dompath = libxl__xs_get_dompath(gc, domid); > + > + rc = libxl__xs_read_checked(gc, XBT_NULL, > + GCSPRINTF("%s/device/vusb/%d/backend", dompath, ctrl), > + &be_path); > + if (rc) goto out; > + > + rc = libxl__xs_read_checked(gc, XBT_NULL, > + GCSPRINTF("%s/port/%d", be_path, port), > + &busid); > + if (rc) goto out; > + > + if (!strcmp(busid, "")) { > + rc = ERROR_FAIL; > + goto out; > + } > + > + usb->ctrl = ctrl; > + usb->port = port; > + rc = usb_busaddr_from_busid(gc, busid, &usb->u.hostdev.hostbus, > + &usb->u.hostdev.hostaddr); You definitely need to set usb->devtype here to HOSTDEV. > +libxl_usbinfo = Struct("usbinfo", [ > + ("ctrl", libxl_devid), > + ("port", integer), > + ("busnum", uint8), > + ("devnum", uint8), > + ("idVendor", uint16), > + ("idProduct", uint16), > + ("prod", string), > + ("manuf", string), > + ], dir=DIR_OUT) As mentioned in the review of another patch, it looks like this is vestigal and should be removed. > +void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr) > +{ > + int i; > + for (i = 0; i < nr; i++) > + libxl_device_usbctrl_dispose(&list[i]); > + free(list); > +} > + > +void libxl_device_usb_list_free(libxl_device_usb *list, int nr) > +{ > + int i; > + for (i = 0; i < nr; i++) > + libxl_device_usb_dispose(&list[i]); > + free(list); > +} This is nitpicky, but as long as you're going to repost: the first-level indents in these two functions are only 3 spaces instead of 4. Other than that (and my previous comments + Ian's comments) it looks good to me! -George