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@linux.vnet.ibm.com>
Signed-off-by: Bjoern Walk <bwalk@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/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index cc560e66f..1ea667cdf 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -318,6 +318,37 @@
</data>
</define>
+ <define name="ccwCssidRange">
+ <choice>
+ <data type="string">
+ <param name="pattern">0x[0-9a-eA-E][0-9a-fA-F]?</param>
+ </data>
+ <data type="string">
+ <param name="pattern">0x[fF][0-9a-eA-E]?</param>
+ </data>
+ <data type="int">
+ <param name="minInclusive">0</param>
+ <param name="maxInclusive">254</param>
+ </data>
+ </choice>
+ </define>
+ <define name="ccwSsidRange">
+ <data type="string">
+ <param name="pattern">(0x)?[0-3]</param>
+ </data>
+ </define>
+ <define name="ccwDevnoRange">
+ <choice>
+ <data type="string">
+ <param name="pattern">0x[0-9a-fA-F]{1,4}</param>
+ </data>
+ <data type="int">
+ <param name="minInclusive">0</param>
+ <param name="maxInclusive">65535</param>
+ </data>
+ </choice>
+ </define>
+
<define name="cpuset">
<data type="string">
<param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f88e84ab6..94de10749 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5770,36 +5770,6 @@
</element>
<empty/>
</define>
- <define name="ccwCssidRange">
- <choice>
- <data type="string">
- <param name="pattern">0x[0-9a-eA-E][0-9a-fA-F]?</param>
- </data>
- <data type="string">
- <param name="pattern">0x[fF][0-9a-eA-E]?</param>
- </data>
- <data type="int">
- <param name="minInclusive">0</param>
- <param name="maxInclusive">254</param>
- </data>
- </choice>
- </define>
- <define name="ccwSsidRange">
- <data type="string">
- <param name="pattern">(0x)?[0-3]</param>
- </data>
- </define>
- <define name="ccwDevnoRange">
- <choice>
- <data type="string">
- <param name="pattern">0x[0-9a-fA-F]{1,4}</param>
- </data>
- <data type="int">
- <param name="minInclusive">0</param>
- <param name="maxInclusive">65535</param>
- </data>
- </choice>
- </define>
<define name="panic">
<element name="panic">
<optional>
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 924f73861..87bfb0c28 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -84,6 +84,7 @@
<ref name="capstorage"/>
<ref name="capdrm"/>
<ref name="capmdev"/>
+ <ref name="capccwdev"/>
</choice>
</element>
</define>
@@ -597,6 +598,21 @@
</element>
</define>
+ <define name='capccwdev'>
+ <attribute name='type'>
+ <value>ccw</value>
+ </attribute>
+ <element name='cssid'>
+ <ref name='ccwCssidRange'/>
+ </element>
+ <element name='ssid'>
+ <ref name='ccwSsidRange'/>
+ </element>
+ <element name='devno'>
+ <ref name='ccwDevnoRange'/>
+ </element>
+ </define>
+
<define name='address'>
<element name='address'>
<attribute name='domain'><ref name='hexuint'/></attribute>
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
@@ -62,7 +62,8 @@ VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST,
"scsi_generic",
"drm",
"mdev_types",
- "mdev")
+ "mdev",
+ "ccw")
VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST,
"80203",
@@ -581,6 +582,14 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n",
data->mdev.iommuGroupNumber);
break;
+ case VIR_NODE_DEV_CAP_CCW_DEV:
+ virBufferAsprintf(&buf, "<cssid>0x%x</cssid>\n",
+ data->ccw_dev.cssid);
+ virBufferAsprintf(&buf, "<ssid>0x%x</ssid>\n",
+ data->ccw_dev.ssid);
+ virBufferAsprintf(&buf, "<devno>0x%04x</devno>\n",
+ data->ccw_dev.devno);
+ break;
case VIR_NODE_DEV_CAP_MDEV_TYPES:
case VIR_NODE_DEV_CAP_FC_HOST:
case VIR_NODE_DEV_CAP_VPORTS:
@@ -718,6 +727,66 @@ virNodeDevCapDRMParseXML(xmlXPathContextPtr ctxt,
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;
+ }
+
+ ret = 0;
+
+ out:
+ ctxt->node = orignode;
+ return ret;
+}
+
+
+static int
virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
virNodeDeviceDefPtr def,
xmlNodePtr node,
@@ -1754,6 +1823,9 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt,
case VIR_NODE_DEV_CAP_MDEV:
ret = virNodeDevCapMdevParseXML(ctxt, def, node, &caps->data.mdev);
break;
+ case VIR_NODE_DEV_CAP_CCW_DEV:
+ ret = virNodeDevCapCCWParseXML(ctxt, def, node, &caps->data.ccw_dev);
+ break;
case VIR_NODE_DEV_CAP_MDEV_TYPES:
case VIR_NODE_DEV_CAP_FC_HOST:
case VIR_NODE_DEV_CAP_VPORTS:
@@ -2083,6 +2155,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
case VIR_NODE_DEV_CAP_DRM:
case VIR_NODE_DEV_CAP_FC_HOST:
case VIR_NODE_DEV_CAP_VPORTS:
+ case VIR_NODE_DEV_CAP_CCW_DEV:
case VIR_NODE_DEV_CAP_LAST:
/* This case is here to shutup the compiler */
break;
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 5743f9d3e..bf9d5fce5 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -66,6 +66,7 @@ typedef enum {
VIR_NODE_DEV_CAP_DRM, /* DRM device */
VIR_NODE_DEV_CAP_MDEV_TYPES, /* Device capable of mediated devices */
VIR_NODE_DEV_CAP_MDEV, /* Mediated device */
+ VIR_NODE_DEV_CAP_CCW_DEV, /* s390 CCW device */
VIR_NODE_DEV_CAP_LAST
} virNodeDevCapType;
@@ -267,6 +268,14 @@ struct _virNodeDevCapDRM {
virNodeDevDRMType type;
};
+typedef struct _virNodeDevCapCCW virNodeDevCapCCW;
+typedef virNodeDevCapCCW *virNodeDevCapCCWPtr;
+struct _virNodeDevCapCCW {
+ unsigned int cssid;
+ unsigned int ssid;
+ unsigned int devno;
+};
+
typedef struct _virNodeDevCapData virNodeDevCapData;
typedef virNodeDevCapData *virNodeDevCapDataPtr;
struct _virNodeDevCapData {
@@ -284,6 +293,7 @@ struct _virNodeDevCapData {
virNodeDevCapSCSIGeneric sg;
virNodeDevCapDRM drm;
virNodeDevCapMdev mdev;
+ virNodeDevCapCCW ccw_dev;
};
};
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 74507c214..286af26bc 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -84,6 +84,7 @@ static int update_caps(virNodeDeviceObjPtr dev)
case VIR_NODE_DEV_CAP_SCSI_GENERIC:
case VIR_NODE_DEV_CAP_MDEV_TYPES:
case VIR_NODE_DEV_CAP_MDEV:
+ case VIR_NODE_DEV_CAP_CCW_DEV:
case VIR_NODE_DEV_CAP_LAST:
break;
}
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)
+{
+ 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
udevGetDeviceNodes(struct udev_device *device,
virNodeDeviceDefPtr def)
{
@@ -1172,8 +1199,8 @@ udevGetDeviceType(struct udev_device *device,
if (udevHasDeviceProperty(device, "INTERFACE"))
*type = VIR_NODE_DEV_CAP_NET;
- /* Neither SCSI generic devices nor mediated devices set DEVTYPE
- * property, therefore we need to rely on the SUBSYSTEM property */
+ /* The following devices do not set the DEVTYPE property, therefore
+ * we need to rely on the SUBSYSTEM property */
if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) < 0)
return -1;
@@ -1181,6 +1208,8 @@ udevGetDeviceType(struct udev_device *device,
*type = VIR_NODE_DEV_CAP_SCSI_GENERIC;
else if (STREQ_NULLABLE(subsystem, "mdev"))
*type = VIR_NODE_DEV_CAP_MDEV;
+ else if (STREQ_NULLABLE(subsystem, "ccw"))
+ *type = VIR_NODE_DEV_CAP_CCW_DEV;
VIR_FREE(subsystem);
}
@@ -1222,6 +1251,8 @@ static int udevGetDeviceDetails(struct udev_device *device,
return udevProcessDRMDevice(device, def);
case VIR_NODE_DEV_CAP_MDEV:
return udevProcessMediatedDevice(device, def);
+ case VIR_NODE_DEV_CAP_CCW_DEV:
+ return udevProcessCCW(device, def);
case VIR_NODE_DEV_CAP_MDEV_TYPES:
case VIR_NODE_DEV_CAP_SYSTEM:
case VIR_NODE_DEV_CAP_FC_HOST:
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>
diff --git a/tests/nodedevschemadata/ccw_0_0_ffff.xml b/tests/nodedevschemadata/ccw_0_0_ffff.xml
new file mode 100644
index 000000000..5ecd0b0aa
--- /dev/null
+++ b/tests/nodedevschemadata/ccw_0_0_ffff.xml
@@ -0,0 +1,10 @@
+<device>
+ <name>ccw_0_0_ffff</name>
+ <path>/sys/devices/css0/0.0.0000/0.0.ffff</path>
+ <parent>computer</parent>
+ <capability type='ccw'>
+ <cssid>0x0</cssid>
+ <ssid>0x0</ssid>
+ <devno>0xffff</devno>
+ </capability>
+</device>
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index a2aad518d..805deef26 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -103,6 +103,7 @@ mymain(void)
DO_TEST("drm_renderD129");
DO_TEST("pci_0000_02_10_7_mdev_types");
DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
+ DO_TEST("ccw_0_0_ffff");
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index ad96dda1f..1822d3dce 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -460,6 +460,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
case VIR_NODE_DEV_CAP_MDEV:
flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV;
break;
+ case VIR_NODE_DEV_CAP_CCW_DEV:
+ /* enable next patch */
case VIR_NODE_DEV_CAP_LAST:
break;
}
--
2.11.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@linux.vnet.ibm.com>
> Signed-off-by: Bjoern Walk <bwalk@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@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
...
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
John Ferlan <jferlan@redhat.com> [2017-05-25, 03:05PM -0400]:
>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...
>
True, yes, for validation we only rely on the rng. I actually wanted to
avoid any complicated error checking here for much easier code.
>> +
>> + 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.
Ok, will remember next time.
>
>> +{
>> + 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?
Since we don't perform any validation in the code, a test would never
actually fail. But this XML is implicitly tested by virschematest so I
thought at least this is covered.
>
>Should it be removed from the patch?
Depends on if we actually want the validation and/or if the test against
the RNG schema is enough.
>
>Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks anyways.
>
>FWIW: I can make adjustments if you'd like or you can provide a v2 of
>this patch. Just let me know
>
>
>John
>
>...
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
>
--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
... >> >> ... >> >>> 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? > > Since we don't perform any validation in the code, a test would never > actually fail. But this XML is implicitly tested by virschematest so I > thought at least this is covered. > >> >> Should it be removed from the patch? > > Depends on if we actually want the validation and/or if the test against > the RNG schema is enough. > oh right - for some reason I had xml2xml type checking - I'll leave it though. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.