From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH V8 3/7] libxl: add pvusb API Date: Mon, 16 Nov 2015 18:06:34 +0000 Message-ID: <22090.6954.35639.703183@mariner.uk.xensource.com> References: <1445418510-19614-1-git-send-email-cyliu@suse.com> <1445418510-19614-4-git-send-email-cyliu@suse.com> <22080.57829.461049.37192@mariner.uk.xensource.com> <56421E2B020000660008019B@relay2.provo.novell.com> <22084.50631.506663.212889@mariner.uk.xensource.com> <5645BBBE0200006600082618@relay2.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5645BBBE0200006600082618@relay2.provo.novell.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: Chun Yan Liu Cc: Juergen Gross , wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, xen-devel@lists.xen.org, Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org Thanks for your attention to my earlier mail. I'll delete all the comments where we agree :-). > > > > > +/* bind/unbind usb device interface */ > > > > > +static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath) > > > > > +{ > > ... > > > > > + dp = libxl__zalloc(gc, PATH_MAX); > > > > > + dp = realpath(spath, dp); > > > > > > > > Why is this call to realpath needed ? > > > > > > In sysfs, /driver sometimes is a link, since we need to save the original > > > driver to xenstore, so need to get the realpath of the driver. > > > > I mean, why is the path with all the symlinks in it not suitable for > > storage and later use ? > > The symlink might be "../../../../../../bus/usb/drivers/btusb", we > couldn't save that to xenstore for later usage. So the real reason is simply that it is a relative path and we need an absolute one, because a relative path is not useful ? OK, thanks for the explanation. (I'm not sure whether this is unobvious enough to warrant a comment, perhaps /* sysfs can produce relative paths */ or something.) > > > > I think you probably do things in the wrong order here. You should > > > > write the old driver info to xenstore first, so that if the bind to > > > > usbback fails, you have the old driver info. > > > > > > Perhaps not. Once we finished adding entries to xenstore, pvusb > > > frontend/backend drivers will detect the change and do probe work, if > > > USB device is still not bound to USBBACK, there might be error. > > > > What I mean is this: > > > > Is it not possible to write the original path to xenstore before doing > > the unbind ? Otherwise it seems like there could be error paths where > > the original path is not recorded, the xenstore write fails, and then > > the information about how to rebind to the original driver has been > > lost. > > I see. Right. Do you think this warrants a change to the code ? > > Does writing USBBACK_INFO_PATH"/%s/%s/driver_path" really trigger > > usbback ? > > No, writing driver_path info to xenstore won't trigger usbback. Writing > frontend/backend info will. Jolly good. > > > If we merge libxl__device_usb_add and do_usb_add, then it's cleaner. > > > > See my comment below. You've explained the distinction to my > > satisfaction. > > > > But, to solve the duplication of the controller info acquisition, > > perhaps you could have do_usb_add take the controller info as a > > paramaeter ? > > Yes, could be. Only do_usb_add and do_usb_remove parameters are not > symmetrical. I don't think that matters. (If it did it would be better to add an unused parameter to the remove function.) Regards, Ian.