From: Boris Fiuczynski <fiuczy@linux.ibm.com>
When a mdev device is destroyed or stopped the udev remove event
handling needs to reset the active config data of the node object
representing a persisted mdev.
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
src/node_device/node_device_driver.h | 3 +++
src/util/virmdev.h | 4 ++++
src/conf/node_device_conf.c | 10 ++--------
src/node_device/node_device_driver.c | 13 +++++++++++++
src/node_device/node_device_udev.c | 1 +
src/util/virmdev.c | 20 ++++++++++++++++++++
src/libvirt_private.syms | 2 ++
7 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index b3bc4b2e96ed..f195cfef9d49 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -197,3 +197,6 @@ int
nodeDeviceUpdate(virNodeDevice *dev,
const char *xmlDesc,
unsigned int flags);
+
+void
+nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def);
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 853041ac0619..e8e69040e5d4 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig {
size_t nattributes;
};
+void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config);
+void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree);
+
typedef struct _virMediatedDeviceType virMediatedDeviceType;
struct _virMediatedDeviceType {
char *id;
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5cfbd6a7eb72..897c67d6af91 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
g_free(data->sg.path);
break;
case VIR_NODE_DEV_CAP_MDEV:
- g_free(data->mdev.defined_config.type);
- g_free(data->mdev.active_config.type);
g_free(data->mdev.uuid);
- for (i = 0; i < data->mdev.defined_config.nattributes; i++)
- virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
- g_free(data->mdev.defined_config.attributes);
- for (i = 0; i < data->mdev.active_config.nattributes; i++)
- virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
- g_free(data->mdev.active_config.attributes);
+ virMediatedDeviceConfigClear(&data->mdev.defined_config);
+ virMediatedDeviceConfigClear(&data->mdev.active_config);
g_free(data->mdev.parent_addr);
break;
case VIR_NODE_DEV_CAP_CSS_DEV:
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index d99b48138ebf..f623339dc973 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
}
+/* A mediated device definition contains data from mdevctl about the active
+ * device. When the device is deactivated the active configuration data needs
+ * to be removed. */
+void
+nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def)
+{
+ if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV)
+ return;
+
+ virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config);
+}
+
+
int
nodeDeviceSetAutostart(virNodeDevice *device,
int autostart)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 03ef0c14371a..3297bdd8d7ef 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1467,6 +1467,7 @@ udevRemoveOneDeviceSysPath(const char *path)
if (virNodeDeviceObjIsPersistent(obj)) {
VIR_FREE(def->sysfs_path);
virNodeDeviceObjSetActive(obj, false);
+ nodeDeviceDefResetMdevActiveConfig(def);
} else {
VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
def->name, path);
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 992f3eb1b74c..6ecdbdf0ab77 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -516,6 +516,26 @@ void virMediatedDeviceAttrFree(virMediatedDeviceAttr *attr)
g_free(attr);
}
+void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config)
+{
+ if (!config)
+ return;
+
+ virMediatedDeviceConfigClear(config);
+ g_free(config);
+}
+
+void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config)
+{
+ size_t i = 0;
+
+ g_clear_pointer(&config->type, g_free);
+ for (i = 0; i < config->nattributes; i++)
+ virMediatedDeviceAttrFree(config->attributes[i]);
+ config->nattributes = 0;
+ g_clear_pointer(&config->attributes, g_free);
+}
+
#define MDEV_BUS_DIR "/sys/class/mdev_bus"
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 84e30b711c67..fd413949dae2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2778,6 +2778,8 @@ virMacMapWriteFile;
# util/virmdev.h
virMediatedDeviceAttrFree;
virMediatedDeviceAttrNew;
+virMediatedDeviceConfigClear;
+virMediatedDeviceConfigFree;
virMediatedDeviceFree;
virMediatedDeviceGetIOMMUGroupDev;
virMediatedDeviceGetIOMMUGroupNum;
--
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 4/12/24 8:36 AM, Marc Hartmayer wrote:
> From: Boris Fiuczynski <fiuczy@linux.ibm.com>
>
> When a mdev device is destroyed or stopped the udev remove event
> handling needs to reset the active config data of the node object
> representing a persisted mdev.
>
> Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
> src/node_device/node_device_driver.h | 3 +++
> src/util/virmdev.h | 4 ++++
> src/conf/node_device_conf.c | 10 ++--------
> src/node_device/node_device_driver.c | 13 +++++++++++++
> src/node_device/node_device_udev.c | 1 +
> src/util/virmdev.c | 20 ++++++++++++++++++++
> src/libvirt_private.syms | 2 ++
> 7 files changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> index b3bc4b2e96ed..f195cfef9d49 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -197,3 +197,6 @@ int
> nodeDeviceUpdate(virNodeDevice *dev,
> const char *xmlDesc,
> unsigned int flags);
> +
> +void
> +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def);
> diff --git a/src/util/virmdev.h b/src/util/virmdev.h
> index 853041ac0619..e8e69040e5d4 100644
> --- a/src/util/virmdev.h
> +++ b/src/util/virmdev.h
> @@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig {
> size_t nattributes;
> };
>
> +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config);
> +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree);
As far as I can tell, this Free function is not actually used anywhere,
either in this patch or any following patches.
> +
> typedef struct _virMediatedDeviceType virMediatedDeviceType;
> struct _virMediatedDeviceType {
> char *id;
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 5cfbd6a7eb72..897c67d6af91 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
> g_free(data->sg.path);
> break;
> case VIR_NODE_DEV_CAP_MDEV:
> - g_free(data->mdev.defined_config.type);
> - g_free(data->mdev.active_config.type);
> g_free(data->mdev.uuid);
> - for (i = 0; i < data->mdev.defined_config.nattributes; i++)
> - virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
> - g_free(data->mdev.defined_config.attributes);
> - for (i = 0; i < data->mdev.active_config.nattributes; i++)
> - virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
> - g_free(data->mdev.active_config.attributes);
> + virMediatedDeviceConfigClear(&data->mdev.defined_config);
> + virMediatedDeviceConfigClear(&data->mdev.active_config);
> g_free(data->mdev.parent_addr);
> break;
> case VIR_NODE_DEV_CAP_CSS_DEV:
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index d99b48138ebf..f623339dc973 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
> }
>
>
> +/* A mediated device definition contains data from mdevctl about the active
> + * device. When the device is deactivated the active configuration data needs
> + * to be removed. */
> +void
> +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def)
> +{
> + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV)
> + return;
> +
> + virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config);
> +}
It doesn't feel necessary to introduce a short single-use function here.
I think it can just be inlined.
> +
> +
> int
> nodeDeviceSetAutostart(virNodeDevice *device,
> int autostart)
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 03ef0c14371a..3297bdd8d7ef 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1467,6 +1467,7 @@ udevRemoveOneDeviceSysPath(const char *path)
> if (virNodeDeviceObjIsPersistent(obj)) {
> VIR_FREE(def->sysfs_path);
> virNodeDeviceObjSetActive(obj, false);
> + nodeDeviceDefResetMdevActiveConfig(def);
> } else {
> VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
> def->name, path);
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 992f3eb1b74c..6ecdbdf0ab77 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -516,6 +516,26 @@ void virMediatedDeviceAttrFree(virMediatedDeviceAttr *attr)
> g_free(attr);
> }
>
> +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config)
> +{
> + if (!config)
> + return;
> +
> + virMediatedDeviceConfigClear(config);
> + g_free(config);
> +}
> +
> +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config)
> +{
> + size_t i = 0;
> +
> + g_clear_pointer(&config->type, g_free);
> + for (i = 0; i < config->nattributes; i++)
> + virMediatedDeviceAttrFree(config->attributes[i]);
> + config->nattributes = 0;
> + g_clear_pointer(&config->attributes, g_free);
> +}
> +
>
> #define MDEV_BUS_DIR "/sys/class/mdev_bus"
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 84e30b711c67..fd413949dae2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2778,6 +2778,8 @@ virMacMapWriteFile;
> # util/virmdev.h
> virMediatedDeviceAttrFree;
> virMediatedDeviceAttrNew;
> +virMediatedDeviceConfigClear;
> +virMediatedDeviceConfigFree;
> virMediatedDeviceFree;
> virMediatedDeviceGetIOMMUGroupDev;
> virMediatedDeviceGetIOMMUGroupNum;
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Apr 18, 2024 at 10:03 AM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
> On 4/12/24 8:36 AM, Marc Hartmayer wrote:
>> From: Boris Fiuczynski <fiuczy@linux.ibm.com>
>>
>> When a mdev device is destroyed or stopped the udev remove event
>> handling needs to reset the active config data of the node object
>> representing a persisted mdev.
>>
>> Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>> src/node_device/node_device_driver.h | 3 +++
>> src/util/virmdev.h | 4 ++++
>> src/conf/node_device_conf.c | 10 ++--------
>> src/node_device/node_device_driver.c | 13 +++++++++++++
>> src/node_device/node_device_udev.c | 1 +
>> src/util/virmdev.c | 20 ++++++++++++++++++++
>> src/libvirt_private.syms | 2 ++
>> 7 files changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
>> index b3bc4b2e96ed..f195cfef9d49 100644
>> --- a/src/node_device/node_device_driver.h
>> +++ b/src/node_device/node_device_driver.h
>> @@ -197,3 +197,6 @@ int
>> nodeDeviceUpdate(virNodeDevice *dev,
>> const char *xmlDesc,
>> unsigned int flags);
>> +
>> +void
>> +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def);
>> diff --git a/src/util/virmdev.h b/src/util/virmdev.h
>> index 853041ac0619..e8e69040e5d4 100644
>> --- a/src/util/virmdev.h
>> +++ b/src/util/virmdev.h
>> @@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig {
>> size_t nattributes;
>> };
>>
>> +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config);
>> +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config);
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree);
>
> As far as I can tell, this Free function is not actually used anywhere,
> either in this patch or any following patches.
Yep, was just for completeness. Shall I remove it? Maybe it will be used
in the future and if we have a …Clear function, I guess it’s always good
to have a …Free function as well.
>
>> +
>> typedef struct _virMediatedDeviceType virMediatedDeviceType;
>> struct _virMediatedDeviceType {
>> char *id;
>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>> index 5cfbd6a7eb72..897c67d6af91 100644
>> --- a/src/conf/node_device_conf.c
>> +++ b/src/conf/node_device_conf.c
>> @@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
>> g_free(data->sg.path);
>> break;
>> case VIR_NODE_DEV_CAP_MDEV:
>> - g_free(data->mdev.defined_config.type);
>> - g_free(data->mdev.active_config.type);
>> g_free(data->mdev.uuid);
>> - for (i = 0; i < data->mdev.defined_config.nattributes; i++)
>> - virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
>> - g_free(data->mdev.defined_config.attributes);
>> - for (i = 0; i < data->mdev.active_config.nattributes; i++)
>> - virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
>> - g_free(data->mdev.active_config.attributes);
>> + virMediatedDeviceConfigClear(&data->mdev.defined_config);
>> + virMediatedDeviceConfigClear(&data->mdev.active_config);
>> g_free(data->mdev.parent_addr);
>> break;
>> case VIR_NODE_DEV_CAP_CSS_DEV:
>> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
>> index d99b48138ebf..f623339dc973 100644
>> --- a/src/node_device/node_device_driver.c
>> +++ b/src/node_device/node_device_driver.c
>> @@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>> }
>>
>>
>> +/* A mediated device definition contains data from mdevctl about the active
>> + * device. When the device is deactivated the active configuration data needs
>> + * to be removed. */
>> +void
>> +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def)
>> +{
>> + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV)
>> + return;
>> +
>> + virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config);
>> +}
>
> It doesn't feel necessary to introduce a short single-use function here.
> I think it can just be inlined.
Don’t have a strong opinion about this, @Boris?
[…snip…]
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
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-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.