Re: [PATCH 00/12] use g_autoptr for all DIR*
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(a)gmail.com>