John Ferlan <jferlan@xxxxxxxxxx> [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@xxxxxxxxxx>
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@xxxxxxxxxx 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@xxxxxxxxxx ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: BöblingenRegistergericht: Amtsgericht Stuttgart, HRB 243294
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
