On Thu, Oct 22, 2020 at 14:08:51 -0500, Jonathon Jongsma wrote:
> On Thu, 22 Oct 2020 09:01:13 +0200
> Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
>
> > On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote:
> > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx>
> > >
> > > "
> > >
> > >
> > > 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
> >
> > I'd like to point out that opening anything in the command line
> > formatters is IMO bad practice. The command line formatter should
> > format the commandline and nothing more. This is visible by the
> > necessity to have a lot of mock override functions which prevent the
> > command line formatter from touching resources on the host in the
> > first place.
> >
> > Better approach is to open resources in 'qemuProcessPrepareHost' and
> > store them in private data for command line formatting. Fake data for
> > tests are added in 'testCompareXMLToArgvCreateArgs'.
> >
> > I'm aware though that there's a lot of "prior art" in this area
> > though.
>
> These are good points. I fell into the trap of modeling the new code on
> some existing code that did similar things rather than thinking
> critically enough about the best way to do it. I'll look into a better
> approach.
Just keep this as a note for future code. No need to refactor your
addition, since there's a lot of other code which does the same. The
fix in this patch is fine once you drop the unneeded conditional.
