[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]