Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device

On Fri, Nov 13, 2020 at 03:45:33PM +0100, Shalini Chellathurai Saroja wrote: > On 11/12/20 9:29 PM, Jonathon Jongsma wrote: >>> diff --git a/src/node_device/node_device_udev.c >>> b/src/node_device/node_device_udev.c index 6bbff571..5f57000e 100644 >>> --- a/src/node_device/node_device_udev.c >>> +++ b/src/node_device/node_device_udev.c >>> @@ -1241,6 +1241,25 @@ udevProcessAPQueue(struct udev_device *device, >>> } >>> +static int >>> +udevProcessAPMatrix(struct udev_device *device, >>> + virNodeDeviceDefPtr def) >>> +{ >>> + size_t i; >>> + virNodeDevCapDataPtr data = &def->caps->data; >>> + >>> + data->ap_matrix.addr = >>> g_strdup(udev_device_get_sysname(device)); >>> + def->name = g_strdup_printf("ap_%s", data->ap_matrix.addr); >> So you're setting this 'addr' field, but it is not used anywhere else in >> this patch. Perhaps you'll use it in upcoming patches. But it is not >> formatted into the node device xml or anything like that. Is that >> intentional? >> > Yes, it is not formatted into the node device xml. > > I will include this change in the patch 10. > >>> + >>> + for (i = 0; i < strlen(def->name); i++) { >>> + if (!(g_ascii_isalnum(*(def->name + i)))) >>> + *(def->name + i) = '_'; >>> + } >>> + >>> + return 0; >>> +} >> Out of curiosity, what's the reason that you're >> hard-coding an "ap_" prefix to the nodedev name rather than just using >> udevGenerateDeviceName() like all of the other device types? > The name generated with udevGenerateDeviceName() is matrix_matrix (as both > udev_device_get_subsystem and udev_device_get_sysname returns matrix), which > is not very helpful, so we decided to go with ap_matrix instead. But if that is the case, I'm wondering why are you trying to generate it dynamically in the first place, why not hardcoding it to "ap_matrix"? I must be missing something here. Regards, Erik