On Thu, Apr 18, 2024 at 10:03 AM -0500, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
> On 4/12/24 8:36 AM, Marc Hartmayer wrote:
>> From: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
