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

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(a)redhat.com&gt;

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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list