[libvirt] [PATCH 3/7] node_device: detect CCW devices

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> Make CCW devices available to the node_device driver. The devices are
> already seen by udev so let's implement necessary code for detecting
> them properly.
> 
> Topologically, CCW devices are similar to PCI devices, e.g.:
> 
>     +- ccw_0_0_1a2b
>         |
>         +- scsi_host0
>             |
>             +- scsi_target0_0_0
>                 |
>                 +- scsi_0_0_0_0
> 
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Signed-off-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> ---
>  docs/schemas/basictypes.rng                       | 31 ++++++++++
>  docs/schemas/domaincommon.rng                     | 30 ---------
>  docs/schemas/nodedev.rng                          | 16 +++++
>  src/conf/node_device_conf.c                       | 75 ++++++++++++++++++++++-
>  src/conf/node_device_conf.h                       | 10 +++
>  src/node_device/node_device_driver.c              |  1 +
>  src/node_device/node_device_udev.c                | 35 ++++++++++-
>  tests/nodedevschemadata/ccw_0_0_10000-invalid.xml | 10 +++
>  tests/nodedevschemadata/ccw_0_0_ffff.xml          | 10 +++
>  tests/nodedevxml2xmltest.c                        |  1 +
>  tools/virsh-nodedev.c                             |  2 +
>  11 files changed, 188 insertions(+), 33 deletions(-)
>  create mode 100644 tests/nodedevschemadata/ccw_0_0_10000-invalid.xml
>  create mode 100644 tests/nodedevschemadata/ccw_0_0_ffff.xml
> 

...

> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 9cb63860f..28d4907f5 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c

...

>  
>  static int
> +virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
> +                         virNodeDeviceDefPtr def,
> +                         xmlNodePtr node,
> +                         virNodeDevCapCCWPtr ccw_dev)
> +{
> +    xmlNodePtr orignode;
> +    int ret = -1;
> +    char *cssid = NULL, *ssid = NULL, *devno = NULL;
> +
> +    orignode = ctxt->node;
> +    ctxt->node = node;
> +
> +   if (!(cssid = virXPathString("string(./cssid[1])", ctxt))) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("missing cssid value for '%s'"), def->name);
> +        goto out;
> +    }
> +
> +    if (virStrToLong_uip(cssid, NULL, 0, &ccw_dev->cssid) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("invalid cssid value '%s' for '%s'"),
> +                       cssid, def->name);
> +        goto out;
> +    }
> +
> +    if (!(ssid = virXPathString("string(./ssid[1])", ctxt))) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("missing ssid value for '%s'"), def->name);
> +        goto out;
> +    }
> +
> +    if (virStrToLong_uip(ssid, NULL, 0, &ccw_dev->ssid) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("invalid ssid value '%s' for '%s'"),
> +                       cssid, def->name);
> +        goto out;
> +    }
> +
> +    if (!(devno = virXPathString("string(./devno[1])", ctxt))) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("missing devno value for '%s'"), def->name);
> +        goto out;
> +    }
> +
> +    if (virStrToLong_uip(devno, NULL, 16, &ccw_dev->devno) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("invalid devno value '%s' for '%s'"),
> +                       devno, def->name);
> +        goto out;
> +    }

One would hope they're in range, but since the rng had ranges should you
check here similar to what virDomainDeviceCCWAddressIsValid does?

It's fine this way since this is essentially reporting from udev which
one can only assume (haha) would do validation...

> +
> +    ret = 0;
> +
> + out:
> +    ctxt->node = orignode;
> +    return ret;
> +}
> +
> +

...

> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 4ecb0b18f..7744c2637 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1105,6 +1105,33 @@ udevProcessMediatedDevice(struct udev_device *dev,
>  }
>  
>  static int
> +udevProcessCCW(struct udev_device *device, virNodeDeviceDefPtr def)

Although you're following the module syntax, this should follow current
practices for multilines and spacing before/after function... I can
adjust that before pushing if this is all that's necessary though.

> +{
> +    int online;
> +    char *p;
> +    virNodeDevCapDataPtr data = &def->caps->data;
> +
> +    /* process only online devices to keep the list sane */
> +    if (udevGetIntSysfsAttr(device, "online", &online, 0) < 0 || online != 1)
> +        return -1;
> +
> +    if ((p = strrchr(def->sysfs_path, '/')) == NULL ||
> +        virStrToLong_ui(p + 1, &p, 16, &data->ccw_dev.cssid) < 0 || p == NULL ||
> +        virStrToLong_ui(p + 1, &p, 16, &data->ccw_dev.ssid) < 0 || p == NULL ||
> +        virStrToLong_ui(p + 1, &p, 16, &data->ccw_dev.devno) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("failed to parse the CCW address from sysfs path: '%s'"),
> +                       def->sysfs_path);
> +        return -1;
> +    }
> +
> +    if (udevGenerateDeviceName(device, def, NULL) != 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +static int

...

> diff --git a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml
> new file mode 100644
> index 000000000..d840555c0
> --- /dev/null
> +++ b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml
> @@ -0,0 +1,10 @@
> +<device>
> +  <name>ccw_0_0_10000</name>
> +  <path>/sys/devices/css0/0.0.0000/0.0.10000</path>
> +  <parent>computer</parent>
> +  <capability type='ccw'>
> +    <cssid>0x0</cssid>
> +    <ssid>0x0</ssid>
> +    <devno>0x10000</devno>
> +  </capability>
> +</device>

I assume you planned to use this, but either forgot or didn't want to
write the EXPECT_FAIL test?

Should it be removed from the patch?

Reviewed-by: John Ferlan <jferlan at redhat.com>

FWIW: I can make adjustments if you'd like or you can provide a v2 of
this patch. Just let me know


John

...