Re: [libvirt PATCH] qemu: fix potential resource leak

On 10/21/20 5:50 PM, Jonathon Jongsma wrote: > Coverity reported a potential resource leak. While it's probably not > a real-world scenario, the code could technically jump to cleanup > between the time that vdpafd is opened and when it is used. Ensure that > it gets cleaned up in that case. > > Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 5c4e37bd9e..cbe7a6e331 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -8135,6 +8135,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, > addfdarg = g_strdup_printf("%s,opaque=%s", fdset, > net->data.vdpa.devicepath); > virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL); > + vdpafd = -1; > } > if (chardev) > @@ -8204,6 +8205,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, > VIR_FREE(tapfdName); > VIR_FREE(vhostfd); > VIR_FREE(tapfd); > + if (vdpafd >= 0) > + VIR_FORCE_CLOSE(vdpafd); VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose() and virFileClose() is a NOP if fd < 0, so this doesn't need the conditional. I *was* going to say "With that change, Reviewed-by: Laine Stump <laine(a)redhat.com&gt; " but this got me looking at the code of the entire function rather than just the diffs in the patch, and I've got a question - is there any reason that you only ope n the vdpa device inside the switch, and save everything else related to it until later (at the "if (vdpafd)")? You could then device