use g_autoptr for all DIR*

[PATCH 00/12] use g_autoptr for all DIR*

Posted by Laine Stump 3 years, 5 months ago

I don't even remember why I started looking at this. I think that
again I was considering changing some function, and making the DIR* in
that function autoclose was a prerequisite for a reasonably clean
addition to the function. I eventually gave up on the other plan
(probably because it was a bad idea), but decided that the DIR*'s
really did need to autoclose.

In the end, all uses of DIR* could be easily converted to use
g_autoptr.

Laine Stump (12):
  consistently use VIR_DIR_CLOSE() instead of virDirClose()
  tools: reduce scope of a DIR* in virHostValidateIOMMU()
  storage: remove extraneous call to VIR_DIR_CLOSE()
  util: reduce scope of a DIR * in virCgroupV1SetOwner()
  util: manually set dirp to NULL after closing in
    virCapabilitiesInitCache()
  util: change virDirClose to take a DIR* instead of DIR**.
  util: declare g_autoptr cleanup function to auto-close DIR*
  change DIR* int g_autoptr(DIR) where appropriate
  conf: convert final DIR* to g_autoptr
  util: remove unused VIR_DIR_CLOSE() macro
  util:  refactor function to simplify and remove label
  remove unnecessary cleanup labels and unused return variables

 src/bhyve/bhyve_capabilities.c       |  3 +-
 src/conf/capabilities.c              |  5 +-
 src/conf/virdomainobjlist.c          |  3 +-
 src/conf/virnetworkobj.c             | 44 +++++---------
 src/conf/virnwfilterbindingobjlist.c |  3 +-
 src/conf/virnwfilterobj.c            |  3 +-
 src/conf/virsecretobj.c              |  3 +-
 src/conf/virstorageobj.c             |  6 +-
 src/openvz/openvz_conf.c             |  3 +-
 src/qemu/qemu_driver.c               |  6 +-
 src/qemu/qemu_interop_config.c       | 14 ++---
 src/security/security_selinux.c      |  8 +--
 src/storage/storage_backend_iscsi.c  |  3 +-
 src/storage/storage_util.c           | 69 ++++++++-------------
 src/util/vircgroup.c                 | 23 +++----
 src/util/vircgroupv1.c               |  5 +-
 src/util/vircommand.c                | 12 ++--
 src/util/virdevmapper.c              |  9 +--
 src/util/virfile.c                   | 65 ++++++++------------
 src/util/virfile.h                   |  4 +-
 src/util/virhook.c                   |  8 +--
 src/util/virhostcpu.c                |  6 +-
 src/util/virnetdev.c                 | 13 ++--
 src/util/virnuma.c                   | 24 +++-----
 src/util/virpci.c                    | 91 +++++++++++-----------------
 src/util/virprocess.c                |  3 +-
 src/util/virresctrl.c                | 30 ++++-----
 src/util/virscsi.c                   | 32 ++++------
 src/util/virscsihost.c               |  3 +-
 src/util/virusb.c                    |  3 +-
 src/util/virutil.c                   | 26 +++-----
 src/util/virvhba.c                   | 20 +++---
 tests/testutilsqemu.c                | 35 ++++-------
 tests/virschematest.c                |  3 +-
 tools/virt-host-validate-common.c    |  4 +-
 35 files changed, 206 insertions(+), 386 deletions(-)

-- 
2.26.2

Re: [PATCH 00/12] use g_autoptr for all DIR*

Posted by Daniel Henrique Barboza 3 years, 5 months ago

Hey,

On 10/27/20 10:35 PM, Laine Stump wrote:
> I don't even remember why I started looking at this. I think that
> again I was considering changing some function, and making the DIR* in
> that function autoclose was a prerequisite for a reasonably clean
> addition to the function. I eventually gave up on the other plan
> (probably because it was a bad idea), but decided that the DIR*'s
> really did need to autoclose.
> 
> In the end, all uses of DIR* could be easily converted to use
> g_autoptr.


I think you created this series in parallel with your "node_device: fix leak
of DIR*" patch, right?. Because you're not changing 'node_device_udev.c' anywhere in
this series, meaning that the code base you used still have the DIR leak in
udevGetVDPACharDev(). The code base didn't have the added VIR_DIR_CLOSE() calls there
to fix the leak, and I believe you forgot to take that into account here. The end result
is that the leak fix patch will break after patch 10 removes the VIR_DIR_CLOSE()
macro.

A quick fix is simply using "node_device: fix leak of DIR*" as the first
patch of this series, then you can handle the removal of VIR_DIR_CLOSE()
and g_autoptr() for that case in patch 8. There's no cleanup labels to
be added there, so it's a trivial removal of the two VIR_DIR_CLOSE()
calls and turning the DIR var to g_autoptr().

Assuming you go with that (and fix my whitespace nit in patch 12! :P ), feel
free to add my R-b in all patches:


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>



Thanks,

DHB


> 
> Laine Stump (12):
>    consistently use VIR_DIR_CLOSE() instead of virDirClose()
>    tools: reduce scope of a DIR* in virHostValidateIOMMU()
>    storage: remove extraneous call to VIR_DIR_CLOSE()
>    util: reduce scope of a DIR * in virCgroupV1SetOwner()
>    util: manually set dirp to NULL after closing in
>      virCapabilitiesInitCache()
>    util: change virDirClose to take a DIR* instead of DIR**.
>    util: declare g_autoptr cleanup function to auto-close DIR*
>    change DIR* int g_autoptr(DIR) where appropriate
>    conf: convert final DIR* to g_autoptr
>    util: remove unused VIR_DIR_CLOSE() macro
>    util:  refactor function to simplify and remove label
>    remove unnecessary cleanup labels and unused return variables
> 
>   src/bhyve/bhyve_capabilities.c       |  3 +-
>   src/conf/capabilities.c              |  5 +-
>   src/conf/virdomainobjlist.c          |  3 +-
>   src/conf/virnetworkobj.c             | 44 +++++---------
>   src/conf/virnwfilterbindingobjlist.c |  3 +-
>   src/conf/virnwfilterobj.c            |  3 +-
>   src/conf/virsecretobj.c              |  3 +-
>   src/conf/virstorageobj.c             |  6 +-
>   src/openvz/openvz_conf.c             |  3 +-
>   src/qemu/qemu_driver.c               |  6 +-
>   src/qemu/qemu_interop_config.c       | 14 ++---
>   src/security/security_selinux.c      |  8 +--
>   src/storage/storage_backend_iscsi.c  |  3 +-
>   src/storage/storage_util.c           | 69 ++++++++-------------
>   src/util/vircgroup.c                 | 23 +++----
>   src/util/vircgroupv1.c               |  5 +-
>   src/util/vircommand.c                | 12 ++--
>   src/util/virdevmapper.c              |  9 +--
>   src/util/virfile.c                   | 65 ++++++++------------
>   src/util/virfile.h                   |  4 +-
>   src/util/virhook.c                   |  8 +--
>   src/util/virhostcpu.c                |  6 +-
>   src/util/virnetdev.c                 | 13 ++--
>   src/util/virnuma.c                   | 24 +++-----
>   src/util/virpci.c                    | 91 +++++++++++-----------------
>   src/util/virprocess.c                |  3 +-
>   src/util/virresctrl.c                | 30 ++++-----
>   src/util/virscsi.c                   | 32 ++++------
>   src/util/virscsihost.c               |  3 +-
>   src/util/virusb.c                    |  3 +-
>   src/util/virutil.c                   | 26 +++-----
>   src/util/virvhba.c                   | 20 +++---
>   tests/testutilsqemu.c                | 35 ++++-------
>   tests/virschematest.c                |  3 +-
>   tools/virt-host-validate-common.c    |  4 +-
>   35 files changed, 206 insertions(+), 386 deletions(-)
>

Re: [PATCH 00/12] use g_autoptr for all DIR*

Posted by Laine Stump 3 years, 5 months ago

On 10/28/20 7:39 AM, Daniel Henrique Barboza wrote:
> Hey,
> 
> On 10/27/20 10:35 PM, Laine Stump wrote:
>> I don't even remember why I started looking at this. I think that
>> again I was considering changing some function, and making the DIR* in
>> that function autoclose was a prerequisite for a reasonably clean
>> addition to the function. I eventually gave up on the other plan
>> (probably because it was a bad idea), but decided that the DIR*'s
>> really did need to autoclose.
>>
>> In the end, all uses of DIR* could be easily converted to use
>> g_autoptr.
> 
> 
> I think you created this series in parallel with your "node_device: fix 
> leak
> of DIR*" patch, right?.

No. This series has been building up for a couple weeks, but the leak 
just came up 2 days ago and I didn't write the patch for it until I had 
finished and posted this series. But the node-device bugfix needs to be 
pushed before the next release, while this series won't be pushed until 
after the release, so the bugfix had to be based on the current state of 
upstream, rather than stacked on top of other work.

> Because you're not changing 'node_device_udev.c' 
> anywhere in
> this series, meaning that the code base you used still have the DIR leak in
> udevGetVDPACharDev(). The code base didn't have the added 
> VIR_DIR_CLOSE() calls there
> to fix the leak, and I believe you forgot to take that into account 
> here. The end result
> is that the leak fix patch will break after patch 10 removes the 
> VIR_DIR_CLOSE()
> macro.

Yep. It wasn't forgotten, just done in a different order than you 
assumed, and both had to be based on current upstream rather than 
dependent on each other.

> 
> A quick fix is simply using "node_device: fix leak of DIR*" as the first
> patch of this series, then you can handle the removal of VIR_DIR_CLOSE()
> and g_autoptr() for that case in patch 8.

Yep, that's what I'd planned to do.

> There's no cleanup labels to
> be added there, so it's a trivial removal of the two VIR_DIR_CLOSE()
> calls and turning the DIR var to g_autoptr().

> 
> Assuming you go with that (and fix my whitespace nit in patch 12! :P ), 
> feel
> free to add my R-b in all patches:

Okay, thanks!

> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>