Re: [PATCH 00/28] a bunch of domain_conf cleanup
On 11/6/20 4:32 AM, Matt Coleman wrote: Most of this is making functions void that unnecessarily return an int. It also includes some conversion to GLib. Feel free to squash related commits, if you'd like. I left them separate to make it easier to review. Matt Coleman (28): domain_conf: make virDomainDiskSetDriver() void domain_conf: make virDomainPostParseCheckISCSIPath() void domain_conf: use g_free() in virDomainPostParseCheckISCSIPath() domain_conf: make virDomainHostdevAssignAddress() void domain_conf: make virDomainChr/RNG/Video/VsockDefPostParse() and virDomainNVRAMDefFormat() void domain_conf: make virDomainDeviceInfoFormat() void domain_conf: make virDomainGraphicsDefParseXMLEGLHeadless() void domain_conf: make virDomainLeaseDefFormat() void domain_conf: make virDomainDiskSourceFormatNetwork() void domain_conf: make virDomainDiskDefFormatIotune() void domain_conf: make virDomainDiskDefFormatDriver() void domain_conf: make virDomainControllerDriverFormat() void domain_conf: make virDomainVirtioNetGuestOpts/HostOpts/DriverFormat() void domain_conf: make virDomainRedirFilterDefFormat() void domain_conf: make virDomainIOMMUDefFormat() void domain_conf: make virDomainDefFormatBlkiotune() void domain_conf: make virDomainChrSourceDefFormat() void domain_conf: make virDomainDiskSetBlockIOTune() void domain_conf: use g_free in virDomainDiskSetBlockIOTune() domain_conf: use g_renew in virDomainDiskInsert() and virDomainControllerInsert() domain_conf: make virDomainDiskInsert() void domain_conf: make virDomainControllerInsert() void domain_conf: use g_renew in virDomainLeaseInsertPreAlloc() domain_conf: make virDomainLeaseInsertPreAlloc() void domain_conf: make virDomainLeaseInsert() void domain_conf: make virDomainPanicDefFormat() void domain_conf: make virDomainShmemDefFormat() void domain_conf: make virDomainVsockDefFormat() void src/conf/domain_conf.c | 349 ++- src/conf/domain_conf.h | 21 +-- src/libxl/libxl_conf.c | 5 +- src/libxl/libxl_domain.c | 5 +- src/libxl/libxl_driver.c | 9 +- src/libxl/xen_xl.c | 12 +- src/libxl/xen_xm.c | 10 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_domain.c | 5 +- src/qemu/qemu_driver.c | 15 +- src/qemu/qemu_hotplug.c | 3 +- src/test/test_driver.c | 3 +- src/vz/vz_sdk.c | 9 +- tests/qemublocktest.c| 5 +- 14 files changed, 158 insertions(+), 296 deletions(-) I've dropped the g_free() patches (even though in this specific case they free a pointer which is set right after), Reviewed-by: Michal Privoznik and pushed. Michal
Re: [PATCH 00/28] a bunch of domain_conf cleanup
> On Nov 9, 2020, at 9:43 AM, Michal Privoznik wrote: > > Nice cleanup. But as I say in 03/28 I think we might want glib adoption to be > done in bigger chunks. Usually we rewrite VIR_ALLOC/VIR_REALLOC_N -> > g_new0()/g_renew() in one patch (might be coupled with g_free() except > VIR_FREE() resets the pointer to NULL and g_free() doesn't do that so I'm not > really a fan of g_free()), then g_strdup() in another path, and so on. I had thought that g_free() was a drop-in replacement for VIR_FREE() based on other earlier commits that I had seen. You can drop the g_free() commits from this series if everything else looks good. -- Matt
Re: [PATCH 00/28] a bunch of domain_conf cleanup
On 11/11/20 8:18 AM, Matt Coleman wrote: On Nov 9, 2020, at 9:43 AM, Michal Privoznik wrote: Nice cleanup. But as I say in 03/28 I think we might want glib adoption to be done in bigger chunks. Usually we rewrite VIR_ALLOC/VIR_REALLOC_N -> g_new0()/g_renew() in one patch (might be coupled with g_free() except VIR_FREE() resets the pointer to NULL and g_free() doesn't do that so I'm not really a fan of g_free()), then g_strdup() in another path, and so on. I had thought that g_free() was a drop-in replacement for VIR_FREE() based on other earlier commits that I had seen. You can drop the g_free() commits from this series if everything else looks good. Yes, please. VIR_FREE() is perfect as is :-) g_free() doesn't clear the pointer and g_clear_pointer(, g_free); is just too long. Michal
Re: [PATCH 00/28] a bunch of domain_conf cleanup
On a Monday in 2020, Michal Privoznik wrote: On 11/6/20 4:32 AM, Matt Coleman wrote: Most of this is making functions void that unnecessarily return an int. It also includes some conversion to GLib. Feel free to squash related commits, if you'd like. I left them separate to make it easier to review. Yeah, some might be squashed. 14 files changed, 158 insertions(+), 296 deletions(-) Nice cleanup. But as I say in 03/28 I think we might want glib adoption to be done in bigger chunks. Of course a bulk rewrite to g_free would get rid of VIR_FREE faster. But this series is about int -> void conversion. I think it's perfectly fine to include minor fixes you notice while doing that. No need to shave the whole yak [0] :) Jano [0] https://en.wiktionary.org/wiki/yak_shaving#Noun Usually we rewrite VIR_ALLOC/VIR_REALLOC_N -> g_new0()/g_renew() in one patch (might be coupled with g_free() except VIR_FREE() resets the pointer to NULL and g_free() doesn't do that so I'm not really a fan of g_free()), then g_strdup() in another path, and so on. Michal signature.asc Description: PGP signature
Re: [PATCH 00/28] a bunch of domain_conf cleanup
On 11/6/20 4:32 AM, Matt Coleman wrote: Most of this is making functions void that unnecessarily return an int. It also includes some conversion to GLib. Feel free to squash related commits, if you'd like. I left them separate to make it easier to review. Yeah, some might be squashed. 14 files changed, 158 insertions(+), 296 deletions(-) Nice cleanup. But as I say in 03/28 I think we might want glib adoption to be done in bigger chunks. Usually we rewrite VIR_ALLOC/VIR_REALLOC_N -> g_new0()/g_renew() in one patch (might be coupled with g_free() except VIR_FREE() resets the pointer to NULL and g_free() doesn't do that so I'm not really a fan of g_free()), then g_strdup() in another path, and so on. Michal
Re: [PATCH 00/28] a bunch of domain_conf cleanup
On Thu, Nov 5, 2020 at 10:33 PM Matt Coleman wrote: > > Most of this is making functions void that unnecessarily return an int. > It also includes some conversion to GLib. > > Feel free to squash related commits, if you'd like. I left them separate > to make it easier to review. > > Matt Coleman (28): > domain_conf: make virDomainDiskSetDriver() void > domain_conf: make virDomainPostParseCheckISCSIPath() void > domain_conf: use g_free() in virDomainPostParseCheckISCSIPath() > domain_conf: make virDomainHostdevAssignAddress() void > domain_conf: make virDomainChr/RNG/Video/VsockDefPostParse() and > virDomainNVRAMDefFormat() void > domain_conf: make virDomainDeviceInfoFormat() void > domain_conf: make virDomainGraphicsDefParseXMLEGLHeadless() void > domain_conf: make virDomainLeaseDefFormat() void > domain_conf: make virDomainDiskSourceFormatNetwork() void > domain_conf: make virDomainDiskDefFormatIotune() void > domain_conf: make virDomainDiskDefFormatDriver() void > domain_conf: make virDomainControllerDriverFormat() void > domain_conf: make virDomainVirtioNetGuestOpts/HostOpts/DriverFormat() > void > domain_conf: make virDomainRedirFilterDefFormat() void > domain_conf: make virDomainIOMMUDefFormat() void > domain_conf: make virDomainDefFormatBlkiotune() void > domain_conf: make virDomainChrSourceDefFormat() void > domain_conf: make virDomainDiskSetBlockIOTune() void > domain_conf: use g_free in virDomainDiskSetBlockIOTune() > domain_conf: use g_renew in virDomainDiskInsert() and > virDomainControllerInsert() > domain_conf: make virDomainDiskInsert() void > domain_conf: make virDomainControllerInsert() void > domain_conf: use g_renew in virDomainLeaseInsertPreAlloc() > domain_conf: make virDomainLeaseInsertPreAlloc() void > domain_conf: make virDomainLeaseInsert() void > domain_conf: make virDomainPanicDefFormat() void > domain_conf: make virDomainShmemDefFormat() void > domain_conf: make virDomainVsockDefFormat() void > > src/conf/domain_conf.c | 349 ++- > src/conf/domain_conf.h | 21 +-- > src/libxl/libxl_conf.c | 5 +- > src/libxl/libxl_domain.c | 5 +- > src/libxl/libxl_driver.c | 9 +- > src/libxl/xen_xl.c | 12 +- > src/libxl/xen_xm.c | 10 +- > src/lxc/lxc_driver.c | 3 +- > src/qemu/qemu_domain.c | 5 +- > src/qemu/qemu_driver.c | 15 +- > src/qemu/qemu_hotplug.c | 3 +- > src/test/test_driver.c | 3 +- > src/vz/vz_sdk.c | 9 +- > tests/qemublocktest.c| 5 +- > 14 files changed, 158 insertions(+), 296 deletions(-) > > -- > 2.27.0 > > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
[PATCH 00/28] a bunch of domain_conf cleanup
Most of this is making functions void that unnecessarily return an int. It also includes some conversion to GLib. Feel free to squash related commits, if you'd like. I left them separate to make it easier to review. Matt Coleman (28): domain_conf: make virDomainDiskSetDriver() void domain_conf: make virDomainPostParseCheckISCSIPath() void domain_conf: use g_free() in virDomainPostParseCheckISCSIPath() domain_conf: make virDomainHostdevAssignAddress() void domain_conf: make virDomainChr/RNG/Video/VsockDefPostParse() and virDomainNVRAMDefFormat() void domain_conf: make virDomainDeviceInfoFormat() void domain_conf: make virDomainGraphicsDefParseXMLEGLHeadless() void domain_conf: make virDomainLeaseDefFormat() void domain_conf: make virDomainDiskSourceFormatNetwork() void domain_conf: make virDomainDiskDefFormatIotune() void domain_conf: make virDomainDiskDefFormatDriver() void domain_conf: make virDomainControllerDriverFormat() void domain_conf: make virDomainVirtioNetGuestOpts/HostOpts/DriverFormat() void domain_conf: make virDomainRedirFilterDefFormat() void domain_conf: make virDomainIOMMUDefFormat() void domain_conf: make virDomainDefFormatBlkiotune() void domain_conf: make virDomainChrSourceDefFormat() void domain_conf: make virDomainDiskSetBlockIOTune() void domain_conf: use g_free in virDomainDiskSetBlockIOTune() domain_conf: use g_renew in virDomainDiskInsert() and virDomainControllerInsert() domain_conf: make virDomainDiskInsert() void domain_conf: make virDomainControllerInsert() void domain_conf: use g_renew in virDomainLeaseInsertPreAlloc() domain_conf: make virDomainLeaseInsertPreAlloc() void domain_conf: make virDomainLeaseInsert() void domain_conf: make virDomainPanicDefFormat() void domain_conf: make virDomainShmemDefFormat() void domain_conf: make virDomainVsockDefFormat() void src/conf/domain_conf.c | 349 ++- src/conf/domain_conf.h | 21 +-- src/libxl/libxl_conf.c | 5 +- src/libxl/libxl_domain.c | 5 +- src/libxl/libxl_driver.c | 9 +- src/libxl/xen_xl.c | 12 +- src/libxl/xen_xm.c | 10 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_domain.c | 5 +- src/qemu/qemu_driver.c | 15 +- src/qemu/qemu_hotplug.c | 3 +- src/test/test_driver.c | 3 +- src/vz/vz_sdk.c | 9 +- tests/qemublocktest.c| 5 +- 14 files changed, 158 insertions(+), 296 deletions(-) -- 2.27.0
