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

[Bjoern Walk]

John Ferlan <jferlan at 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 at 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 at 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 at 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 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 896 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170526/196be9c1/attachment-0001.sig>