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.
