Re: [PATCH v2] nodedev: transient mdev update on nodeDeviceCreateXML

On 7/6/23 9:40 PM, Jonathon Jongsma wrote: > On 6/30/23 6:34 AM, Boris Fiuczynski wrote: >> Update the optional mdev attributes by running an mdevctl update on a >> new created nodedev object representing an mdev. >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158 >> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; >> --- >>   src/node_device/node_device_udev.c | 12 ++++++++++-- >>   1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/src/node_device/node_device_udev.c >> b/src/node_device/node_device_udev.c >> index 4c37ec3189..fce4212728 100644 >> --- a/src/node_device/node_device_udev.c >> +++ b/src/node_device/node_device_udev.c >> @@ -1757,12 +1757,20 @@ nodeStateCleanup(void) >>   static int >>   udevHandleOneDevice(struct udev_device *device) >>   { >> +    virNodeDevCapType dev_cap_type; >>       const char *action = udev_device_get_action(device); >>       VIR_DEBUG("udev action: '%s': %s", action, >> udev_device_get_syspath(device)); >> -    if (STREQ(action, "add") || STREQ(action, "change")) >> -        return udevAddOneDevice(device); >> +    if (STREQ(action, "add") || STREQ(action, "change")) { >> +        int ret = udevAddOneDevice(device); >> +        if (ret == 0 && >> +            udevGetDeviceType(device, &dev_cap_type) == 0 && >> +            dev_cap_type == VIR_NODE_DEV_CAP_MDEV) >> +            if (nodeDeviceUpdateMediatedDevices() < 0) >> +                VIR_WARN("mdevctl failed to update mediated devices"); >> +        return ret; >> +    } >>       if (STREQ(action, "remove")) >>           return udevRemoveOneDevice(device); > > > So, this should work and roughly matches what we've done for other > similar cases. But this is running from within the udev event handler > thread, and theoretically there could already be an mdevctl thread > running concurrently and updating the internal device list. The device > list update functions are thread-safe so I don't think it'll corrupt > anything, but it seems better to do all mdevctl updates from  mdevctl > update thread rather than calling it directly here. The same actually > applies to a couple other existing calls to > nodeDeviceUpdateMediatedDevices(). I'll send a couple patches that > apply on top of yours and refactor things a bit. Let me know what you > think. > > Jonathon > LGTM and testing was successful. See my direct replies on the patches.