From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v8 3/5] libxl: add support for vscsi Date: Mon, 15 Feb 2016 15:16:10 +0000 Message-ID: <20160215151610.GC10489@citrix.com> References: <1455205411-25460-1-git-send-email-olaf@aepfle.de> <1455205411-25460-4-git-send-email-olaf@aepfle.de> <20160212172748.GA8818@citrix.com> <20160212182459.GA28852@aepfle.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20160212182459.GA28852@aepfle.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Olaf Hering Cc: Ian Jackson , Stefano Stabellini , Wei Liu , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, Feb 12, 2016 at 07:24:59PM +0100, Olaf Hering wrote: [...] > > > > + if (libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs)) { > > > + rc = libxl__device_vscsi_reconfigure_add(egc, aodev, &vscsi_saved, &d_config, be_path); > > > + if (rc) > > > + goto out; > > > + /* Notify that this is done */ > > > + aodev->callback(egc, aodev); > > > > Wouldn't it be better if you call aodev->callback unconditionally at the > > end of this function? BTW you seem to have forgotten to set aodev->rc. > > I have to rework this code, its still using libxl__wait_for_backend. > This is already fixed in the detach case. Somehow I was only > concentrating on the detach case this week, and did not notice this. > OK. > > > > +/* Remove vscsidev connected to vscsictrl */ > > > +int libxl_device_vscsidev_remove(libxl_ctx *ctx, uint32_t domid, > > > + libxl_device_vscsidev *dev, > > > + const libxl_asyncop_how *ao_how) > > > + LIBXL_EXTERNAL_CALLERS_ONLY; > > > > Where is libxl_device_vscsidev_add? > > This is done in scsi-attach, via libxl_device_vscsictrl_add. Its not > even, compared to the scsi-detach case. I will see if that can be > changed. > > > > > + libxl__add_vscsictrls(egc, ao, domid, d_config, &dcs->multidev); > > > > This is because you need vscsi disk early? > > I think yes, DEFINE_DEVICES_ADD has to be used somewhere. > I'm confused. You're joking, right? "Has to be used somewhere" is not a justification for having it in this particular place. > > > > > +int main_vscsiattach(int argc, char **argv); > > > +int main_vscsilist(int argc, char **argv); > > > +int main_vscsidetach(int argc, char **argv); > > > > What about other commands? Looking at PVUSB series: > > > > +int main_usbctrl_attach(int argc, char **argv); > > +int main_usbctrl_detach(int argc, char **argv); > > +int main_usbdev_attach(int argc, char **argv); > > +int main_usbdev_detach(int argc, char **argv); > > +int main_usblist(int argc, char **argv); > > > > so we should be able to attach / detach both controller and specific > > device? > > There are no empty scsi controllers, just controllers with at least one > device. IMO its not useful to define empty controllers, what would be > the point? If it is mandated by hardware that empty scsi controller doesn't exist, that's of course fine. But I don't think it is mandated in reality? I can have no disk attached to a controller and that should be fine. > While toying around I noticed that removing all vscsidevs and > leaving the vscsictrl in xenstore the frontend preserved its SCSI > controller entry in sysfs. > That's what I would expect, too. Wei. > Olaf