Re: [RFC PATCH v1 04/15] nodedev: reset active config data on udev remove event

On 4/12/24 8:36 AM, Marc Hartmayer wrote: > From: Boris Fiuczynski <fiuczy(a)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(a)linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay(a)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.

Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;