Re: [RFC PATCH v1 03/15] nodedev: immediate update of active config on udev add

2024-04-19 Thread Marc Hartmayer
On Thu, Apr 18, 2024 at 09:47 AM -0500, Jonathon Jongsma  
wrote:
> On 4/12/24 8:36 AM, Marc Hartmayer wrote:
>> From: Boris Fiuczynski 
>> 
>> When an udev add event occurs the mdev active config data requires an
>> update via mdevctl as the udev does not contain all config data.
>> This update needs to occur immediate and to be finished before the
>
> s/immediate/immediately/

Will change.

[…snip…]

>>   
>> +/* The added mdev needs an immediate active config update before
>> + * the event is issued to allow sane API usage. */
>
> How about simply something like "so that full device information is 
> available at the time that the 'created' event is emitted"

Okay, thanks.

>
>> +if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) {
>> +VIR_WARN("Update of mediated device %s failed",
>> + NULLSTR_EMPTY(sysfs_path));
>> +}
>> + >   ret = 0;
>>   
>>cleanup:
>> @@ -1758,19 +1769,12 @@ 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")) {
>> -int ret = udevAddOneDevice(device);
>> -if (ret == 0 &&
>> -udevGetDeviceType(device, _cap_type) == 0 &&
>> -dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
>> -scheduleMdevctlUpdate(driver->privateData, false);
>> -return ret;
>> -}
>> +if (STREQ(action, "add") || STREQ(action, "change"))
>> +return udevAddOneDevice(device);
>>   
>>   if (STREQ(action, "remove"))
>>   return udevRemoveOneDevice(device);
>
> Reviewed-by: Jonathon Jongsma 

Thanks.

>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [RFC PATCH v1 03/15] nodedev: immediate update of active config on udev add

2024-04-18 Thread Jonathon Jongsma

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

From: Boris Fiuczynski 

When an udev add event occurs the mdev active config data requires an
update via mdevctl as the udev does not contain all config data.
This update needs to occur immediate and to be finished before the


s/immediate/immediately/


libvirt CREATE event is issued to keep the API usage reliable.

After this change, scheduleMdevctlUpdate call is already called in
`udevAddOneDevice` and can therefore be removed in `udevHandleOneDevice`.

Signed-off-by: Boris Fiuczynski 
Signed-off-by: Marc Hartmayer 
---
  src/node_device/node_device_udev.c | 22 +-
  1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 6613528d0e37..03ef0c14371a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1535,6 +1535,7 @@ udevSetParent(struct udev_device *device,
  static int
  udevAddOneDevice(struct udev_device *device)
  {
+g_autofree char *sysfs_path = NULL;
  virNodeDeviceDef *def = NULL;
  virNodeDeviceObj *obj = NULL;
  virNodeDeviceDef *objdef;
@@ -1549,6 +1550,9 @@ udevAddOneDevice(struct udev_device *device)
  def = g_new0(virNodeDeviceDef, 1);
  
  def->sysfs_path = g_strdup(udev_device_get_syspath(device));

+/* Create a copy of sysfs_path so it can be safely accessed, even without
+ * holding the @obj lock during the VIR_WARN(...) call at the end. */
+sysfs_path = g_strdup(def->sysfs_path);
  
  udevGetStringProperty(device, "DRIVER", >driver);
  
@@ -1608,6 +1612,13 @@ udevAddOneDevice(struct udev_device *device)

  if (has_mdev_types)
  scheduleMdevctlUpdate(driver->privateData, false);
  
+/* The added mdev needs an immediate active config update before

+ * the event is issued to allow sane API usage. */


How about simply something like "so that full device information is 
available at the time that the 'created' event is emitted"



+if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) {
+VIR_WARN("Update of mediated device %s failed",
+ NULLSTR_EMPTY(sysfs_path));
+}
+ >   ret = 0;
  
   cleanup:

@@ -1758,19 +1769,12 @@ 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")) {

-int ret = udevAddOneDevice(device);
-if (ret == 0 &&
-udevGetDeviceType(device, _cap_type) == 0 &&
-dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
-scheduleMdevctlUpdate(driver->privateData, false);
-return ret;
-}
+if (STREQ(action, "add") || STREQ(action, "change"))
+return udevAddOneDevice(device);
  
  if (STREQ(action, "remove"))

  return udevRemoveOneDevice(device);


Reviewed-by: Jonathon Jongsma 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org