10:20 p.m.
On 10/22/20 3:01 AM, Peter Krempa 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(a)redhat.com>
>> ---
>> 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>
>
> "
>
>
> 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.
Well... I agree that it is an ugly design, but that's pretty much what's
in place for almost everything.
The command line formatter should format
the commandline and nothing more.
It would be nice if that was the case, but it already isn't. :-/
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'.
That's nice in the fact that it eliminates the need for mock overrides
(would be even *nicer* if that function had even a single line of
documentation included that described its purpose, and what code in the
qemu driver it should be mimicking, amirite?).
But it's bad because the code in qemuProcessPrepareHost() won't be
tested (can't be tested if there are no mocks for the system functions
it calls). Basically you're trading the extra work of mocking
system-level functions for the extra work of filling in stuff in the
privateData (and the maintenance of that code), and eliminating testing
of the code that's been moved (pretty *lame* testing, I'll admit, since
it's getting back canned results from the fake system calls).
So it's not really the panacea your advocacy implies :-)
(Don't get me wrong! I've always disliked the mixing of
device/file/whatever init with the commandline generating functions.)
(actually a couple months ago I looked into putting the network
interface "prepare" stuff into privateData similar to what's done with
the slirp stuff now. In the end I gave up because it didn't provide the
result I wanted - I was trying to keep track of what device prep actions
had been done for which devices during domain startup so that the
shutdown in case of startup failure would only shutdown those things
that had actually been setup; it ended up being too complicated to make
it work correctly both in the case of an aborted startup and a normal
shutdown, once you took into account the possibility of libvirtd being
restarted as part of a libvirt package update.
I'll point out that during all my searches through the code during the
experiment referenced in the previous paragraph, I never ran across
testCompareXMLToArgvCreateArgs(), and didn't know of its existence (or
at least didn't remember it, if I had known about it before). Is this
documented somewhere? Or is it expected to be learned by reading every
patch coming across the mailing list (I unfortunately fail at that in a
major way)?
I'm aware though that there's a lot of "prior art"
in this area though.
... and nothing in the code or the coding practices to warn against it,
point people in the other direction.
This sounds like another "saga" in the making - split all commandline
generating functions into separate "prepare device" and "generate
commandline" parts. I don't know that we should require Jonathon to
change his code that much just to fix a memory leak though ... (too bad
I hadn't kept up with the latest cool stuff so I would have pointed it
out in review of the original patch).