9:59 a.m.
*** BLURB HERE ***
Michal Prívozník (4):
virpci: Decrease scope of VIR_PF_PHYS_PORT_NAME_REGEX macro
qemu_command: Don't open code virPCIDeviceAddressAsString()
vircpi: Decrease scope of VIR_PCI_DEVICE_ADDRESS_FMT macro
ch: Don't leak ch_driver->chCaps
src/ch/ch_driver.c | 5 +++--
src/qemu/qemu_command.c | 6 +-----
src/util/virpci.c | 9 +++++++++
src/util/virpci.h | 7 -------
4 files changed, 13 insertions(+), 14 deletions(-)
--
2.41.0
9:59 a.m.
New subject: [PATCH 1/4] virpci: Decrease scope of VIR_PF_PHYS_PORT_NAME_REGEX macro
The VIR_PF_PHYS_PORT_NAME_REGEX macro is used only in
virPCIGetNetName() and nowhere else. It's not necessary to expose
it in the header file.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virpci.c | 8 ++++++++
src/util/virpci.h | 5 -----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index baacde4c14..eae698c0a0 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2536,6 +2536,12 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddress *addr,
return 0;
}
+
+/* Represents format of PF's phys_port_name in switchdev mode:
+ * 'p%u' or 'p%us%u'. New line checked since value is read from sysfs
file.
+ */
+# define VIR_PF_PHYS_PORT_NAME_REGEX "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
+
/**
* virPCIGetNetName:
* @device_link_sysfs_path: sysfs path to the PCI device
@@ -2661,6 +2667,8 @@ virPCIGetNetName(const char *device_link_sysfs_path,
return -1;
}
+# undef VIR_PF_PHYS_PORT_NAME_REGEX
+
int
virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
int pfNetDevIdx,
diff --git a/src/util/virpci.h b/src/util/virpci.h
index faca6cf6f9..e964a2685c 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -52,11 +52,6 @@ struct _virZPCIDeviceAddress {
#define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
-/* Represents format of PF's phys_port_name in switchdev mode:
- * 'p%u' or 'p%us%u'. New line checked since value is read from sysfs
file.
- */
-#define VIR_PF_PHYS_PORT_NAME_REGEX "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
-
struct _virPCIDeviceAddress {
unsigned int domain;
unsigned int bus;
--
2.41.0
9:59 a.m.
New subject: [PATCH 2/4] qemu_command: Don't open code virPCIDeviceAddressAsString()
When building a hostdev props, its PCI address is formatted via
g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, ...); Well, we have a
function that does exactly that: virPCIDeviceAddressAsString().
Us the latter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d54149ed2d..23909dbbab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4704,11 +4704,7 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
g_autoptr(virJSONValue) props = NULL;
virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci;
virDomainNetTeamingInfo *teaming;
- g_autofree char *host = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT,
- pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
+ g_autofree char *host = virPCIDeviceAddressAsString(&pcisrc->addr);
const char *failover_pair_id = NULL;
/* caller has to assign proper passthrough backend type */
--
2.41.0
10:09 a.m.
New subject: [PATCH 2/4] qemu_command: Don't open code
virPCIDeviceAddressAsString()
On 11/28/23 9:59 AM, Michal Privoznik wrote:
When building a hostdev props, its PCI address is formatted via
g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, ...); Well, we have a
function that does exactly that: virPCIDeviceAddressAsString().
Us the latter.
s/Us/Use/
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d54149ed2d..23909dbbab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4704,11 +4704,7 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
g_autoptr(virJSONValue) props = NULL;
virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci;
virDomainNetTeamingInfo *teaming;
- g_autofree char *host = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT,
- pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
+ g_autofree char *host = virPCIDeviceAddressAsString(&pcisrc->addr);
const char *failover_pair_id = NULL;
/* caller has to assign proper passthrough backend type */
9:59 a.m.
New subject: [PATCH 3/4] vircpi: Decrease scope of VIR_PCI_DEVICE_ADDRESS_FMT macro
The VIR_PCI_DEVICE_ADDRESS_FMT macro is used only in virpci.c and
nowhere else. It's not necessary to expose it in the header file.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virpci.c | 1 +
src/util/virpci.h | 2 --
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index eae698c0a0..afce7b52b7 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -42,6 +42,7 @@ VIR_LOG_INIT("util.pci");
#define PCI_SYSFS "/sys/bus/pci/"
#define PCI_ID_LEN 10 /* "XXXX XXXX" */
+#define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
VIR_ENUM_IMPL(virPCIELinkSpeed,
VIR_PCIE_LINK_SPEED_LAST,
diff --git a/src/util/virpci.h b/src/util/virpci.h
index e964a2685c..bc7cb2329f 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -50,8 +50,6 @@ struct _virZPCIDeviceAddress {
/* Don't forget to update virPCIDeviceAddressCopy if needed. */
};
-#define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
-
struct _virPCIDeviceAddress {
unsigned int domain;
unsigned int bus;
--
2.41.0
9:59 a.m.
New subject: [PATCH 4/4] ch: Don't leak ch_driver->chCaps
During CH driver initialization (chStateInitialize()) the
driver's capabilities bitmap is allocated
(virCHCapsInitCHVersionCaps()), but corresponding free call is
missing in chStateCleanup().
And while at it, reorder calls to virObjectUnref() inside of
chStateCleanup() to be the reverse order of that in
chStateInitialize() so that it's easier to spot missing
free/unref call.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_driver.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index bd271fc0ee..96de5044ac 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -850,10 +850,11 @@ static int chStateCleanup(void)
if (ch_driver == NULL)
return -1;
- virObjectUnref(ch_driver->domains);
+ virBitmapFree(ch_driver->chCaps);
+ virObjectUnref(ch_driver->config);
virObjectUnref(ch_driver->xmlopt);
virObjectUnref(ch_driver->caps);
- virObjectUnref(ch_driver->config);
+ virObjectUnref(ch_driver->domains);
virMutexDestroy(&ch_driver->lock);
g_clear_pointer(&ch_driver, g_free);
--
2.41.0
11:43 a.m.
New subject: [PATCH 4/4] ch: Don't leak ch_driver->chCaps
On 11/28/2023 9:59 AM, Michal Privoznik wrote:
During CH driver initialization (chStateInitialize()) the
driver's capabilities bitmap is allocated
(virCHCapsInitCHVersionCaps()), but corresponding free call is
missing in chStateCleanup().
And while at it, reorder calls to virObjectUnref() inside of
chStateCleanup() to be the reverse order of that in
chStateInitialize() so that it's easier to spot missing
free/unref call.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_driver.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index bd271fc0ee..96de5044ac 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -850,10 +850,11 @@ static int chStateCleanup(void)
if (ch_driver == NULL)
return -1;
- virObjectUnref(ch_driver->domains);
+ virBitmapFree(ch_driver->chCaps);
`chCaps` is implemented as `g_autoptr`. Is this free required here
because `chCaps` never goes out of scope because it is used in `ch_driver`?
If that is the case, is there any value is having `chCaps` as `g_autoptr`?
+ virObjectUnref(ch_driver->config);
virObjectUnref(ch_driver->xmlopt);
virObjectUnref(ch_driver->caps);
- virObjectUnref(ch_driver->config);
+ virObjectUnref(ch_driver->domains);
virMutexDestroy(&ch_driver->lock);
g_clear_pointer(&ch_driver, g_free);
--
Regards,
Praveen
11:52 a.m.
New subject: [PATCH 4/4] ch: Don't leak ch_driver->chCaps
On Tue, Nov 28, 2023 at 11:43:11AM -0600, Praveen K Paladugu wrote:
On 11/28/2023 9:59 AM, Michal Privoznik wrote:
> During CH driver initialization (chStateInitialize()) the
> driver's capabilities bitmap is allocated
> (virCHCapsInitCHVersionCaps()), but corresponding free call is
> missing in chStateCleanup().
>
> And while at it, reorder calls to virObjectUnref() inside of
> chStateCleanup() to be the reverse order of that in
> chStateInitialize() so that it's easier to spot missing
> free/unref call.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/ch/ch_driver.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index bd271fc0ee..96de5044ac 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -850,10 +850,11 @@ static int chStateCleanup(void)
> if (ch_driver == NULL)
> return -1;
> - virObjectUnref(ch_driver->domains);
> + virBitmapFree(ch_driver->chCaps);
`chCaps` is implemented as `g_autoptr`. Is this free required here because
`chCaps` never goes out of scope because it is used in `ch_driver`?
If that is the case, is there any value is having `chCaps` as `g_autoptr`?
In this context 'chCaps' is a struct field, and so we need to free any
fields when the struct (ch_driver) is freed.
The g_autoptr is useful for variables that are only alive / referenced
for the duration of a method (or inner code block scope).
Our best practice is that all data types have g_autoptr support no
matter whether the current code actually needs it or not.
> + virObjectUnref(ch_driver->config);
> virObjectUnref(ch_driver->xmlopt);
> virObjectUnref(ch_driver->caps);
> - virObjectUnref(ch_driver->config);
> + virObjectUnref(ch_driver->domains);
> virMutexDestroy(&ch_driver->lock);
> g_clear_pointer(&ch_driver, g_free);
--
Regards,
Praveen
_______________________________________________
Devel mailing list -- devel(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
12:05 p.m.
New subject: [PATCH 4/4] ch: Don't leak ch_driver->chCaps
On 11/28/2023 11:52 AM, Daniel P. Berrangé wrote:
On Tue, Nov 28, 2023 at 11:43:11AM -0600, Praveen K Paladugu wrote:
>
>
> On 11/28/2023 9:59 AM, Michal Privoznik wrote:
>> During CH driver initialization (chStateInitialize()) the
>> driver's capabilities bitmap is allocated
>> (virCHCapsInitCHVersionCaps()), but corresponding free call is
>> missing in chStateCleanup().
>>
>> And while at it, reorder calls to virObjectUnref() inside of
>> chStateCleanup() to be the reverse order of that in
>> chStateInitialize() so that it's easier to spot missing
>> free/unref call.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/ch/ch_driver.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
>> index bd271fc0ee..96de5044ac 100644
>> --- a/src/ch/ch_driver.c
>> +++ b/src/ch/ch_driver.c
>> @@ -850,10 +850,11 @@ static int chStateCleanup(void)
>> if (ch_driver == NULL)
>> return -1;
>> - virObjectUnref(ch_driver->domains);
>> + virBitmapFree(ch_driver->chCaps);
>
> `chCaps` is implemented as `g_autoptr`. Is this free required here because
> `chCaps` never goes out of scope because it is used in `ch_driver`?
>
> If that is the case, is there any value is having `chCaps` as `g_autoptr`?
In this context 'chCaps' is a struct field, and so we need to free any
fields when the struct (ch_driver) is freed.
The g_autoptr is useful for variables that are only alive / referenced
for the duration of a method (or inner code block scope).
Our best practice is that all data types have g_autoptr support no
matter whether the current code actually needs it or not.
Thanks for clarifying that.
>> + virObjectUnref(ch_driver->config);
>> virObjectUnref(ch_driver->xmlopt);
>> virObjectUnref(ch_driver->caps);
>> - virObjectUnref(ch_driver->config);
>> + virObjectUnref(ch_driver->domains);
>> virMutexDestroy(&ch_driver->lock);
>> g_clear_pointer(&ch_driver, g_free);
>
> --
> Regards,
> Praveen
> _______________________________________________
> Devel mailing list -- devel(a)lists.libvirt.org
> To unsubscribe send an email to devel-leave(a)lists.libvirt.org
With regards,
Daniel
--
Regards,
Praveen
10:11 a.m.
On 11/28/23 9:59 AM, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (4):
virpci: Decrease scope of VIR_PF_PHYS_PORT_NAME_REGEX macro
qemu_command: Don't open code virPCIDeviceAddressAsString()
vircpi: Decrease scope of VIR_PCI_DEVICE_ADDRESS_FMT macro
ch: Don't leak ch_driver->chCaps
src/ch/ch_driver.c | 5 +++--
src/qemu/qemu_command.c | 6 +-----
src/util/virpci.c | 9 +++++++++
src/util/virpci.h | 7 -------
4 files changed, 13 insertions(+), 14 deletions(-)
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
10:12 a.m.
On a Tuesday in 2023, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (4):
virpci: Decrease scope of VIR_PF_PHYS_PORT_NAME_REGEX macro
qemu_command: Don't open code virPCIDeviceAddressAsString()
vircpi: Decrease scope of VIR_PCI_DEVICE_ADDRESS_FMT macro
ch: Don't leak ch_driver->chCaps
src/ch/ch_driver.c | 5 +++--
src/qemu/qemu_command.c | 6 +-----
src/util/virpci.c | 9 +++++++++
src/util/virpci.h | 7 -------
4 files changed, 13 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Also, patch 4/4 might be worth including in this release.
Jano
361
days inactive
361
days old
10 comments
5 participants
participants (5)
-
Daniel P. Berrangé -
Jonathon Jongsma -
Ján Tomko -
Michal Privoznik -
Praveen K Paladugu