[PATCH v3 0/4] libbxl: add support for pvscsi, iteration 3

[PATCH v3 0/4] libbxl: add support for pvscsi, iteration 3

[Olaf Hering]

Port vscsi=[] and scsi-{attach,detach,list} commands from xend to libxl.

Changes between v2 and v3:
 - Adjust change for vscsiif.h
 - Support "naa.wwn:lun" notation in pvops kernel
 - Add example for pvops kernel using targetcli
   patch required for python-rtslib:
   http://article.gmane.org/gmane.linux.scsi.target.devel/8146
 - Use vdev variable in libxl_device_vscsi_parse

Initial attempt was sent a year ago:
http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg03958.html
Most comments are addressed.

This version has been tested with SLES as backend and frontend.
This version has been tested with pvops as backend and SLES as frontend.


Please review. The first two changes could be applied independent from
the libxl changes.

Olaf

Olaf Hering (4):
  vscsiif.h: fix WWN notation for p-dev property
  docs: add vscsi to xenstore-paths.markdown
  docs: add pvscsi.txt
  libxl: add support for vscsi

 docs/misc/pvscsi.txt                 | 188 ++++++++++++++++++
 docs/misc/xenstore-paths.markdown    |  10 +
 tools/libxl/Makefile                 |   1 +
 tools/libxl/libxl.c                  | 172 ++++++++++++++++
 tools/libxl/libxl.h                  |  29 +++
 tools/libxl/libxl_create.c           |   1 +
 tools/libxl/libxl_device.c           |   2 +
 tools/libxl/libxl_freebsd.c          |   8 +
 tools/libxl/libxl_internal.h         |  12 ++
 tools/libxl/libxl_linux.c            |  60 ++++++
 tools/libxl/libxl_netbsd.c           |   8 +
 tools/libxl/libxl_types.idl          |  49 +++++
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/libxl_vscsi.c            | 375 +++++++++++++++++++++++++++++++++++
 tools/libxl/xl.h                     |   3 +
 tools/libxl/xl_cmdimpl.c             | 249 ++++++++++++++++++++++-
 tools/libxl/xl_cmdtable.c            |  15 ++
 xen/include/public/io/vscsiif.h      |   2 +-
 18 files changed, 1183 insertions(+), 2 deletions(-)
 create mode 100644 docs/misc/pvscsi.txt
 create mode 100644 tools/libxl/libxl_vscsi.c

[PATCH v3 1/4] vscsiif.h: fix WWN notation for p-dev property

[Olaf Hering]

The pvops kernel expects either "naa.WWN:LUN" or "h:c:t:l" in the p-dev
property. Add the missing :LUN part to the comment.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/io/vscsiif.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h
index 7a1db05..e8e38a9 100644
--- a/xen/include/public/io/vscsiif.h
+++ b/xen/include/public/io/vscsiif.h
@@ -60,7 +60,7 @@
  *
  *      A string specifying the backend device: either a 4-tuple "h:c:t:l"
  *      (host, controller, target, lun, all integers), or a WWN (e.g.
- *      "naa.60014054ac780582").
+ *      "naa.60014054ac780582:0").
  *
  * v-dev
  *      Values:         string

[PATCH v3 2/4] docs: add vscsi to xenstore-paths.markdown

[Olaf Hering]

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 docs/misc/xenstore-paths.markdown | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index d94ea9d..77ba2dd 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -232,6 +232,11 @@ A virtual keyboard device frontend. Described by
 A virtual network device frontend. Described by
 [xen/include/public/io/netif.h][NETIF]
 
+#### ~/device/vscsi/$DEVID/* []
+
+A virtual scsi device frontend. Described by
+[xen/include/public/io/vscsiif.h][SCSIIF]
+
 #### ~/console/* []
 
 The primary PV console device. Described in [console.txt](console.txt)
@@ -302,6 +307,10 @@ A virtual keyboard device backend. Described by
 A virtual network device backend. Described by
 [xen/include/public/io/netif.h][NETIF]
 
+#### ~/backend/vscsi/$DOMID/$DEVID/* []
+
+A PV SCSI backend. Described in [pvscsi.txt](pvscsi.txt)
+
 #### ~/backend/console/$DOMID/$DEVID/* []
 
 A PV console backend. Described in [console.txt](console.txt)
@@ -403,6 +412,7 @@ ifb device used by Remus to buffer network output from the associated vif.
 [KBDIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,kbdif.h.html
 [LIBXLMEM]: http://xenbits.xen.org/docs/unstable/misc/libxl_memory.txt
 [NETIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,netif.h.html
+[SCSIIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vscsiif.h.html
 [SI]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,xen.h.html#Struct_start_info
 [VCPU]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vcpu.h.html
 [XSWIRE]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,xs_wire.h.html

Re: [PATCH v3 2/4] docs: add vscsi to xenstore-paths.markdown

[Wei Liu]

On Fri, Mar 06, 2015 at 10:45:54AM +0100, Olaf Hering wrote:
[...]
>  #### ~/backend/console/$DOMID/$DEVID/* []
>  
>  A PV console backend. Described in [console.txt](console.txt)
> @@ -403,6 +412,7 @@ ifb device used by Remus to buffer network output from the associated vif.
>  [KBDIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,kbdif.h.html
>  [LIBXLMEM]: http://xenbits.xen.org/docs/unstable/misc/libxl_memory.txt
>  [NETIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,netif.h.html
> +[SCSIIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vscsiif.h.html
                                                                           ^
                                                                    public,io,vscsiif.h

>  [SI]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,xen.h.html#Struct_start_info
>  [VCPU]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vcpu.h.html
>  [XSWIRE]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,xs_wire.h.html
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 2/4] docs: add vscsi to xenstore-paths.markdown

[Olaf Hering]

On Fri, Mar 06, Wei Liu wrote:

> On Fri, Mar 06, 2015 at 10:45:54AM +0100, Olaf Hering wrote:
> [...]
> >  #### ~/backend/console/$DOMID/$DEVID/* []
> >  
> >  A PV console backend. Described in [console.txt](console.txt)
> > @@ -403,6 +412,7 @@ ifb device used by Remus to buffer network output from the associated vif.
> >  [KBDIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,kbdif.h.html
> >  [LIBXLMEM]: http://xenbits.xen.org/docs/unstable/misc/libxl_memory.txt
> >  [NETIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,netif.h.html
> > +[SCSIIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,vscsiif.h.html
>                                                                            ^
>                                                                     public,io,vscsiif.h

Thanks, I will fix that.

Olaf

[PATCH v3 3/4] docs: add pvscsi.txt

[Olaf Hering]

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 docs/misc/pvscsi.txt | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)

diff --git a/docs/misc/pvscsi.txt b/docs/misc/pvscsi.txt
new file mode 100644
index 0000000..988a12b
--- /dev/null
+++ b/docs/misc/pvscsi.txt
@@ -0,0 +1,188 @@
+. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
+PVSCSI
+
+== Overview ==
+
+PVSCSI allows to assing a "physical" SCSI device from dom0 to a domU. The device
+is not limited to be a native SCSI device. Everything visible as SCSI device in
+dom0 can be used. Currently PVSCSI is only available in Linux dom0 and domU.
+
+== TODO ===
+
+How to do live migration?
+ - pdev will likely be evaluated again on the target host if it came from
+   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
+   to adjust h:c:t:l on the target host.
+
+How to handle FIXME in libxl_retrieve_domain_configuration?
+ - "MERGE(vscsi, vscsis, COMPARE_DEVID, {});", when does this code run?
+ 
+Implement libvirt integration.
+ - Is the API used by xl usable for libvirt?
+ - How does libvirt assign a raw SCSI device?
+   Jim said:
+    > vscsi=[ '/dev/sda,0:1:2:3', '4:3:2:1,1:1:1:1,feature-host' ]
+    In libvirt-speak, this is SCSI host device assignment
+    http://libvirt.org/formatdomain.html#elementsHostDev
+    Here's the SCSI example
+      <devices>
+       <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'>
+        <source>
+         <adapter name='scsi_host0'/>
+         <address type='scsi' bus='0' target='0' unit='0'/>
+        </source>
+        <readonly/>
+        <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+       </hostdev>
+      ...
+      </devices>
+    I suppose 'feature-host' maps to attributes such as 'sgio' and 'rawio'.
+    The source element describes the SCSI device as seen from the host.
+
+
+== History ==
+
+Support for PVSCSI was added to the classic xenlinux kernel and to xend in
+xen-3.3 in 2008. Support for PVSCSI was added to the upstream pvops kernel 3.18
+in 2014. Backends exists just for Linux. A frontend exists for Linux, and maybe
+for FreeBSD according to google.
+
+
+== Config Format ==
+
+The domU.cfg syntax is "vscsi=[ 'pdev,vdev[,option]', 'pdev,vdev[,option]' ]".
+'pdev' describes the physical device, preferable in a persistant format.
+'vdev' is the domU device in vHOST:CHANNEL:TARGET:LUN notation, all integers.
+'option' lists additional flags which a backend may recognize.
+
+
+== Configuring xenlinux backend ==
+
+"pdev" is the dom0 device in either /dev/scsidev or pHOST:CHANNEL:TARGET:LUN
+notation.  Currently only the option value "feature-host" is recognized. SCSI
+command emulation in backend driver is bypassed when "feature-host" is
+specified.
+
+Its recommended to use persistant names "/dev/disk/by-*/*" to refer to a "pdev".
+The toolstack will translate this internally to "h:c:t:l" notation, which is how
+the backend driver will access the device. Using the "h:c:t:l" notation for
+"pdev" in domU.cfg is discouraged because this value will change across reboots,
+depending on the detection order in the OS.
+
+
+== Configuring pvops backend ==
+
+The backend driver in the pvops kernel is part of the Linux-IO Target framework
+(LIO).  As such the SCSI devices have to be configured first with the tools
+provided by this framework, such as targetcli. The "pdev" in domU.cfg has to
+refer to a config item in that framework instead of the raw device.
+
+The LIO uses a concept of backstore and frontend drivers. LIO backstore drivers
+provide access to the actual storage, LIO frontend drivers provide an interface
+which is accessed with SCSI commands. Within the LIO the frontend is connected
+to a certain backstore.
+
+For Xen its recommended to use the PSCSI backstore. Other backstores may work as
+well, but they are not tested. The xen-scsiback module is the LIO frontend, and
+at the same time the backend for the domU. In general the config flow is:
+- the LIO backstore refers to physical storage in a persistant way
+- the given storage gets a free, unique name within the LIO backstore
+- the LIO frontend refers to that free, unique name in the LIO backstore
+- the connection to the LIO backstore gets a naa.WWN:LUN in the LIO frontend
+- the "pdev" in domU.cfg refers to that naa.WWN:LUN in the LIO frontend
+
+Example for targetcli and a xen-scsiback aware python-rtslib:
+
+ # modprobe -v xen-scsiback
+ # DEV=/dev/disk/by-id/ata-TSSTcorp_DVD+_-RW_TS-H653G_R4576GFZ638411
+ # NAME="my_dvd"
+ # NAA=$(printf "naa.%s\n" `od -N 8 -t x8 -A n < /dev/urandom`)
+ # targetcli "/backstores/pscsi create name=$NAME dev=${DEV}"
+ # targetcli "/xen-pvscsi create wwn=$NAA"
+ # targetcli "/xen-pvscsi/$NAA/tpg1/luns create /backstores/pscsi/$NAME"
+ # echo "vscsi=[ '${NAA}:0,0:0:0:0' ]"
+
+The last line is the domU.cfg snippet.
+
+
+== Xenstore Format ==
+
+The backend driver uses a single_host:many_devices notation to manage domU
+devices. Everything is stored in /local/domain/0/backend/vscsi/. The xenstore
+layout looks like this:
+
+    <domid>/<vhost>/feature-host = "0"
+    <domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
+    <domid>/<vhost>/frontend-id = "<domid>"
+    <domid>/<vhost>/online = "1"
+    <domid>/<vhost>/state = "4"
+    <domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1" or "naa.wwn:lun"
+    <domid>/<vhost>/vscsi-devs/dev-0/state = "4"
+    <domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
+    <domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
+    <domid>/<vhost>/vscsi-devs/dev-1/state = "4"
+    <domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
+
+The frontend driver maintains its state in /local/domain/<domid>/device/vscsi/.
+
+    <vhost>/backend = "/local/domain/0/backend/vscsi/<domid>/<vhost>"
+    <vhost>/backend-id = "0"
+    <vhost>/event-channel = "20"
+    <vhost>/ring-ref = "43"
+    <vhost>/state = "4"
+    <vhost>/vscsi-devs/dev-0/state = "4"
+    <vhost>/vscsi-devs/dev-1/state = "4"
+
+In addition to the entries for backend and frontend these flags are stored for
+the toolstack:
+
+    <domid>/<vhost>/vscsi-devs/dev-1/p-devname = "/dev/$device"
+
+
+== Backend - Frontend Protocol ==
+
+To create a vhost along with a device:
+    <domid>/<vhost>/feature-host = "0"
+    <domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
+    <domid>/<vhost>/frontend-id = "<domid>"
+    <domid>/<vhost>/online = "1"
+    <domid>/<vhost>/state = "1"
+    <domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1"
+    <domid>/<vhost>/vscsi-devs/dev-0/state = "1"
+    <domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
+Wait for <domid>/<vhost>/state + <domid>/<vhost>/vscsi-devs/dev-0/state become 4
+
+To add another device to a vhost:
+    <domid>/<vhost>/state = "7"
+    <domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
+    <domid>/<vhost>/vscsi-devs/dev-1/state = "1"
+    <domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
+Wait for <domid>/<vhost>/state + <domid>/<vhost>/vscsi-devs/dev-1/state become 4
+
+To remove a device from a vhost:
+    <domid>/<vhost>/state = "7"
+    <domid>/<vhost>/vscsi-devs/dev-1/state = "5"
+Wait for <domid>/<vhost>/state to become 4
+Wait for <domid>/<vhost>/vscsi-devs/dev-1/state become 6
+Remove <domid>/<vhost>/vscsi-devs/dev-1/{state,p-dev,v-dev,p-devname}
+Remove <domid>/<vhost>/vscsi-devs/dev-1/
+
+
+== Interface in xl ==
+
+xl scsi-attach domid <PhysDevice> <VirtDevice>[,<option>]
+xl scsi-detach domid <VirtDevice>
+xl scsi-list domid [domid, ...]
+
+
+== Interface in libxl ==
+
+To be defined so that both xl and libvirt pass in the same data.
+
+
+== Interface in libvirt ==
+
+Has yet to be found.
+
+. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
+# vim: tw=80 et ts=4

Re: [PATCH v3 3/4] docs: add pvscsi.txt

[Wei Liu]

On Fri, Mar 06, 2015 at 10:45:55AM +0100, Olaf Hering wrote:
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  docs/misc/pvscsi.txt | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 188 insertions(+)
> 
> diff --git a/docs/misc/pvscsi.txt b/docs/misc/pvscsi.txt
> new file mode 100644
> index 0000000..988a12b
> --- /dev/null
> +++ b/docs/misc/pvscsi.txt
> @@ -0,0 +1,188 @@
> +. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> +PVSCSI
> +
> +== Overview ==
> +
> +PVSCSI allows to assing a "physical" SCSI device from dom0 to a domU. The device
> +is not limited to be a native SCSI device. Everything visible as SCSI device in
> +dom0 can be used. Currently PVSCSI is only available in Linux dom0 and domU.
> +
> +== TODO ===
> +
> +How to do live migration?
> + - pdev will likely be evaluated again on the target host if it came from
> +   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
> +   to adjust h:c:t:l on the target host.
> +
> +How to handle FIXME in libxl_retrieve_domain_configuration?
> + - "MERGE(vscsi, vscsis, COMPARE_DEVID, {});", when does this code run?
> + 

I don't think this kind of information that relates to libxl internal
belongs here.

Anyway, to answer you question. libxl_retrieve_domain_configuration is
called by xl when you do migration / detailed domain configuration
listing etc.

I think the comment above MERGE macro states what it does and how to use
it. Is there anything that's not clear to you?

Wei.

Re: [PATCH v3 3/4] docs: add pvscsi.txt

[Olaf Hering]

On Fri, Mar 06, Wei Liu wrote:

> On Fri, Mar 06, 2015 at 10:45:55AM +0100, Olaf Hering wrote:
> > +== TODO ===
> > +
> > +How to do live migration?
> > + - pdev will likely be evaluated again on the target host if it came from
> > +   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
> > +   to adjust h:c:t:l on the target host.
> > +
> > +How to handle FIXME in libxl_retrieve_domain_configuration?
> > + - "MERGE(vscsi, vscsis, COMPARE_DEVID, {});", when does this code run?
> > + 
> 
> I don't think this kind of information that relates to libxl internal
> belongs here.

Its the overall TODO list, it has to go somewhere. And it will be
removed once its empty.

> Anyway, to answer you question. libxl_retrieve_domain_configuration is
> called by xl when you do migration / detailed domain configuration
> listing etc.

How can the values diverge? With commands like 'scsi-attach'?

> I think the comment above MERGE macro states what it does and how to use
> it. Is there anything that's not clear to you?

I think for vscsi it has to compare the vdev h:c:t:l part, just as disk
compares xvda. I will look at this once I'm done with libvirt
integration.

Olaf

Re: [PATCH v3 3/4] docs: add pvscsi.txt

[Wei Liu]

On Fri, Mar 06, 2015 at 04:11:20PM +0100, Olaf Hering wrote:
> On Fri, Mar 06, Wei Liu wrote:
> 
> > On Fri, Mar 06, 2015 at 10:45:55AM +0100, Olaf Hering wrote:
> > > +== TODO ===
> > > +
> > > +How to do live migration?
> > > + - pdev will likely be evaluated again on the target host if it came from
> > > +   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
> > > +   to adjust h:c:t:l on the target host.
> > > +
> > > +How to handle FIXME in libxl_retrieve_domain_configuration?
> > > + - "MERGE(vscsi, vscsis, COMPARE_DEVID, {});", when does this code run?
> > > + 
> > 
> > I don't think this kind of information that relates to libxl internal
> > belongs here.
> 
> Its the overall TODO list, it has to go somewhere. And it will be
> removed once its empty.
> 
> > Anyway, to answer you question. libxl_retrieve_domain_configuration is
> > called by xl when you do migration / detailed domain configuration
> > listing etc.
> 
> How can the values diverge? With commands like 'scsi-attach'?
> 

Because there are states that are only available in xenstore.  We want
to reflect that in the retrieved configuration. I think at the moment
only disk needs such handling. If you don't think vscsi values can
diverge you can just use {}.

> > I think the comment above MERGE macro states what it does and how to use
> > it. Is there anything that's not clear to you?
> 
> I think for vscsi it has to compare the vdev h:c:t:l part, just as disk
> compares xvda. I will look at this once I'm done with libvirt
> integration.

The point is to identify a device from guest's PoV. h:c:t:l might be
good enough.

Wei.

> 
> Olaf

Re: [PATCH v3 3/4] docs: add pvscsi.txt

[Ian Campbell]

On Fri, 2015-03-06 at 16:11 +0100, Olaf Hering wrote:
> On Fri, Mar 06, Wei Liu wrote:
> 
> > On Fri, Mar 06, 2015 at 10:45:55AM +0100, Olaf Hering wrote:
> > > +== TODO ===
> > > +
> > > +How to do live migration?
> > > + - pdev will likely be evaluated again on the target host if it came from
> > > +   domU.cfg. But what about pdev from 'xl scsi-attach pdev vdev'? Its required
> > > +   to adjust h:c:t:l on the target host.
> > > +
> > > +How to handle FIXME in libxl_retrieve_domain_configuration?
> > > + - "MERGE(vscsi, vscsis, COMPARE_DEVID, {});", when does this code run?
> > > + 
> > 
> > I don't think this kind of information that relates to libxl internal
> > belongs here.
> 
> Its the overall TODO list, it has to go somewhere. And it will be
> removed once its empty.

I'm not convinced it needs to go anywhere other than perhaps the cover
letter or your own ledger.

There doesn't seem to be very much in this document which actually
belongs, almost everything should be somewhere else IMHO.

"== Config Format ==" certainly belongs in xl.cfg(5) and not here as do
"== Configuring FOO backend ==" (or perhaps a relevant "host setup" wiki
page).

"== Xenstore Format ==" belongs in either
docs/misc/xenstore-paths.markdown or (more likely) in vscsiif.h. So does
"== Backend - Frontend Protocol ==".

"== Interface in xl ==" belongs in the xl manpages.

"== Interface in libxl ==" is a waste of time as it stands, but if it
contained useful information it should go in libxl.h.

"== Interface in libvirt ==" should go in the libvirt docs.

Ian.

[PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

Port pvscsi support from xend to libxl. See pvscsi.txt for details.

Outstanding work is listed in the TODO section.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/Makefile                 |   1 +
 tools/libxl/libxl.c                  | 172 ++++++++++++++++
 tools/libxl/libxl.h                  |  29 +++
 tools/libxl/libxl_create.c           |   1 +
 tools/libxl/libxl_device.c           |   2 +
 tools/libxl/libxl_freebsd.c          |   8 +
 tools/libxl/libxl_internal.h         |  12 ++
 tools/libxl/libxl_linux.c            |  60 ++++++
 tools/libxl/libxl_netbsd.c           |   8 +
 tools/libxl/libxl_types.idl          |  49 +++++
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/libxl_vscsi.c            | 375 +++++++++++++++++++++++++++++++++++
 tools/libxl/xl.h                     |   3 +
 tools/libxl/xl_cmdimpl.c             | 249 ++++++++++++++++++++++-
 tools/libxl/xl_cmdtable.c            |  15 ++
 15 files changed, 984 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 7329521..9622d66 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -91,6 +91,7 @@ endif
 LIBXL_LIBS += -lyajl
 
 LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
+			libxl_vscsi.o \
 			libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
 			libxl_internal.o libxl_utils.o libxl_uuid.o \
 			libxl_json.o libxl_aoutils.o libxl_numa.o \
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 088786e..73504b9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1967,6 +1967,166 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
 }
 
 /******************************************************************************/
+static int libxl__device_from_vscsi(libxl__gc *gc, uint32_t domid,
+                                    libxl_device_vscsi *vscsi,
+                                    libxl__device *device)
+{
+    device->backend_domid = vscsi->backend_domid;
+    device->devid         = vscsi->devid;
+    device->domid         = domid;
+    device->backend_kind  = LIBXL__DEVICE_KIND_VSCSI;
+    device->kind          = LIBXL__DEVICE_KIND_VSCSI;
+
+    return 0;
+}
+
+void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
+                             libxl_device_vscsi *vscsi,
+                             libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    flexarray_t *front;
+    flexarray_t *back;
+    libxl__device *device;
+    char *be_path;
+    unsigned int be_dirs = 0, rc, i;
+
+    if (vscsi->devid == -1) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Prealloc key+value: 4 toplevel + 4 per device */
+    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
+    back = flexarray_make(gc, i, 1);
+    front = flexarray_make(gc, 2 * 2, 1);
+
+    GCNEW(device);
+    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
+    if ( rc != 0 ) goto out;
+
+    /* Check if backend device path is already present */
+    be_path = libxl__device_backend_path(gc, device);
+    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
+        /* backend does not exist, create a new one */
+        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
+        flexarray_append_pair(back, "online", "1");
+        flexarray_append_pair(back, "state", "1");
+        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
+
+        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
+        flexarray_append_pair(front, "state", "1");
+    }
+
+    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
+        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
+        /* Trigger removal, otherwise create new device */
+        if (be_dirs) {
+            unsigned int nb = 0;
+            /* Preserve existing device */
+            if (libxl__xs_directory(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id), &nb) && nb) {
+                /* Trigger device removal by forwarding state to XenbusStateClosing */
+                if (v->remove)
+                    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "5");
+                continue;
+            }
+        }
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id), v->p_devname);
+        switch (v->pdev_type) {
+            case LIBXL_VSCSI_PDEV_TYPE_WWN:
+                flexarray_append_pair(back,
+                                      GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
+                                      v->p_devname);
+                break;
+            case LIBXL_VSCSI_PDEV_TYPE_DEV:
+            case LIBXL_VSCSI_PDEV_TYPE_HCTL:
+                flexarray_append_pair(back,
+                                      GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
+                                      GCSPRINTF("%u:%u:%u:%u", v->pdev.hst, v->pdev.chn, v->pdev.tgt, v->pdev.lun));
+                break;
+            case LIBXL_VSCSI_PDEV_TYPE_INVALID:
+            default:
+                rc = ERROR_FAIL;
+                goto out;
+        }
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/v-dev", v->vscsi_dev_id),
+                              GCSPRINTF("%u:%u:%u:%u", v->vdev.hst, v->vdev.chn, v->vdev.tgt, v->vdev.lun));
+        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "1");
+    }
+
+    aodev->dev = device;
+    /* Either create new host or reconfigure existing host */
+    if (be_dirs == 0) {
+        libxl__device_generic_add(gc, XBT_NULL, device,
+                                  libxl__xs_kvs_of_flexarray(gc, back, back->count),
+                                  libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                                  NULL);
+        aodev->action = LIBXL__DEVICE_ACTION_ADD;
+        libxl__wait_device_connection(egc, aodev);
+        rc = 0;
+        /* Done with new host */
+        goto out;
+    }
+
+    /* Only new devices, write them and do vscsi host reconfiguration */
+    xs_transaction_t t;
+retry_transaction:
+    t = xs_transaction_start(ctx->xsh);
+    libxl__xs_writev(gc, t, be_path,
+                     libxl__xs_kvs_of_flexarray(gc, back, back->count));
+    xs_write(ctx->xsh, t, GCSPRINTF("%s/state", be_path), "7", 2);
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        LOGE(ERROR, "xs transaction failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    libxl__wait_for_backend(gc, be_path, "4");
+
+retry_transaction2:
+    t = xs_transaction_start(ctx->xsh);
+    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
+        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
+        if (v->remove) {
+            char *path, *val;
+            path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
+            val = libxl__xs_read(gc, t, path);
+            if (val && strcmp(val, "6") == 0) {
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+                path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
+                xs_rm(ctx->xsh, t, path);
+            } else {
+                LOGE(ERROR, "%s: %s has %s, expected 6", __func__, path, val);
+            }
+        }
+    }
+
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction2;
+        LOGE(ERROR, "xs transaction failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    /* As we are not adding new device, skip waiting for it */
+    libxl__ao_complete(egc, aodev->ao, 0);
+
+    rc = 0;
+out:
+    aodev->rc = rc;
+    if(rc) aodev->callback(egc, aodev);
+    return;
+}
+
 int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
 {
     int rc;
@@ -4111,6 +4271,8 @@ out:
  * libxl_device_disk_destroy
  * libxl_device_nic_remove
  * libxl_device_nic_destroy
+ * libxl_device_vscsi_remove
+ * libxl_device_vscsi_destroy
  * libxl_device_vtpm_remove
  * libxl_device_vtpm_destroy
  * libxl_device_vkb_remove
@@ -4155,6 +4317,10 @@ DEFINE_DEVICE_REMOVE(disk, destroy, 1)
 DEFINE_DEVICE_REMOVE(nic, remove, 0)
 DEFINE_DEVICE_REMOVE(nic, destroy, 1)
 
+/* vtpm */
+DEFINE_DEVICE_REMOVE(vscsi, remove, 0)
+DEFINE_DEVICE_REMOVE(vscsi, destroy, 1)
+
 /* vkb */
 DEFINE_DEVICE_REMOVE(vkb, remove, 0)
 DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
@@ -4209,6 +4375,9 @@ DEFINE_DEVICE_ADD(disk)
 /* nic */
 DEFINE_DEVICE_ADD(nic)
 
+/* vscsi */
+DEFINE_DEVICE_ADD(vscsi)
+
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
@@ -6654,6 +6823,9 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
 
     MERGE(nic, nics, COMPARE_DEVID, {});
 
+    /* FIXME */
+    MERGE(vscsi, vscsis, COMPARE_DEVID, {});
+
     MERGE(vtpm, vtpms, COMPARE_DEVID, {});
 
     MERGE(pci, pcidevs, COMPARE_PCI, {});
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..1ad52e3 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1224,6 +1224,35 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
                                  libxl_device_channel *channel,
                                  libxl_channelinfo *channelinfo);
 
+/* Virtual SCSI */
+int libxl_device_vscsi_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vscsi *vscsi,
+                           const libxl_asyncop_how *ao_how)
+                           LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_vscsi_remove(libxl_ctx *ctx, uint32_t domid,
+                              libxl_device_vscsi *vscsi,
+                              const libxl_asyncop_how *ao_how)
+                              LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_vscsi_destroy(libxl_ctx *ctx, uint32_t domid,
+                               libxl_device_vscsi *vscsi,
+                               const libxl_asyncop_how *ao_how)
+                               LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num);
+int libxl_device_vscsi_getinfo(libxl_ctx *ctx,
+                               uint32_t domid,
+                               libxl_device_vscsi *vscsi_host,
+                               libxl_vscsi_dev *vscsi_dev,
+                               libxl_vscsiinfo *vscsiinfo);
+void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
+                                   libxl_vscsi_dev *dev);
+int libxl_device_vscsi_get_host(libxl_ctx *ctx,
+                                uint32_t domid,
+                                const char *cfg,
+                                libxl_device_vscsi **vscsi_host);
+int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
+                             libxl_device_vscsi *vscsi_host,
+                             libxl_vscsi_dev *vscsi_dev);
+
 /* Virtual TPMs */
 int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
                           const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 98687bd..5cacc30 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1136,6 +1136,7 @@ static void domcreate_rebuild_done(libxl__egc *egc,
     libxl__multidev_begin(ao, &dcs->multidev);
     dcs->multidev.callback = domcreate_launch_dm;
     libxl__add_disks(egc, ao, domid, d_config, &dcs->multidev);
+    libxl__add_vscsis(egc, ao, domid, d_config, &dcs->multidev);
     libxl__multidev_prepared(egc, &dcs->multidev, 0);
 
     return;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 0f50d04..035a4b6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -543,6 +543,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
  * The following functions are defined:
  * libxl__add_disks
  * libxl__add_nics
+ * libxl__add_vscsis
  * libxl__add_vtpms
  */
 
@@ -562,6 +563,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 DEFINE_DEVICES_ADD(disk)
 DEFINE_DEVICES_ADD(nic)
+DEFINE_DEVICES_ADD(vscsi)
 DEFINE_DEVICES_ADD(vtpm)
 
 #undef DEFINE_DEVICES_ADD
diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index e8b88b3..71bfae6 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -131,3 +131,11 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
 }
+
+int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
+                                unsigned int *chn, unsigned int *tgt,
+                                unsigned int *lun)
+{
+
+    return ERROR_NOPARAVIRT;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..76f6056 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1794,6 +1794,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
+  /* Convert h:c:t:l string to int */
+_hidden int libxl_device_vscsi_parse_hctl(libxl__gc *gc, char *str, libxl_vscsi_hctl *hctl);
+  /* Convert /dev/scsi to h:c:t:l */
+_hidden int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, libxl_vscsi_hctl *hctl);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
@@ -2386,6 +2390,10 @@ _hidden void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
                                    libxl_device_nic *nic,
                                    libxl__ao_device *aodev);
 
+_hidden void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
+                                     libxl_device_vscsi *vscsi,
+                                     libxl__ao_device *aodev);
+
 _hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
                                    libxl_device_vtpm *vtpm,
                                    libxl__ao_device *aodev);
@@ -3005,6 +3013,10 @@ _hidden void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
                              libxl_domain_config *d_config,
                              libxl__multidev *multidev);
 
+_hidden void libxl__add_vscsis(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+                               libxl_domain_config *d_config,
+                               libxl__multidev *multidev);
+
 _hidden void libxl__add_vtpms(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
                              libxl_domain_config *d_config,
                              libxl__multidev *multidev);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index b51930c..305948e 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -279,3 +279,63 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
 }
+
+int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, libxl_vscsi_hctl *hctl)
+{
+    struct stat dentry;
+    char *sysfs;
+    const char *type;
+    int rc, found = 0;
+    DIR *dirp;
+    struct dirent *de;
+
+    /* stat pdev to get device's sysfs entry */
+    if (stat (pdev, &dentry) < 0) {
+        LOG(ERROR, "vscsi: %s, device node not found", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (S_ISBLK (dentry.st_mode)) {
+        type = "block";
+    } else if (S_ISCHR (dentry.st_mode)) {
+        type = "char";
+    } else {
+        LOG(ERROR, "vscsi: %s, device node not a block or char device", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* /sys/dev/type/major:minor symlink added in 2.6.27 */
+    sysfs = GCSPRINTF("/sys/dev/%s/%u:%u/device/scsi_device", type,
+                      major(dentry.st_rdev), minor(dentry.st_rdev));
+
+    dirp = opendir(sysfs);
+    if (!dirp) {
+        LOG(ERROR, "vscsi: %s, no major:minor link in sysfs", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    while ((de = readdir(dirp))) {
+        if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
+            continue;
+
+        if (libxl_device_vscsi_parse_hctl(gc, de->d_name, hctl))
+            continue;
+
+        found = 1;
+        break;
+    }
+    closedir(dirp);
+
+    if (!found) {
+        LOG(ERROR, "vscsi: %s, no h:c:t:l link in sysfs", pdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+out:
+    return rc;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 898e160..b8972f0 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -95,3 +95,11 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
 {
     return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
 }
+
+int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
+                                unsigned int *chn, unsigned int *tgt,
+                                unsigned int *lun)
+{
+
+    return ERROR_NOPARAVIRT;
+}
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..e56f231 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -542,6 +542,37 @@ libxl_device_channel = Struct("device_channel", [
            ])),
 ])
 
+libxl_vscsi_pdev_type = Enumeration("vscsi_pdev_type", [
+    (0, "INVALID"),
+    (1, "DEV"),
+    (2, "WWN"),
+    (3, "HCTL"),
+    ], init_val = "LIBXL_VSCSI_PDEV_TYPE_INVALID")
+
+libxl_vscsi_hctl = Struct("vscsi_hctl", [
+    ("hst", uint32),
+    ("chn", uint32),
+    ("tgt", uint32),
+    ("lun", uint32),
+    ])
+
+libxl_vscsi_dev = Struct("vscsi_dev", [
+    ("vscsi_dev_id",     libxl_devid),
+    ("remove",           bool),
+    ("p_devname",        string),
+    ("pdev_type",        libxl_vscsi_pdev_type),
+    ("pdev",             libxl_vscsi_hctl),
+    ("vdev",             libxl_vscsi_hctl),
+    ])
+
+libxl_device_vscsi = Struct("device_vscsi", [
+    ("backend_domid",    libxl_domid),
+    ("devid",            libxl_devid),
+    ("v_hst",            uint32),
+    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
+    ("feature_host",     bool),
+    ])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -551,6 +582,7 @@ libxl_domain_config = Struct("domain_config", [
     ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
+    ("vscsis", Array(libxl_device_vscsi, "num_vscsis")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
     # a channel manifests as a console with a name,
     # see docs/misc/channels.txt
@@ -585,6 +617,23 @@ libxl_nicinfo = Struct("nicinfo", [
     ("rref_rx", integer),
     ], dir=DIR_OUT)
 
+libxl_vscsiinfo = Struct("vscsiinfo", [
+    ("backend", string),
+    ("backend_id", uint32),
+    ("frontend", string),
+    ("frontend_id", uint32),
+    ("devid", libxl_devid),
+    ("p_devname", string),
+    ("pdev", libxl_vscsi_hctl),
+    ("vdev", libxl_vscsi_hctl),
+    ("vscsi_dev_id", libxl_devid),
+    ("feature_host", bool),
+    ("vscsi_host_state", integer),
+    ("vscsi_dev_state", integer),
+    ("evtch", integer),
+    ("rref", integer),
+    ], dir=DIR_OUT)
+
 libxl_vtpminfo = Struct("vtpminfo", [
     ("backend", string),
     ("backend_id", uint32),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 5e55685..84c44f5 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -22,6 +22,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (6, "VKBD"),
     (7, "CONSOLE"),
     (8, "VTPM"),
+    (9, "VSCSI"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
new file mode 100644
index 0000000..ab4cb5f
--- /dev/null
+++ b/tools/libxl/libxl_vscsi.c
@@ -0,0 +1,375 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+
+int libxl_device_vscsi_parse_hctl(libxl__gc *gc, char *str, libxl_vscsi_hctl *hctl)
+{
+    unsigned int hst, chn, tgt, lun;
+
+    if (sscanf(str, "%u:%u:%u:%u", &hst, &chn, &tgt, &lun) != 4)
+        return ERROR_INVAL;
+
+    hctl->hst = hst;
+    hctl->chn = chn;
+    hctl->tgt = tgt;
+    hctl->lun = lun;
+    return 0;
+}
+
+static char *vscsi_trim_string(char *s)
+{
+    unsigned int len;
+
+    while (isspace(*s))
+        s++;
+    len = strlen(s);
+    while (len-- > 1 && isspace(s[len]))
+        s[len] = '\0';
+    return s;
+}
+
+static bool vscsi_wwn_valid(const char *p)
+{
+    bool ret = true;
+    int i = 0;
+
+    for (i = 0; i < 16; i++, p++) {
+        if (*p >= '0' && *p <= '9')
+            continue;
+        if (*p >= 'a' && *p <= 'f')
+            continue;
+        if (*p >= 'A' && *p <= 'F')
+            continue;
+        ret = false;
+        break;
+    }
+    return ret;
+}
+
+int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
+                             libxl_device_vscsi *new_host,
+                             libxl_vscsi_dev *new_dev)
+{
+    GC_INIT(ctx);
+    int rc;
+    unsigned int lun;
+    char wwn[16 + 1], *buf, *pdev, *vdev, *fhost;
+
+    buf = libxl__strdup(gc, cfg);
+
+    pdev = strtok(buf, ",");
+    vdev = strtok(NULL, ",");
+    fhost = strtok(NULL, ",");
+    if (!(pdev && vdev)) {
+        LOG(ERROR, "invalid vscsi= devspec: '%s'\n", cfg);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    pdev = vscsi_trim_string(pdev);
+    vdev = vscsi_trim_string(vdev);
+
+    new_dev->pdev_type = LIBXL_VSCSI_PDEV_TYPE_INVALID;
+    if (strncmp(pdev, "/dev/", 5) == 0) {
+        if (libxl_device_vscsi_parse_pdev(gc, pdev, &new_dev->pdev) == 0)
+            new_dev->pdev_type = LIBXL_VSCSI_PDEV_TYPE_DEV;
+    } else if (strncmp(pdev, "naa.", 4) == 0) {
+        memset(wwn, 0, sizeof(wwn));
+        if (sscanf(pdev, "naa.%16c:%u", wwn, &lun) == 2 && vscsi_wwn_valid(wwn))
+            new_dev->pdev_type = LIBXL_VSCSI_PDEV_TYPE_WWN;
+    } else if (libxl_device_vscsi_parse_hctl(gc, pdev, &new_dev->pdev) == 0) {
+        new_dev->pdev_type = LIBXL_VSCSI_PDEV_TYPE_HCTL;
+    }
+
+    switch (new_dev->pdev_type) {
+        case LIBXL_VSCSI_PDEV_TYPE_WWN:
+            new_dev->pdev.lun = lun;
+            /* Fall through.  */
+        case LIBXL_VSCSI_PDEV_TYPE_DEV:
+        case LIBXL_VSCSI_PDEV_TYPE_HCTL:
+            new_dev->p_devname = libxl__strdup(NOGC, pdev);
+            break;
+        case LIBXL_VSCSI_PDEV_TYPE_INVALID:
+            LOG(ERROR, "vscsi: invalid pdev '%s'", pdev);
+            rc = ERROR_INVAL;
+            goto out;
+    }
+
+
+    if (libxl_device_vscsi_parse_hctl(gc, vdev, &new_dev->vdev)) {
+        LOG(ERROR, "vscsi: invalid '%s', expecting hst:chn:tgt:lun", vdev);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* Record group index */
+    new_host->v_hst = new_dev->vdev.hst;
+
+    if (fhost) {
+        fhost = vscsi_trim_string(fhost);
+        new_host->feature_host = strcmp(fhost, "feature-host") == 0;
+        if (!new_host->feature_host) {
+            LOG(ERROR, "vscsi: invalid option '%s', expecting %s", fhost, "feature-host");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+    }
+    rc = 0;
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
+                                   libxl_vscsi_dev *dev)
+{
+    GC_INIT(ctx);
+    hst->vscsi_devs = libxl__realloc(NOGC, hst->vscsi_devs,
+                                     sizeof(*dev) * (hst->num_vscsi_devs + 1));
+    libxl_vscsi_dev_init(hst->vscsi_devs + hst->num_vscsi_devs);
+    dev->vscsi_dev_id = hst->num_vscsi_devs;
+    libxl_vscsi_dev_copy(ctx, hst->vscsi_devs + hst->num_vscsi_devs, dev);
+    hst->num_vscsi_devs++;
+    GC_FREE;
+}
+
+int libxl_device_vscsi_get_host(libxl_ctx *ctx, uint32_t domid, const char *cfg, libxl_device_vscsi **vscsi_host)
+{
+    GC_INIT(ctx);
+    libxl_vscsi_dev *new_dev = NULL;
+    libxl_device_vscsi *new_host, *vscsi_hosts = NULL, *tmp;
+    int rc, found_host = -1, i, j;
+    int num_hosts;
+
+    GCNEW(new_host);
+    libxl_device_vscsi_init(new_host);
+
+    GCNEW(new_dev);
+    libxl_vscsi_dev_init(new_dev);
+
+    if (libxl_device_vscsi_parse(ctx, cfg, new_host, new_dev)) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* FIXME: foreach domain, because pdev is not multiplexed by backend */
+    /* FIXME: other device types do not have the multiplexing issue */
+    /* FIXME: pci can solve it by unbinding the native driver */
+
+    /* Look for existing vscsi_host for given domain */
+    vscsi_hosts = libxl_device_vscsi_list(ctx, domid, &num_hosts);
+    if (vscsi_hosts) {
+        for (i = 0; i < num_hosts; ++i) {
+            for (j = 0; j < vscsi_hosts[i].num_vscsi_devs; j++) {
+                if (vscsi_hosts[i].vscsi_devs[j].pdev.hst == new_dev->pdev.hst &&
+                    vscsi_hosts[i].vscsi_devs[j].pdev.chn == new_dev->pdev.chn &&
+                    vscsi_hosts[i].vscsi_devs[j].pdev.tgt == new_dev->pdev.tgt &&
+                    vscsi_hosts[i].vscsi_devs[j].pdev.lun == new_dev->pdev.lun) {
+                    LOG(ERROR, "Host device '%u:%u:%u:%u' is already in use"
+                        " by guest vscsi specification '%u:%u:%u:%u'.\n",
+                        new_dev->pdev.hst, new_dev->pdev.chn, new_dev->pdev.tgt, new_dev->pdev.lun,
+                        new_dev->vdev.hst, new_dev->vdev.chn, new_dev->vdev.tgt, new_dev->vdev.lun);
+                    rc = ERROR_INVAL;
+                    goto out;
+                }
+            }
+            if (vscsi_hosts[i].v_hst == new_host->v_hst) {
+                found_host = i;
+                break;
+            }
+        }
+    }
+
+    if (found_host == -1) {
+        /* Not found, create new host */
+        new_host->devid = 0;
+        tmp = new_host;
+    } else {
+        tmp = vscsi_hosts + found_host;
+
+        /* Check if the vdev address is already taken */
+        for (i = 0; i < tmp->num_vscsi_devs; ++i) {
+            if (tmp->vscsi_devs[i].vdev.chn == new_dev->vdev.chn &&
+                tmp->vscsi_devs[i].vdev.tgt == new_dev->vdev.tgt &&
+                tmp->vscsi_devs[i].vdev.lun == new_dev->vdev.lun) {
+                fprintf(stderr, "Target vscsi specification '%u:%u:%u:%u' is already taken\n",
+                        new_dev->vdev.hst, new_dev->vdev.chn, new_dev->vdev.tgt, new_dev->vdev.lun);
+                rc = ERROR_INVAL;
+                goto out;
+            }
+        }
+    }
+
+    /* The caller gets a copy along with appended new_dev */
+    *vscsi_host = libxl__malloc(NOGC, sizeof(*new_host));
+    libxl_device_vscsi_init(*vscsi_host);
+    libxl_device_vscsi_copy(ctx, *vscsi_host, tmp);
+    libxl_device_vscsi_append_dev(ctx, *vscsi_host, new_dev);
+
+    rc = 0;
+
+out:
+    if (vscsi_hosts) {
+        for (i = 0; i < num_hosts; ++i){
+            libxl_device_vscsi_dispose(&vscsi_hosts[i]);
+        }
+        free(vscsi_hosts);
+    }
+    libxl_device_vscsi_dispose(new_host);
+    libxl_vscsi_dev_dispose(new_dev);
+    GC_FREE;
+    return rc;
+}
+
+libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num)
+{
+    GC_INIT(ctx);
+    libxl_vscsi_dev *v_dev;
+    libxl_device_vscsi *v_hst, *vscsi_hosts = NULL;
+    char *fe_path, *tmp, *c, *p, *v;
+    char **dir, **devs_dir;
+    const char *devs_path, *be_path;
+    int r;
+    bool parsed_ok;
+    unsigned int ndirs = 0, ndevs_dirs = 0, i;
+    unsigned int vscsi_dev_id;
+
+    fe_path = libxl__sprintf(gc, "%s/device/vscsi", libxl__xs_get_dompath(gc, domid));
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
+    /* Nothing to do */
+    if (!(dir && ndirs))
+        goto out;
+
+    /* List of hosts to be returned to the caller */
+    vscsi_hosts = libxl__malloc(NOGC, ndirs * sizeof(*vscsi_hosts));
+
+    /* Fill each host */
+    for (v_hst = vscsi_hosts; v_hst < vscsi_hosts + ndirs; ++v_hst, ++dir) {
+        libxl_device_vscsi_init(v_hst);
+
+        v_hst->devid = atoi(*dir);
+
+        tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend-id",
+                             fe_path, *dir));
+        /* FIXME what if xenstore is broken? */
+        if (tmp)
+            v_hst->backend_domid = atoi(tmp);
+
+        be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s/backend",
+                                 fe_path, *dir));
+        /* FIXME what if xenstore is broken? */
+        if (be_path) {
+            devs_path = libxl__sprintf(gc, "%s/vscsi-devs", be_path);
+            devs_dir = libxl__xs_directory(gc, XBT_NULL, devs_path, &ndevs_dirs);
+        } else {
+            devs_dir = NULL;
+        }
+
+        if (devs_dir && ndevs_dirs) {
+            v_hst->vscsi_devs = libxl__malloc(NOGC, ndevs_dirs * sizeof(*v_dev));
+            v_hst->num_vscsi_devs = ndevs_dirs;
+            /* Fill each device connected to the host */
+            for (i = 0; i < ndevs_dirs; i++, devs_dir++) {
+                v_dev = &v_hst->vscsi_devs[i];
+                libxl_vscsi_dev_init(v_dev);
+                parsed_ok = false;
+                r = sscanf(*devs_dir, "dev-%u", &vscsi_dev_id);
+                if (r == 1) {
+                    c = libxl__xs_read(gc, XBT_NULL,
+                                         GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname",
+                                         be_path, vscsi_dev_id));
+                    p = libxl__xs_read(gc, XBT_NULL,
+                                         GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev",
+                                         be_path, vscsi_dev_id));
+                    v = libxl__xs_read(gc, XBT_NULL,
+                                          GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev",
+                                          be_path, vscsi_dev_id));
+                    if (c && p && v) {
+                        v_dev->p_devname = libxl__strdup(NOGC, c);
+                        if (libxl_device_vscsi_parse_hctl(gc, p, &v_dev->pdev) == 0 &&
+                            libxl_device_vscsi_parse_hctl(gc, v, &v_dev->vdev) == 0)
+                            parsed_ok = true;
+                        v_dev->vscsi_dev_id = vscsi_dev_id;
+                        v_hst->v_hst = v_dev->vdev.hst;
+                    }
+                }
+
+                if (!parsed_ok) {
+                    /* FIXME what if xenstore is broken? */
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/scsi-devs/%s failed to parse", be_path, *devs_dir);
+                    continue;
+                }
+            }
+        }
+    }
+
+out:
+    *num = ndirs;
+
+    GC_FREE;
+    return vscsi_hosts;
+}
+
+int libxl_device_vscsi_getinfo(libxl_ctx *ctx, uint32_t domid,
+                               libxl_device_vscsi *vscsi_host,
+                               libxl_vscsi_dev *vscsi_dev,
+                               libxl_vscsiinfo *vscsiinfo)
+{
+    GC_INIT(ctx);
+    char *dompath, *vscsipath;
+    char *val;
+    int rc = ERROR_FAIL;
+
+    libxl_vscsiinfo_init(vscsiinfo);
+    dompath = libxl__xs_get_dompath(gc, domid);
+    vscsiinfo->devid = vscsi_host->devid;
+    libxl_vscsi_hctl_copy(ctx, &vscsiinfo->pdev, &vscsi_dev->pdev);
+    libxl_vscsi_hctl_copy(ctx, &vscsiinfo->vdev, &vscsi_dev->vdev);
+
+    vscsipath = GCSPRINTF("%s/device/vscsi/%d", dompath, vscsiinfo->devid);
+    vscsiinfo->backend = xs_read(ctx->xsh, XBT_NULL,
+                                 GCSPRINTF("%s/backend", vscsipath), NULL);
+    if (!vscsiinfo->backend)
+        goto out;
+    if(!libxl__xs_read(gc, XBT_NULL, vscsiinfo->backend))
+        goto out;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", vscsipath));
+    vscsiinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", vscsipath));
+    vscsiinfo->vscsi_host_state = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/event-channel", vscsipath));
+    vscsiinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/ring-ref", vscsipath));
+    vscsiinfo->rref = val ? strtoul(val, NULL, 10) : -1;
+
+    vscsiinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
+                                  GCSPRINTF("%s/frontend", vscsiinfo->backend), NULL);
+
+    val = libxl__xs_read(gc, XBT_NULL,
+                         GCSPRINTF("%s/frontend-id", vscsiinfo->backend));
+    vscsiinfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL,
+                         GCSPRINTF("%s/vscsi-devs/dev-%u/state",
+                         vscsiinfo->backend, vscsi_dev->vscsi_dev_id));
+    vscsiinfo->vscsi_dev_state = val ? strtoul(val, NULL, 10) : -1;
+
+    rc = 0;
+out:
+    GC_FREE;
+    return rc;
+}
+
+
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 5bc138c..5c82688 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -83,6 +83,9 @@ int main_channellist(int argc, char **argv);
 int main_blockattach(int argc, char **argv);
 int main_blocklist(int argc, char **argv);
 int main_blockdetach(int argc, char **argv);
+int main_vscsiattach(int argc, char **argv);
+int main_vscsilist(int argc, char **argv);
+int main_vscsidetach(int argc, char **argv);
 int main_vtpmattach(int argc, char **argv);
 int main_vtpmlist(int argc, char **argv);
 int main_vtpmdetach(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..da5d95b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -985,7 +985,7 @@ static void parse_config_data(const char *config_source,
     const char *buf;
     long l;
     XLU_Config *config;
-    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
+    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms, *vscsis;
     XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian;
     int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian;
     int pci_power_mgmt = 0;
@@ -1488,6 +1488,59 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list(config, "vscsi", &vscsis, 0, 0)) {
+        int num_vscsi_items = 0;
+        d_config->num_vscsis = 0;
+        d_config->vscsis = NULL;
+        while ((buf = xlu_cfg_get_listitem (vscsis, num_vscsi_items)) != NULL) {
+            libxl_vscsi_dev v_dev = { };
+            libxl_device_vscsi *tmp, v_hst = { };
+            bool hst_found = false;
+
+            /*
+             * #1: parse the devspec and place it in temporary host+dev part
+             * #2: find existing vscsi_host with number v_hst
+             *     if found, append the vscsi_dev to this vscsi_host
+             * #3: otherwise, create new vscsi_host and append vscsi_dev
+             * Note: v_hst does not represent the index named "num_vscsis",
+             *       it is a private index used just in the config file
+             */
+            libxl_device_vscsi_init(&v_hst);
+            libxl_vscsi_dev_init(&v_dev);
+
+            if (libxl_device_vscsi_parse(ctx, buf, &v_hst, &v_dev))
+                exit (-1);
+
+            if (d_config->num_vscsis) {
+                for (i = 0; i < d_config->num_vscsis; i++) {
+                    if (d_config->vscsis[i].v_hst == v_hst.v_hst) {
+                        tmp = &d_config->vscsis[i];
+                        libxl_device_vscsi_append_dev(ctx, tmp, &v_dev);
+                        hst_found = true;
+                        break;
+                    }
+                }
+            }
+
+            if (!hst_found || !d_config->num_vscsis) {
+                d_config->vscsis = realloc(d_config->vscsis, sizeof(v_hst) * (d_config->num_vscsis + 1));
+                tmp = &d_config->vscsis[d_config->num_vscsis];
+                libxl_device_vscsi_init(tmp);
+
+                v_hst.devid = d_config->num_vscsis;
+                libxl_device_vscsi_copy(ctx, tmp, &v_hst);
+
+                libxl_device_vscsi_append_dev(ctx, tmp, &v_dev);
+
+                d_config->num_vscsis++;
+            }
+
+            libxl_vscsi_dev_dispose(&v_dev);
+            libxl_device_vscsi_dispose(&v_hst);
+            num_vscsi_items++;
+        }
+    }
+
     if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
         d_config->num_vtpms = 0;
         d_config->vtpms = NULL;
@@ -6439,6 +6492,200 @@ int main_blockdetach(int argc, char **argv)
     return rc;
 }
 
+int main_vscsiattach(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt, rc;
+    libxl_device_vscsi *vscsi_host = NULL;
+    char *cfg = NULL, *feat_buf = NULL;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-attach", 1) {
+        /* No options */
+    }
+
+    if (argc < 4 || argc > 5) {
+        help("scsi-attach");
+        return 1;
+    }
+
+    if (libxl_domain_qualifier_to_domid(ctx, argv[optind], &domid) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
+        return 1;
+    }
+
+    optind++;
+
+    if (argc == 5) {
+        if (asprintf(&feat_buf, ",%s", argv[4]) < 0) {
+            perror("asprintf");
+            return 1;
+        }
+    }
+
+    if (asprintf(&cfg, "%s,%s%s", argv[2], argv[3], feat_buf ?: "") < 0) {
+        perror("asprintf");
+        rc = 1;
+        goto out;;
+    }
+
+    /* Parse config string and store result */
+    rc = libxl_device_vscsi_get_host(ctx, domid, cfg, &vscsi_host);
+    if (rc < 0)
+        goto out;
+
+    if (dryrun_only) {
+        char *json = libxl_device_vscsi_to_json(ctx, vscsi_host);
+        printf("vscsi: %s\n", json);
+        free(json);
+        if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
+        rc = 0;
+        goto out;
+    }
+
+    /* Finally add the device */
+    if (libxl_device_vscsi_add(ctx, domid, vscsi_host, NULL)) {
+        fprintf(stderr, "libxl_device_vscsi_add failed.\n");
+        rc = 1;
+        goto out;
+    }
+
+    rc = 0;
+out:
+    if (vscsi_host)
+        libxl_device_vscsi_dispose(vscsi_host);
+    free(vscsi_host);
+    free(cfg);
+    free(feat_buf);
+    return rc;
+}
+
+int main_vscsilist(int argc, char **argv)
+{
+    int opt;
+    uint32_t domid;
+    libxl_device_vscsi *vscsi_hosts;
+    libxl_vscsiinfo vscsiinfo;
+    int num_hosts, h, d;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-list", 1) {
+        /* No options */
+    }
+    if (argc < 2) {
+        help("scsi-list");
+        return 1;
+    }
+
+    /*      Idx  BE  state host p_hst v_hst state */
+    printf("%-3s %-3s %-5s %-5s %-10s %-10s %-5s\n",
+           "Idx", "BE", "state", "host", "phy-hctl", "vir-hctl", "devstate");
+    for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
+        if (libxl_domain_qualifier_to_domid(ctx, *argv, &domid) < 0) {
+            fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
+            continue;
+        }
+        if (!(vscsi_hosts = libxl_device_vscsi_list(ctx, domid, &num_hosts))) {
+            continue;
+        }
+        for (h = 0; h < num_hosts; ++h) {
+            for (d = 0; d < vscsi_hosts[h].num_vscsi_devs; d++) {
+                if (!libxl_device_vscsi_getinfo(ctx, domid, &vscsi_hosts[h], &vscsi_hosts[h].vscsi_devs[d], &vscsiinfo)) {
+                    char pdev[64], vdev[64];
+                    snprintf(pdev, sizeof(pdev), "%u:%u:%u:%u",
+                             vscsiinfo.pdev.hst, vscsiinfo.pdev.chn, vscsiinfo.pdev.tgt, vscsiinfo.pdev.lun);
+                    snprintf(vdev, sizeof(vdev), "%u:%u:%u:%u",
+                             vscsiinfo.vdev.hst, vscsiinfo.vdev.chn, vscsiinfo.vdev.tgt, vscsiinfo.vdev.lun);
+                    /*      Idx  BE  state Sta */
+                    printf("%-3d %-3d %-5d %-5d %-10s %-10s %d\n",
+                           vscsiinfo.devid,
+                           vscsiinfo.backend_id,
+                           vscsiinfo.vscsi_host_state,
+                           vscsiinfo.backend_id,
+                           pdev, vdev,
+                           vscsiinfo.vscsi_dev_state);
+
+                    libxl_vscsiinfo_dispose(&vscsiinfo);
+                }
+            }
+            libxl_device_vscsi_dispose(&vscsi_hosts[h]);
+        }
+        free(vscsi_hosts);
+
+    }
+
+    return 0;
+}
+
+int main_vscsidetach(int argc, char **argv)
+{
+    int opt;
+    libxl_vscsi_dev v_dev = { }, *vd;
+    libxl_device_vscsi v_hst = { }, *vh;
+    libxl_device_vscsi *vscsi_hosts;
+    char *tmp = NULL, *dom = argv[1], *vdev = argv[2];
+    uint32_t domid;
+    int num_hosts, h, d, found = 0;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "scsi-detach", 1) {
+        /* No options */
+    }
+
+    if (argc < 3) {
+        help("scsi-detach");
+        return 1;
+    }
+
+    if (libxl_domain_qualifier_to_domid(ctx, dom, &domid) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n", dom);
+        return 1;
+    }
+
+    vscsi_hosts = libxl_device_vscsi_list(ctx, domid, &num_hosts);
+    if (!vscsi_hosts)
+        return 0;
+
+    /* Create a dummy cfg */
+    if (asprintf(&tmp, "0:0:0:0,%s", vdev) < 0) {
+        perror("asprintf");
+        goto done;
+    }
+
+    libxl_vscsi_dev_init(&v_dev);
+    libxl_device_vscsi_init(&v_hst);
+    if (libxl_device_vscsi_parse(ctx, tmp, &v_hst, &v_dev))
+        goto done;
+
+    for (h = 0; h < num_hosts; ++h) {
+        vh = &vscsi_hosts[h];
+        for (d = 0; !found && d < vh->num_vscsi_devs; d++) {
+#define CMP(member) (vd->vdev.member == v_dev.vdev.member)
+            vd = &vh->vscsi_devs[d];
+            if (CMP(hst) && CMP(chn) && CMP(tgt) && CMP(lun)) {
+                if (vh->num_vscsi_devs > 1) {
+                    vd->remove = true;
+                    if (libxl_device_vscsi_add(ctx, domid, vh, 0)) {
+                        fprintf(stderr, "libxl_device_vscsi_remove failed.\n");
+                        goto done;
+                    }
+                } else {
+                    libxl_device_vscsi_remove(ctx, domid, vh, 0);
+                }
+                found = 1;
+            }
+#undef CMP
+        }
+    }
+    if (!found)
+        fprintf(stderr, "%s(%u) vdev %s does not exist in domain %s\n", __func__, __LINE__, vdev, dom);
+done:
+    if (vscsi_hosts) {
+        for (h = 0; h < num_hosts; ++h)
+            libxl_device_vscsi_dispose(&vscsi_hosts[h]);
+        free(vscsi_hosts);
+    }
+    free(tmp);
+    return !found;
+}
+
 int main_vtpmattach(int argc, char **argv)
 {
     int opt;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 22ab63b..54b7a3a 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -365,6 +365,21 @@ struct cmd_spec cmd_table[] = {
       "Destroy a domain's virtual block device",
       "<Domain> <DevId>",
     },
+    { "scsi-attach",
+      &main_vscsiattach, 1, 1,
+      "Attach a dom0 SCSI device to a domain.",
+      "<Domain> <PhysDevice> <VirtDevice>",
+    },
+    { "scsi-list",
+      &main_vscsilist, 0, 0,
+      "List all dom0 SCSI devices currently attached to a domain.",
+      "<Domain(s)>",
+    },
+    { "scsi-detach",
+      &main_vscsidetach, 0, 1,
+      "Detach a specified SCSI device from a domain.",
+      "<Domain> <VirtDevice>",
+    },
     { "vtpm-attach",
       &main_vtpmattach, 1, 1,
       "Create a new virtual TPM device",

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Wei Liu]

This is a very big patch, I've only skimmed it so far.

I think you need to fix some overly long lines. I won't mention them
individually inline.

Regarding all the parsing stuffs, you haven't defined vscsispec so I
cannot review it. You might want to look at
docs/misc/xl-disk-configuration.txt.

I'm not suggesting you have to write something like that, but
considering all the compatibility issues you might have you might
actually end up writing up a document like that.

Then after that, do you consider writing a lexer for vscsispec?
We have one for diskspec, see libxlu_disk_l.l. That might result in a
shorter patch?

On Fri, Mar 06, 2015 at 10:45:56AM +0100, Olaf Hering wrote:
> Port pvscsi support from xend to libxl. See pvscsi.txt for details.
> 
> Outstanding work is listed in the TODO section.
> 

There is no TODO section in this patch. :-)

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/Makefile                 |   1 +
>  tools/libxl/libxl.c                  | 172 ++++++++++++++++
>  tools/libxl/libxl.h                  |  29 +++
>  tools/libxl/libxl_create.c           |   1 +
>  tools/libxl/libxl_device.c           |   2 +
>  tools/libxl/libxl_freebsd.c          |   8 +
>  tools/libxl/libxl_internal.h         |  12 ++
>  tools/libxl/libxl_linux.c            |  60 ++++++
>  tools/libxl/libxl_netbsd.c           |   8 +
>  tools/libxl/libxl_types.idl          |  49 +++++
>  tools/libxl/libxl_types_internal.idl |   1 +
>  tools/libxl/libxl_vscsi.c            | 375 +++++++++++++++++++++++++++++++++++
>  tools/libxl/xl.h                     |   3 +
>  tools/libxl/xl_cmdimpl.c             | 249 ++++++++++++++++++++++-
>  tools/libxl/xl_cmdtable.c            |  15 ++

Better split this patch into two. One for libxl and one for xl.

Or you can even split it into three, one for introducing libxl types,
one for implementing functionalities in libxl and one for xl.

>  15 files changed, 984 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 7329521..9622d66 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -91,6 +91,7 @@ endif
>  LIBXL_LIBS += -lyajl
>  
>  LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
> +			libxl_vscsi.o \
>  			libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
>  			libxl_internal.o libxl_utils.o libxl_uuid.o \
>  			libxl_json.o libxl_aoutils.o libxl_numa.o \
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 088786e..73504b9 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1967,6 +1967,166 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
>  }
>  
>  /******************************************************************************/
> +static int libxl__device_from_vscsi(libxl__gc *gc, uint32_t domid,
> +                                    libxl_device_vscsi *vscsi,
> +                                    libxl__device *device)
> +{
> +    device->backend_domid = vscsi->backend_domid;
> +    device->devid         = vscsi->devid;
> +    device->domid         = domid;
> +    device->backend_kind  = LIBXL__DEVICE_KIND_VSCSI;
> +    device->kind          = LIBXL__DEVICE_KIND_VSCSI;
> +
> +    return 0;
> +}
> +
> +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> +                             libxl_device_vscsi *vscsi,
> +                             libxl__ao_device *aodev)

You need to update this domain's JSON configuration. Cf.
libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.

> +{
> +    STATE_AO_GC(aodev->ao);
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    flexarray_t *front;
> +    flexarray_t *back;
> +    libxl__device *device;
> +    char *be_path;
> +    unsigned int be_dirs = 0, rc, i;
> +
> +    if (vscsi->devid == -1) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    /* Prealloc key+value: 4 toplevel + 4 per device */
> +    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
> +    back = flexarray_make(gc, i, 1);
> +    front = flexarray_make(gc, 2 * 2, 1);
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> +    if ( rc != 0 ) goto out;
> +

Coding style. No space after ( and before ). You can even just use

     if (!rc) goto out;

> +    /* Check if backend device path is already present */
> +    be_path = libxl__device_backend_path(gc, device);
> +    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
> +        /* backend does not exist, create a new one */
> +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> +        flexarray_append_pair(back, "online", "1");
> +        flexarray_append_pair(back, "state", "1");
> +        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
> +
> +        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> +        flexarray_append_pair(front, "state", "1");
> +    }
> +
> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> +        /* Trigger removal, otherwise create new device */

Why do you want to trigger removal in "add" function?

> +        if (be_dirs) {
> +            unsigned int nb = 0;
> +            /* Preserve existing device */
> +            if (libxl__xs_directory(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id), &nb) && nb) {
> +                /* Trigger device removal by forwarding state to XenbusStateClosing */
> +                if (v->remove)
> +                    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "5");
> +                continue;
> +            }
> +        }
> +        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id), v->p_devname);
> +        switch (v->pdev_type) {
> +            case LIBXL_VSCSI_PDEV_TYPE_WWN:
[...]
> +    xs_transaction_t t;
> +retry_transaction:
> +    t = xs_transaction_start(ctx->xsh);
> +    libxl__xs_writev(gc, t, be_path,
> +                     libxl__xs_kvs_of_flexarray(gc, back, back->count));
> +    xs_write(ctx->xsh, t, GCSPRINTF("%s/state", be_path), "7", 2);
> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction;
> +        LOGE(ERROR, "xs transaction failed");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    libxl__wait_for_backend(gc, be_path, "4");
> +
> +retry_transaction2:
> +    t = xs_transaction_start(ctx->xsh);
> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> +        if (v->remove) {
> +            char *path, *val;
> +            path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +            val = libxl__xs_read(gc, t, path);
> +            if (val && strcmp(val, "6") == 0) {
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +            } else {
> +                LOGE(ERROR, "%s: %s has %s, expected 6", __func__, path, val);
> +            }
> +        }
> +    }
> +

Please avoid using goto to retry transaction.

> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction2;
> +        LOGE(ERROR, "xs transaction failed");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    /* As we are not adding new device, skip waiting for it */
> +    libxl__ao_complete(egc, aodev->ao, 0);
> +
> +    rc = 0;
> +out:
> +    aodev->rc = rc;
> +    if(rc) aodev->callback(egc, aodev);
> +    return;
> +}
> +
[...]
> +    GC_FREE;
> +    return rc;
> +}
> +
> +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> +                                   libxl_vscsi_dev *dev)
> +{

Can this only be an internal function? I.e. use libxl__ namespace.

Same comment applies to similar functions.

Wei.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Fri, Mar 06, Wei Liu wrote:

> I think you need to fix some overly long lines. I won't mention them
> individually inline.

Some are just copy&paste from other places. I will check what can be
trimmed.

> Regarding all the parsing stuffs, you haven't defined vscsispec so I
> cannot review it. You might want to look at
> docs/misc/xl-disk-configuration.txt.

Its in pvscsi.txt, not complete yet.

> I'm not suggesting you have to write something like that, but
> considering all the compatibility issues you might have you might
> actually end up writing up a document like that.

I will check if its needed.

> Then after that, do you consider writing a lexer for vscsispec?
> We have one for diskspec, see libxlu_disk_l.l. That might result in a
> shorter patch?

The bulk is not the parser but using the result of the parsing. Or do
you spot something that is better done with flex?

> On Fri, Mar 06, 2015 at 10:45:56AM +0100, Olaf Hering wrote:
> > Port pvscsi support from xend to libxl. See pvscsi.txt for details.
> > Outstanding work is listed in the TODO section.
> There is no TODO section in this patch. :-)

See the line above, its in pvscsi.txt.

> Better split this patch into two. One for libxl and one for xl.
> 
> Or you can even split it into three, one for introducing libxl types,
> one for implementing functionalities in libxl and one for xl.

Would that actually improve things? In another thread it was suggested
to have it all in a single patch to avoid jumping around between mails.

> > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> > +                             libxl_device_vscsi *vscsi,
> > +                             libxl__ao_device *aodev)
> You need to update this domain's JSON configuration. Cf.
> libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.

So I need to add my function to the comment near DEFINE_DEVICE_ADD()?
Or do you mean something else?

> > +{
> > +    STATE_AO_GC(aodev->ao);
> > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    flexarray_t *front;
> > +    flexarray_t *back;
> > +    libxl__device *device;
> > +    char *be_path;
> > +    unsigned int be_dirs = 0, rc, i;
> > +
> > +    if (vscsi->devid == -1) {
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> > +    /* Prealloc key+value: 4 toplevel + 4 per device */
> > +    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
> > +    back = flexarray_make(gc, i, 1);
> > +    front = flexarray_make(gc, 2 * 2, 1);
> > +
> > +    GCNEW(device);
> > +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> > +    if ( rc != 0 ) goto out;
> > +
> Coding style. No space after ( and before ). You can even just use
>      if (!rc) goto out;

Copy&paste from some similar code from staging-4.4:
libxl__device_vtpm_add

> > +    /* Check if backend device path is already present */
> > +    be_path = libxl__device_backend_path(gc, device);
> > +    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
> > +        /* backend does not exist, create a new one */
> > +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> > +        flexarray_append_pair(back, "online", "1");
> > +        flexarray_append_pair(back, "state", "1");
> > +        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
> > +
> > +        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> > +        flexarray_append_pair(front, "state", "1");
> > +    }
> > +
> > +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> > +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> > +        /* Trigger removal, otherwise create new device */
> Why do you want to trigger removal in "add" function?

Because the vscsi is a single_host:many_devices, contrary to
single_host:singe_device for other backends.

> Please avoid using goto to retry transaction.

Just copy&paste from other code, just grep for "retry_transaction".

> > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > +                                   libxl_vscsi_dev *dev)
> > +{
> 
> Can this only be an internal function? I.e. use libxl__ namespace.

Ok, will add the underscore.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Wei Liu]

On Fri, Mar 06, 2015 at 04:25:54PM +0100, Olaf Hering wrote:
> On Fri, Mar 06, Wei Liu wrote:
> 
> > I think you need to fix some overly long lines. I won't mention them
> > individually inline.
> 
> Some are just copy&paste from other places. I will check what can be
> trimmed.
> 
> > Regarding all the parsing stuffs, you haven't defined vscsispec so I
> > cannot review it. You might want to look at
> > docs/misc/xl-disk-configuration.txt.
> 
> Its in pvscsi.txt, not complete yet.
> 
> > I'm not suggesting you have to write something like that, but
> > considering all the compatibility issues you might have you might
> > actually end up writing up a document like that.
> 
> I will check if its needed.
> 
> > Then after that, do you consider writing a lexer for vscsispec?
> > We have one for diskspec, see libxlu_disk_l.l. That might result in a
> > shorter patch?
> 
> The bulk is not the parser but using the result of the parsing. Or do
> you spot something that is better done with flex?
> 

I didn't look into details. I had the impression that there were lots of
manual string parsing.

> > On Fri, Mar 06, 2015 at 10:45:56AM +0100, Olaf Hering wrote:
> > > Port pvscsi support from xend to libxl. See pvscsi.txt for details.
> > > Outstanding work is listed in the TODO section.
> > There is no TODO section in this patch. :-)
> 
> See the line above, its in pvscsi.txt.
> 
> > Better split this patch into two. One for libxl and one for xl.
> > 
> > Or you can even split it into three, one for introducing libxl types,
> > one for implementing functionalities in libxl and one for xl.
> 
> Would that actually improve things? In another thread it was suggested
> to have it all in a single patch to avoid jumping around between mails.

I think it would, because 1) it only requires me to focus on one
specific aspect, 2) it enables we commit work that is ready.

But I don't have very strong opinion on this, so you can keep using one
patch.

> 
> > > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> > > +                             libxl_device_vscsi *vscsi,
> > > +                             libxl__ao_device *aodev)
> > You need to update this domain's JSON configuration. Cf.
> > libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.
> 
> So I need to add my function to the comment near DEFINE_DEVICE_ADD()?
> Or do you mean something else?
> 

No. I mean you need to modify this function to comply with locking
scheme and update JSON template.

libxl_internal.h documents how this supposes to work.  You can use
libxl__device_vtpm_add as an example to see how it is actually
implemented.

See

2052     if (aodev->update_json) {
2053         lock = libxl__lock_domain_userdata(gc, domid);
2054         if (!lock) {
2055             rc = ERROR_LOCK_FAIL;
2056             goto out;
2057         }
2058
2059         rc = libxl__get_domain_configuration(gc, domid, &d_config);
2060         if (rc) goto out;
2061
2062         DEVICE_ADD(vtpm, vtpms, domid, &vtpm_saved, COMPARE_DEVID, &d_config);
2063     }
2064

That handles JSON template update when you do device hotplug.

> > > +{
> > > +    STATE_AO_GC(aodev->ao);
> > > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > > +    flexarray_t *front;
> > > +    flexarray_t *back;
> > > +    libxl__device *device;
> > > +    char *be_path;
> > > +    unsigned int be_dirs = 0, rc, i;
> > > +
> > > +    if (vscsi->devid == -1) {
> > > +        rc = ERROR_FAIL;
> > > +        goto out;
> > > +    }
> > > +
> > > +    /* Prealloc key+value: 4 toplevel + 4 per device */
> > > +    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
> > > +    back = flexarray_make(gc, i, 1);
> > > +    front = flexarray_make(gc, 2 * 2, 1);
> > > +
> > > +    GCNEW(device);
> > > +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> > > +    if ( rc != 0 ) goto out;
> > > +
> > Coding style. No space after ( and before ). You can even just use
> >      if (!rc) goto out;
> 
> Copy&paste from some similar code from staging-4.4:
> libxl__device_vtpm_add
> 

Sorry about the inconsistency in code.

Let's not introduce more code that violates coding style. :-)

> > > +    /* Check if backend device path is already present */
> > > +    be_path = libxl__device_backend_path(gc, device);
> > > +    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
> > > +        /* backend does not exist, create a new one */
> > > +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> > > +        flexarray_append_pair(back, "online", "1");
> > > +        flexarray_append_pair(back, "state", "1");
> > > +        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
> > > +
> > > +        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> > > +        flexarray_append_pair(front, "state", "1");
> > > +    }
> > > +
> > > +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> > > +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> > > +        /* Trigger removal, otherwise create new device */
> > Why do you want to trigger removal in "add" function?
> 
> Because the vscsi is a single_host:many_devices, contrary to
> single_host:singe_device for other backends.
> 

I need to do some more research before commenting on this.

> > Please avoid using goto to retry transaction.
> 
> Just copy&paste from other code, just grep for "retry_transaction".
> 

New code should use for or while to do this. We would very much like to
fix those goto loop but just haven't got there.

Wei.

> > > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > > +                                   libxl_vscsi_dev *dev)
> > > +{
> > 
> > Can this only be an internal function? I.e. use libxl__ namespace.
> 
> Ok, will add the underscore.
> 
> Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Fri, Mar 06, Wei Liu wrote:

> > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> > +                             libxl_device_vscsi *vscsi,
> > +                             libxl__ao_device *aodev)
> You need to update this domain's JSON configuration. Cf.
> libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.

Its not clear to me why every libxl__device_<type>_add copies the passed
in libxl_device_<type> into a local named <type>_saved. Why is it not
enough to work directly with the input? Its not obvious what the purpose
of that copy is, nothing seems to alter the input.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Wei Liu]

On Mon, Mar 09, 2015 at 05:08:31PM +0100, Olaf Hering wrote:
> On Fri, Mar 06, Wei Liu wrote:
> 
> > > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> > > +                             libxl_device_vscsi *vscsi,
> > > +                             libxl__ao_device *aodev)
> > You need to update this domain's JSON configuration. Cf.
> > libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.
> 
> Its not clear to me why every libxl__device_<type>_add copies the passed
> in libxl_device_<type> into a local named <type>_saved. Why is it not
> enough to work directly with the input? Its not obvious what the purpose
> of that copy is, nothing seems to alter the input.
> 

The input might be altered in the hotplug function, see
libxl__device_<type>_setdefault functions.

We want to save the original one. However the save need to happen later,
so we need to copy the original input and save that copy if necessary.

Wei.

> Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Fri, 2015-03-06 at 14:31 +0000, Wei Liu wrote:
> > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > +                                   libxl_vscsi_dev *dev)
> > +{
> 
> Can this only be an internal function? I.e. use libxl__ namespace.

And take a libxl__gc *gc not a libxl_ctx, except in very exception
circumstances.

> 
> Same comment applies to similar functions.
> 
> Wei.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bbc52d..1ad52e3 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h

Needs a LIBXL_HAVE define too.

> @@ -1224,6 +1224,35 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
>                                   libxl_device_channel *channel,
>                                   libxl_channelinfo *channelinfo);
>  
> +/* Virtual SCSI */
> +int libxl_device_vscsi_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vscsi *vscsi,
> +                           const libxl_asyncop_how *ao_how)
> +                           LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vscsi_remove(libxl_ctx *ctx, uint32_t domid,
> +                              libxl_device_vscsi *vscsi,
> +                              const libxl_asyncop_how *ao_how)
> +                              LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vscsi_destroy(libxl_ctx *ctx, uint32_t domid,
> +                               libxl_device_vscsi *vscsi,
> +                               const libxl_asyncop_how *ao_how)
> +                               LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num);
> +int libxl_device_vscsi_getinfo(libxl_ctx *ctx,
> +                               uint32_t domid,
> +                               libxl_device_vscsi *vscsi_host,
> +                               libxl_vscsi_dev *vscsi_dev,
> +                               libxl_vscsiinfo *vscsiinfo);

> +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> +                                   libxl_vscsi_dev *dev);
> +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> +                                uint32_t domid,
> +                                const char *cfg,
> +                                libxl_device_vscsi **vscsi_host);

What do these two non-standard functions do?

In general the caller would be expected to provide a libxl_device_vscsi,
which will be filled in, rather than having the function allocate one.

> +int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
> +                             libxl_device_vscsi *vscsi_host,
> +                             libxl_vscsi_dev *vscsi_dev);

Like with disk, this is xend/xl specific but might be of use to other
toolstacks, therefore it belongs in libxlu not in libxl proper.

(this might apply to get_host too, depending on what it does)

> +
>  /* Virtual TPMs */
>  int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
>                            const libxl_asyncop_how *ao_how)

> +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> +                                unsigned int *chn, unsigned int *tgt,
> +                                unsigned int *lun)
> +{
> +
> +    return ERROR_NOPARAVIRT;

That's a rather odd error code to use here.

(also, unnecessary blank line)

> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 898e160..b8972f0 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -95,3 +95,11 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
>  {
>      return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
>  }
> +
> +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> +                                unsigned int *chn, unsigned int *tgt,
> +                                unsigned int *lun)
> +{
> +
> +    return ERROR_NOPARAVIRT;

As before.

> +}
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 47af340..e56f231 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -542,6 +542,37 @@ libxl_device_channel = Struct("device_channel", [
>             ])),
>  ])
>  
> +libxl_vscsi_pdev_type = Enumeration("vscsi_pdev_type", [
> +    (0, "INVALID"),
> +    (1, "DEV"),
> +    (2, "WWN"),
> +    (3, "HCTL"),
> +    ], init_val = "LIBXL_VSCSI_PDEV_TYPE_INVALID")

You don't need init_val if it happens to be zero.

> +
> +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> +    ("hst", uint32),
> +    ("chn", uint32),
> +    ("tgt", uint32),
> +    ("lun", uint32),
> +    ])
> +
> +libxl_vscsi_dev = Struct("vscsi_dev", [
> +    ("vscsi_dev_id",     libxl_devid),
> +    ("remove",           bool),
> +    ("p_devname",        string),
> +    ("pdev_type",        libxl_vscsi_pdev_type),
> +    ("pdev",             libxl_vscsi_hctl),
> +    ("vdev",             libxl_vscsi_hctl),

Are these last two valid for LIBXL_VSCSI_PDEV_TYPE != HCTL?

> +    ])
> +
> +libxl_device_vscsi = Struct("device_vscsi", [
> +    ("backend_domid",    libxl_domid),
> +    ("devid",            libxl_devid),
> +    ("v_hst",            uint32),
> +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> +    ("feature_host",     bool),

What is this feature thing? What does !host imply?

> diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
> new file mode 100644
> index 0000000..ab4cb5f
> --- /dev/null
> +++ b/tools/libxl/libxl_vscsi.c

Needs a copyright and a license.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Wed, Mar 11, Ian Campbell wrote:

> On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > +                                   libxl_vscsi_dev *dev);
> > +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> > +                                uint32_t domid,
> > +                                const char *cfg,
> > +                                libxl_device_vscsi **vscsi_host);
> What do these two non-standard functions do?
> 
> In general the caller would be expected to provide a libxl_device_vscsi,
> which will be filled in, rather than having the function allocate one.
> 
> > +int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
> > +                             libxl_device_vscsi *vscsi_host,
> > +                             libxl_vscsi_dev *vscsi_dev);
> 
> Like with disk, this is xend/xl specific but might be of use to other
> toolstacks, therefore it belongs in libxlu not in libxl proper.
> (this might apply to get_host too, depending on what it does)

libvirt for example will do the very same what xl currently does. But so
far I did not get to libvirt yet. The interface is not finished yet.

Last time I looked at libxl vs. libxlu it introduced a circular
dependency. Thats why all the simple string parsing is in libxl itself.
But I will double check if the current layout allows a split. Basically
I tried to simplify things for the caller and have all the functionality
in libxl before libxl_device_vscsi is passed to
libxl_device_vscsi_{add,remove).

I will address the other comments as well in the next round.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Wed, 2015-03-11 at 17:02 +0100, Olaf Hering wrote:
> On Wed, Mar 11, Ian Campbell wrote:
> 
> > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > > +                                   libxl_vscsi_dev *dev);
> > > +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> > > +                                uint32_t domid,
> > > +                                const char *cfg,
> > > +                                libxl_device_vscsi **vscsi_host);
> > What do these two non-standard functions do?
> > 
> > In general the caller would be expected to provide a libxl_device_vscsi,
> > which will be filled in, rather than having the function allocate one.
> > 
> > > +int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
> > > +                             libxl_device_vscsi *vscsi_host,
> > > +                             libxl_vscsi_dev *vscsi_dev);
> > 
> > Like with disk, this is xend/xl specific but might be of use to other
> > toolstacks, therefore it belongs in libxlu not in libxl proper.
> > (this might apply to get_host too, depending on what it does)
> 
> libvirt for example will do the very same what xl currently does. But so
> far I did not get to libvirt yet. The interface is not finished yet.

That sort of "libvirt uses some xm/xl style syntax" is exactly why
libxlu exists.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Wed, Mar 11, Ian Campbell wrote:

> On Wed, 2015-03-11 at 17:02 +0100, Olaf Hering wrote:
> > On Wed, Mar 11, Ian Campbell wrote:
> > 
> > > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > > > +                                   libxl_vscsi_dev *dev);
> > > > +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> > > > +                                uint32_t domid,
> > > > +                                const char *cfg,
> > > > +                                libxl_device_vscsi **vscsi_host);
> > > What do these two non-standard functions do?
> > > 
> > > In general the caller would be expected to provide a libxl_device_vscsi,
> > > which will be filled in, rather than having the function allocate one.
> > > 
> > > > +int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
> > > > +                             libxl_device_vscsi *vscsi_host,
> > > > +                             libxl_vscsi_dev *vscsi_dev);
> > > Like with disk, this is xend/xl specific but might be of use to other
> > > toolstacks, therefore it belongs in libxlu not in libxl proper.
> > > (this might apply to get_host too, depending on what it does)
> > libvirt for example will do the very same what xl currently does. But so
> > far I did not get to libvirt yet. The interface is not finished yet.
> That sort of "libvirt uses some xm/xl style syntax" is exactly why
> libxlu exists.

I moved these functions to libxlu now. The only function called by libxl
and libxlu was libxl__device_vscsi_parse_hctl. I just duplicated that
function and made it static in their source files for the time being.


Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Wed, Mar 11, Ian Campbell wrote:

> On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > +                                   libxl_vscsi_dev *dev);
> > +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> > +                                uint32_t domid,
> > +                                const char *cfg,
> > +                                libxl_device_vscsi **vscsi_host);
> What do these two non-standard functions do?

append_dev handles the single_host:many_devices layout, append yet
another libxl_vscsi_dev to libxl_device_vscsi.

get_host either finds an existing libxl_device_vscsi or creates a new
one, to be passed to libxl_device_vscsi_add.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Wed, Mar 11, Ian Campbell wrote:

> On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > +                                   libxl_vscsi_dev *dev);
> > +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> > +                                uint32_t domid,
> > +                                const char *cfg,
> > +                                libxl_device_vscsi **vscsi_host);
> What do these two non-standard functions do?
> In general the caller would be expected to provide a libxl_device_vscsi,
> which will be filled in, rather than having the function allocate one.

Is it OK if the caller allocates libxl_device_vscsi, but libxl does
further allocations for the number of libxl_vscsi_dev?

Related: I see libxl_device_vscsi_dispose does now a pointer check, so I
assume its required to validate input in libxl_device_vscsi_get_host.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Thu, 2015-03-12 at 17:07 +0100, Olaf Hering wrote:
> On Wed, Mar 11, Ian Campbell wrote:
> 
> > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > > +                                   libxl_vscsi_dev *dev);
> > > +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> > > +                                uint32_t domid,
> > > +                                const char *cfg,
> > > +                                libxl_device_vscsi **vscsi_host);
> > What do these two non-standard functions do?
> > In general the caller would be expected to provide a libxl_device_vscsi,
> > which will be filled in, rather than having the function allocate one.
> 
> Is it OK if the caller allocates libxl_device_vscsi, but libxl does
> further allocations for the number of libxl_vscsi_dev?

Yes, we do that a lot.

See also the comment about memory allocation in libxl.h

> Related: I see libxl_device_vscsi_dispose does now a pointer check, so I
> assume its required to validate input in libxl_device_vscsi_get_host.

Not sure what you mean, you should call libxl_device_vcsci_init before
using the object and then _dispose when you are done.

Ian.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Thu, Mar 12, Ian Campbell wrote:

> On Thu, 2015-03-12 at 17:07 +0100, Olaf Hering wrote:
> > Related: I see libxl_device_vscsi_dispose does now a pointer check, so I
> > assume its required to validate input in libxl_device_vscsi_get_host.
> Not sure what you mean, you should call libxl_device_vcsci_init before
> using the object and then _dispose when you are done.

I mean NULL pointer checks, will do that for at least *vscsi_host.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Wei Liu]

On Fri, Mar 13, 2015 at 02:45:28PM +0100, Olaf Hering wrote:
> On Thu, Mar 12, Ian Campbell wrote:
> 
> > On Thu, 2015-03-12 at 17:07 +0100, Olaf Hering wrote:
> > > Related: I see libxl_device_vscsi_dispose does now a pointer check, so I
> > > assume its required to validate input in libxl_device_vscsi_get_host.
> > Not sure what you mean, you should call libxl_device_vcsci_init before
> > using the object and then _dispose when you are done.
> 
> I mean NULL pointer checks, will do that for at least *vscsi_host.
> 

That is only to make _dispose function NULL-tolerant. Why does that
affect your implementation?

Wei.

> Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Fri, Mar 13, Wei Liu wrote:

> On Fri, Mar 13, 2015 at 02:45:28PM +0100, Olaf Hering wrote:
> > On Thu, Mar 12, Ian Campbell wrote:
> > 
> > > On Thu, 2015-03-12 at 17:07 +0100, Olaf Hering wrote:
> > > > Related: I see libxl_device_vscsi_dispose does now a pointer check, so I
> > > > assume its required to validate input in libxl_device_vscsi_get_host.
> > > Not sure what you mean, you should call libxl_device_vcsci_init before
> > > using the object and then _dispose when you are done.
> > 
> > I mean NULL pointer checks, will do that for at least *vscsi_host.
> > 
> 
> That is only to make _dispose function NULL-tolerant. Why does that
> affect your implementation?

I have not checked other code pats in libxl, but does libxl expect valid
input? At least glibc for example expects valid input, str*(NULL) crashes.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Wei Liu]

On Mon, Mar 16, 2015 at 09:16:31AM +0100, Olaf Hering wrote:
> On Fri, Mar 13, Wei Liu wrote:
> 
> > On Fri, Mar 13, 2015 at 02:45:28PM +0100, Olaf Hering wrote:
> > > On Thu, Mar 12, Ian Campbell wrote:
> > > 
> > > > On Thu, 2015-03-12 at 17:07 +0100, Olaf Hering wrote:
> > > > > Related: I see libxl_device_vscsi_dispose does now a pointer check, so I
> > > > > assume its required to validate input in libxl_device_vscsi_get_host.
> > > > Not sure what you mean, you should call libxl_device_vcsci_init before
> > > > using the object and then _dispose when you are done.
> > > 
> > > I mean NULL pointer checks, will do that for at least *vscsi_host.
> > > 
> > 
> > That is only to make _dispose function NULL-tolerant. Why does that
> > affect your implementation?
> 
> I have not checked other code pats in libxl, but does libxl expect valid
> input? At least glibc for example expects valid input, str*(NULL) crashes.
> 

At the very least that's not the reason _dispose checks for NULL.
Dispose function checks for NULL so that you don't need to do the extra
check in caller like  "if(xx) _dispose(xx);"

Most if not all libxl APIs assume that the input is valid, because
following our paradigm, you must call _init function before using a
type. At the point you pass a type to libxl functions it should already
be initialised (which means it is valid).

Wei.

> Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Wed, Mar 11, Ian Campbell wrote:

> On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> > +                                unsigned int *chn, unsigned int *tgt,
> > +                                unsigned int *lun)
> > +{
> > +
> > +    return ERROR_NOPARAVIRT;
> That's a rather odd error code to use here.
> 
> (also, unnecessary blank line)

What error should be returned?

Looks like this function can be moved to xlu. Should I introduce a
libxlu_$(OS).c or just use #ifdef in libxlu_vscsi.c?

> > +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> > +    ("hst", uint32),
> > +    ("chn", uint32),
> > +    ("tgt", uint32),
> > +    ("lun", uint32),
> > +    ])
> > +
> > +libxl_vscsi_dev = Struct("vscsi_dev", [
> > +    ("vscsi_dev_id",     libxl_devid),
> > +    ("remove",           bool),
> > +    ("p_devname",        string),
> > +    ("pdev_type",        libxl_vscsi_pdev_type),
> > +    ("pdev",             libxl_vscsi_hctl),
> > +    ("vdev",             libxl_vscsi_hctl),
> 
> Are these last two valid for LIBXL_VSCSI_PDEV_TYPE != HCTL?

At least for vdev, which is always in HCTL format.

pdev is kind of orphan in case of LIBXL_VSCSI_PDEV_TYPE_WWN. The code
records pdev.lun somewhere, just in case. But in the end the LUN is
already part of the cfg string so nothing needs to make use of pdev
anymore.

> > +    ])
> > +
> > +libxl_device_vscsi = Struct("device_vscsi", [
> > +    ("backend_domid",    libxl_domid),
> > +    ("devid",            libxl_devid),
> > +    ("v_hst",            uint32),
> > +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> > +    ("feature_host",     bool),
> 
> What is this feature thing? What does !host imply?

This enables raw SCSI command passthrough in xenlinux. If the flag is
off then each command is checked. The check allows many commands to
passthrough, one command needs emulation and unhandled commands are
rejected. The flag is a noop in pvops because it doesnt look at such
flag. I was told its not required, dont know the details.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Thu, 2015-03-12 at 17:20 +0100, Olaf Hering wrote:
> On Wed, Mar 11, Ian Campbell wrote:
> 
> > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> > > +                                unsigned int *chn, unsigned int *tgt,
> > > +                                unsigned int *lun)
> > > +{
> > > +
> > > +    return ERROR_NOPARAVIRT;
> > That's a rather odd error code to use here.
> > 
> > (also, unnecessary blank line)
> 
> What error should be returned?

Take a look at the list and find one that fits, or look at other similar
stub functions. If there is no suitable existing error code then feel
free to add one.

> Looks like this function can be moved to xlu. Should I introduce a
> libxlu_$(OS).c or just use #ifdef in libxlu_vscsi.c?

I think we could live with either.

> > > +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> > > +    ("hst", uint32),
> > > +    ("chn", uint32),
> > > +    ("tgt", uint32),
> > > +    ("lun", uint32),
> > > +    ])
> > > +
> > > +libxl_vscsi_dev = Struct("vscsi_dev", [
> > > +    ("vscsi_dev_id",     libxl_devid),
> > > +    ("remove",           bool),
> > > +    ("p_devname",        string),
> > > +    ("pdev_type",        libxl_vscsi_pdev_type),
> > > +    ("pdev",             libxl_vscsi_hctl),
> > > +    ("vdev",             libxl_vscsi_hctl),
> > 
> > Are these last two valid for LIBXL_VSCSI_PDEV_TYPE != HCTL?
> 
> At least for vdev, which is always in HCTL format.
> 
> pdev is kind of orphan in case of LIBXL_VSCSI_PDEV_TYPE_WWN. The code
> records pdev.lun somewhere, just in case. But in the end the LUN is
> already part of the cfg string so nothing needs to make use of pdev
> anymore.

Perhaps pdev should be in the !WWN part of a KeyedUnion then?

> 
> > > +    ])
> > > +
> > > +libxl_device_vscsi = Struct("device_vscsi", [
> > > +    ("backend_domid",    libxl_domid),
> > > +    ("devid",            libxl_devid),
> > > +    ("v_hst",            uint32),
> > > +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> > > +    ("feature_host",     bool),
> > 
> > What is this feature thing? What does !host imply?
> 
> This enables raw SCSI command passthrough in xenlinux. If the flag is
> off then each command is checked. The check allows many commands to
> passthrough, one command needs emulation and unhandled commands are
> rejected. The flag is a noop in pvops because it doesnt look at such
> flag. I was told its not required, dont know the details.

Sounds like it at the very least needs a better name.

But if you don't really know what a thing is for or what the details are
then it would probably be best just to leave it out until someone who
does know comes along with a followup patch.

Ian.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Thu, Mar 12, Ian Campbell wrote:

> On Thu, 2015-03-12 at 17:20 +0100, Olaf Hering wrote:
> > On Wed, Mar 11, Ian Campbell wrote:
> > 
> > > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > > +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> > > > +                                unsigned int *chn, unsigned int *tgt,
> > > > +                                unsigned int *lun)
> > > > +{
> > > > +
> > > > +    return ERROR_NOPARAVIRT;
> > > That's a rather odd error code to use here.
> > What error should be returned?
> Take a look at the list and find one that fits, or look at other similar
> stub functions. If there is no suitable existing error code then feel
> free to add one.

NOPARAVIRT looked like a perfect fit, there are no PV drivers for BSD. I
will use something else then.

> > Looks like this function can be moved to xlu. Should I introduce a
> > libxlu_$(OS).c or just use #ifdef in libxlu_vscsi.c?
> I think we could live with either.

Ok, will check what looks best, and where to bail out if vscsi= is used
on BSD.

> > > > +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> > > > +    ("hst", uint32),
> > > > +    ("chn", uint32),
> > > > +    ("tgt", uint32),
> > > > +    ("lun", uint32),
> > > > +    ])
> > > > +libxl_vscsi_dev = Struct("vscsi_dev", [
> > > > +    ("vscsi_dev_id",     libxl_devid),
> > > > +    ("remove",           bool),
> > > > +    ("p_devname",        string),
> > > > +    ("pdev_type",        libxl_vscsi_pdev_type),
> > > > +    ("pdev",             libxl_vscsi_hctl),
> > > > +    ("vdev",             libxl_vscsi_hctl),
> > > Are these last two valid for LIBXL_VSCSI_PDEV_TYPE != HCTL?
> > At least for vdev, which is always in HCTL format.
> > pdev is kind of orphan in case of LIBXL_VSCSI_PDEV_TYPE_WWN. The code
> > records pdev.lun somewhere, just in case. But in the end the LUN is
> > already part of the cfg string so nothing needs to make use of pdev
> > anymore.
> Perhaps pdev should be in the !WWN part of a KeyedUnion then?

Its not clear to me how a Union would look like. I will study the other
usages. From a quick look the layout may look like:

DEV { pdev, vdev }
WWN { string, vdev }
HCTL { pdev, vdev }

> > > > +    ])
> > > > +
> > > > +libxl_device_vscsi = Struct("device_vscsi", [
> > > > +    ("backend_domid",    libxl_domid),
> > > > +    ("devid",            libxl_devid),
> > > > +    ("v_hst",            uint32),
> > > > +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> > > > +    ("feature_host",     bool),
> > > What is this feature thing? What does !host imply?
> > This enables raw SCSI command passthrough in xenlinux. If the flag is
> > off then each command is checked. The check allows many commands to
> > passthrough, one command needs emulation and unhandled commands are
> > rejected. The flag is a noop in pvops because it doesnt look at such
> > flag. I was told its not required, dont know the details.
> Sounds like it at the very least needs a better name.

Ok, will rename it to feature_noemul or feature_xenlinux_raw_scsicmd or
whatever.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Fri, 2015-03-13 at 14:44 +0100, Olaf Hering wrote:
> On Thu, Mar 12, Ian Campbell wrote:
> 
> > On Thu, 2015-03-12 at 17:20 +0100, Olaf Hering wrote:
> > > On Wed, Mar 11, Ian Campbell wrote:
> > > 
> > > > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > > > +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> > > > > +                                unsigned int *chn, unsigned int *tgt,
> > > > > +                                unsigned int *lun)
> > > > > +{
> > > > > +
> > > > > +    return ERROR_NOPARAVIRT;
> > > > That's a rather odd error code to use here.
> > > What error should be returned?
> > Take a look at the list and find one that fits, or look at other similar
> > stub functions. If there is no suitable existing error code then feel
> > free to add one.
> 
> NOPARAVIRT looked like a perfect fit, there are no PV drivers for BSD.

Which has no impact on the ability to parse a pdev or not.

(Also this error code is used when it is not possible to speak the pv
control protocol, but you weren't to know that)

> I will use something else then.

Thanks.

> > > > > +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> > > > > +    ("hst", uint32),
> > > > > +    ("chn", uint32),
> > > > > +    ("tgt", uint32),
> > > > > +    ("lun", uint32),
> > > > > +    ])
> > > > > +libxl_vscsi_dev = Struct("vscsi_dev", [
> > > > > +    ("vscsi_dev_id",     libxl_devid),
> > > > > +    ("remove",           bool),
> > > > > +    ("p_devname",        string),
> > > > > +    ("pdev_type",        libxl_vscsi_pdev_type),
> > > > > +    ("pdev",             libxl_vscsi_hctl),
> > > > > +    ("vdev",             libxl_vscsi_hctl),
> > > > Are these last two valid for LIBXL_VSCSI_PDEV_TYPE != HCTL?
> > > At least for vdev, which is always in HCTL format.
> > > pdev is kind of orphan in case of LIBXL_VSCSI_PDEV_TYPE_WWN. The code
> > > records pdev.lun somewhere, just in case. But in the end the LUN is
> > > already part of the cfg string so nothing needs to make use of pdev
> > > anymore.
> > Perhaps pdev should be in the !WWN part of a KeyedUnion then?
> 
> Its not clear to me how a Union would look like. I will study the other
> usages. From a quick look the layout may look like:
> 
> DEV { pdev, vdev }
> WWN { string, vdev }
> HCTL { pdev, vdev }

I don't think vdev should be in the union.

I don't know what the keys are so I can't comment on what should be in
them.

> 
> > > > > +    ])
> > > > > +
> > > > > +libxl_device_vscsi = Struct("device_vscsi", [
> > > > > +    ("backend_domid",    libxl_domid),
> > > > > +    ("devid",            libxl_devid),
> > > > > +    ("v_hst",            uint32),
> > > > > +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> > > > > +    ("feature_host",     bool),
> > > > What is this feature thing? What does !host imply?
> > > This enables raw SCSI command passthrough in xenlinux. If the flag is
> > > off then each command is checked. The check allows many commands to
> > > passthrough, one command needs emulation and unhandled commands are
> > > rejected. The flag is a noop in pvops because it doesnt look at such
> > > flag. I was told its not required, dont know the details.
> > Sounds like it at the very least needs a better name.
> 
> Ok, will rename it to feature_noemul or feature_xenlinux_raw_scsicmd or
> whatever.

Neither of those appear to be a good description of what you say quoted
above.

It sounds somewhat analogous to the pciback permissive mode, but I don't
know that for sure.

Also, should this be a defbool or not?

Ian.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Fri, Mar 13, Ian Campbell wrote:

> > > > > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > > > > +    ("feature_host",     bool),
> > > > > What is this feature thing? What does !host imply?
> > > > This enables raw SCSI command passthrough in xenlinux. If the flag is
> > > > off then each command is checked. The check allows many commands to
> > > > passthrough, one command needs emulation and unhandled commands are
> > > > rejected. The flag is a noop in pvops because it doesnt look at such
> > > > flag. I was told its not required, dont know the details.
> > > Sounds like it at the very least needs a better name.
> > Ok, will rename it to feature_noemul or feature_xenlinux_raw_scsicmd or
> > whatever.
> Neither of those appear to be a good description of what you say quoted
> above.

Ian, after some thought its not clear to me why the name "feature_host"
should be changed. The config option is "feature-host", the backend
expects a property named "feature-host". So it seems natural to also
name the variable that way. In the end it just passes a boolean from the
config to xenstore, nothing else is done with it.

> Also, should this be a defbool or not?

This is a good idea. I remmeber xend had code to make sure that all
devices within a vhost had the same value for "feature-host". I think
the intention was that whole physical hosts are passed to a domU. A
defbool makes it easier to preserve the initial value based on the first
config item.

Olaf

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Ian Campbell]

On Thu, 2015-03-26 at 13:55 +0100, Olaf Hering wrote:
> On Fri, Mar 13, Ian Campbell wrote:
> 
> > > > > > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > > > > > +    ("feature_host",     bool),
> > > > > > What is this feature thing? What does !host imply?
> > > > > This enables raw SCSI command passthrough in xenlinux. If the flag is
> > > > > off then each command is checked. The check allows many commands to
> > > > > passthrough, one command needs emulation and unhandled commands are
> > > > > rejected. The flag is a noop in pvops because it doesnt look at such
> > > > > flag. I was told its not required, dont know the details.
> > > > Sounds like it at the very least needs a better name.
> > > Ok, will rename it to feature_noemul or feature_xenlinux_raw_scsicmd or
> > > whatever.
> > Neither of those appear to be a good description of what you say quoted
> > above.
> 
> Ian, after some thought its not clear to me why the name "feature_host"
> should be changed. The config option is "feature-host", the backend
> expects a property named "feature-host". So it seems natural to also
> name the variable that way. In the end it just passes a boolean from the
> config to xenstore, nothing else is done with it.

Regardless of the current internal details feature_host gives me no clue
at all what the flag does, and this is a public facing API.

If not a better name then some docs (or a pointer to them) would help.

> > Also, should this be a defbool or not?
> 
> This is a good idea. I remmeber xend had code to make sure that all
> devices within a vhost had the same value for "feature-host". I think
> the intention was that whole physical hosts are passed to a domU. A
> defbool makes it easier to preserve the initial value based on the first
> config item.

In which case it sounds to me like this flag should be at a per-vhost
level and not at a per device level.

Ian.

Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering]

On Thu, Mar 26, Ian Campbell wrote:

> On Thu, 2015-03-26 at 13:55 +0100, Olaf Hering wrote:
> > On Fri, Mar 13, Ian Campbell wrote:
> > > Also, should this be a defbool or not?
> > This is a good idea. I remmeber xend had code to make sure that all
> > devices within a vhost had the same value for "feature-host". I think
> > the intention was that whole physical hosts are passed to a domU. A
> > defbool makes it easier to preserve the initial value based on the first
> > config item.
> 
> In which case it sounds to me like this flag should be at a per-vhost
> level and not at a per device level.

Its per vhost already, "libxl_device_vscsi" refers to the virtual scsi
host, and "libxl_vscsi_dev" refers to a device connected to that host.
Perhaps the latter needs a better name to make the difference more
obvious.

Olaf

wating for backend changes (was Re: [PATCH v3 4/4] libxl: add support for vscsi)

[Olaf Hering]

How is new code supposed to wait for backend changes?

Right now there are two APIs for that:
- libxl__wait_for_backend loops for a while until it returns an error.
- libxl__ev_devstate_wait registers a watch and a timer.


In case of pvscsi there are three variants:
- new, can use libxl__wait_device_connection
- reconfigure, can use both of the above
- remove, may use DEFINE_DEVICE_REMOVE and its
  libxl__initiate_device_remove

The reconfigure case has to wait for various states, depending on the
state before the reconfiguration.

The pci code in libxl uses just libxl__wait_for_backend. Looking through
the history it seems the function was added just for pci. Is it a
deprecated function, should callers get converted to
libxl__ev_devstate_wait?

Olaf

On Fri, Mar 06, Olaf Hering wrote:

> +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> +                             libxl_device_vscsi *vscsi,
> +                             libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    flexarray_t *front;
> +    flexarray_t *back;
> +    libxl__device *device;
> +    char *be_path;
> +    unsigned int be_dirs = 0, rc, i;
> +
> +    if (vscsi->devid == -1) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    /* Prealloc key+value: 4 toplevel + 4 per device */
> +    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
> +    back = flexarray_make(gc, i, 1);
> +    front = flexarray_make(gc, 2 * 2, 1);
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> +    if ( rc != 0 ) goto out;
> +
> +    /* Check if backend device path is already present */
> +    be_path = libxl__device_backend_path(gc, device);
> +    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
> +        /* backend does not exist, create a new one */
> +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> +        flexarray_append_pair(back, "online", "1");
> +        flexarray_append_pair(back, "state", "1");
> +        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
> +
> +        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> +        flexarray_append_pair(front, "state", "1");
> +    }
> +
> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> +        /* Trigger removal, otherwise create new device */
> +        if (be_dirs) {
> +            unsigned int nb = 0;
> +            /* Preserve existing device */
> +            if (libxl__xs_directory(gc, XBT_NULL, GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id), &nb) && nb) {
> +                /* Trigger device removal by forwarding state to XenbusStateClosing */
> +                if (v->remove)
> +                    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "5");
> +                continue;
> +            }
> +        }
> +        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id), v->p_devname);
> +        switch (v->pdev_type) {
> +            case LIBXL_VSCSI_PDEV_TYPE_WWN:
> +                flexarray_append_pair(back,
> +                                      GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
> +                                      v->p_devname);
> +                break;
> +            case LIBXL_VSCSI_PDEV_TYPE_DEV:
> +            case LIBXL_VSCSI_PDEV_TYPE_HCTL:
> +                flexarray_append_pair(back,
> +                                      GCSPRINTF("vscsi-devs/dev-%u/p-dev", v->vscsi_dev_id),
> +                                      GCSPRINTF("%u:%u:%u:%u", v->pdev.hst, v->pdev.chn, v->pdev.tgt, v->pdev.lun));
> +                break;
> +            case LIBXL_VSCSI_PDEV_TYPE_INVALID:
> +            default:
> +                rc = ERROR_FAIL;
> +                goto out;
> +        }
> +        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/v-dev", v->vscsi_dev_id),
> +                              GCSPRINTF("%u:%u:%u:%u", v->vdev.hst, v->vdev.chn, v->vdev.tgt, v->vdev.lun));
> +        flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id), "1");
> +    }
> +
> +    aodev->dev = device;
> +    /* Either create new host or reconfigure existing host */
> +    if (be_dirs == 0) {
> +        libxl__device_generic_add(gc, XBT_NULL, device,
> +                                  libxl__xs_kvs_of_flexarray(gc, back, back->count),
> +                                  libxl__xs_kvs_of_flexarray(gc, front, front->count),
> +                                  NULL);
> +        aodev->action = LIBXL__DEVICE_ACTION_ADD;
> +        libxl__wait_device_connection(egc, aodev);
> +        rc = 0;
> +        /* Done with new host */
> +        goto out;
> +    }
> +
> +    /* Only new devices, write them and do vscsi host reconfiguration */
> +    xs_transaction_t t;
> +retry_transaction:
> +    t = xs_transaction_start(ctx->xsh);
> +    libxl__xs_writev(gc, t, be_path,
> +                     libxl__xs_kvs_of_flexarray(gc, back, back->count));
> +    xs_write(ctx->xsh, t, GCSPRINTF("%s/state", be_path), "7", 2);
> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction;
> +        LOGE(ERROR, "xs transaction failed");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    libxl__wait_for_backend(gc, be_path, "4");
> +
> +retry_transaction2:
> +    t = xs_transaction_start(ctx->xsh);
> +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> +        if (v->remove) {
> +            char *path, *val;
> +            path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +            val = libxl__xs_read(gc, t, path);
> +            if (val && strcmp(val, "6") == 0) {
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +                path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
> +                xs_rm(ctx->xsh, t, path);
> +            } else {
> +                LOGE(ERROR, "%s: %s has %s, expected 6", __func__, path, val);
> +            }
> +        }
> +    }
> +
> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction2;
> +        LOGE(ERROR, "xs transaction failed");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    /* As we are not adding new device, skip waiting for it */
> +    libxl__ao_complete(egc, aodev->ao, 0);
> +
> +    rc = 0;
> +out:
> +    aodev->rc = rc;
> +    if(rc) aodev->callback(egc, aodev);
> +    return;
> +}

Re: wating for backend changes (was Re: [PATCH v3 4/4] libxl: add support for vscsi)

[Olaf Hering]

On Fri, Apr 10, Olaf Hering wrote:

> How is new code supposed to wait for backend changes?
> 
> Right now there are two APIs for that:
> - libxl__wait_for_backend loops for a while until it returns an error.
> - libxl__ev_devstate_wait registers a watch and a timer.
> 
> 
> In case of pvscsi there are three variants:
> - new, can use libxl__wait_device_connection
> - reconfigure, can use both of the above
> - remove, may use DEFINE_DEVICE_REMOVE and its
>   libxl__initiate_device_remove
> 
> The reconfigure case has to wait for various states, depending on the
> state before the reconfiguration.

For the time being I used also polling via libxl__wait_for_backend.
Doing event driven reconfigure can be implemented, but only with quite
some effort.

Olaf

wating for backend changes (was Re: [PATCH v3 4/4] libxl: add support for vscsi)

[Ian Jackson]

Olaf Hering writes ("wating for backend changes (was Re: [PATCH v3 4/4] libxl: add support for vscsi)"):
> How is new code supposed to wait for backend changes?
> 
> Right now there are two APIs for that:
> - libxl__wait_for_backend loops for a while until it returns an error.

This is deprecated.  I seem to remember I wrote a patch to clarify the
internal docs but it seems to be languishing in one of my outstanding
series.

> - libxl__ev_devstate_wait registers a watch and a timer.

New code should use this.

> In case of pvscsi there are three variants:
> - new, can use libxl__wait_device_connection
> - reconfigure, can use both of the above
> - remove, may use DEFINE_DEVICE_REMOVE and its
>   libxl__initiate_device_remove
...
> The pci code in libxl uses just libxl__wait_for_backend. Looking through
> the history it seems the function was added just for pci. Is it a
> deprecated function, should callers get converted to
> libxl__ev_devstate_wait?

Ideally, yes.

Thanks,
Ian.