qemu: Don't leak file descriptors in case of error

[c3d]

If we take one of the error paths from setupVirtiofsd() after
opening the fd variable, the fd.Close() function is not called.

Fixes: #2683

Signed-off-by: Christophe de Dinechin dinechin@redhat.com

[jodh-intel]

Good catch @c3d, and thanks!

lgtm

[jodh-intel]

/test-ubuntu

[jcvenegas]

I think you need to remove line

runtime/virtcontainers/qemu.go

Line 680 in 7b269ff

fd.Close()

[fidencio]

Shouldn't we remove the fd.Close() call a few lines below?

[grahamwhaley]

OOI, I had wondered if we needed to drop the later fd.Close() as well - but, hmm, as it is it means we close the fd after its last use. If we drop that, it will stay open for longer when not needed. On the flip side, we can only leave it there if calling fd.Close() on an already closed fd is safe - as it will still be called by the defer...

[c3d]

The intent of that fd.Close() is clearly to close the fd before entering the loop, so I left it there intentionally, after checking that closing twice was OK.

Close closes the File, rendering it unusable for I/O. On files that support SetDeadline, any pending I/O operations will be canceled and return immediately with an error. Close will return an error if it has already been called.

(Emphasis mine)

Reference: https://golang.org/pkg/os/#File.Close

[fidencio]

So, just to confirm, the final action on that is to remove the fd.Close() line mentioned, considering "Close will return an error if it has already been called". is it?

(Christophe's Emphasis ;-))

[fidencio]

Ah, I see @c3d's point after a private conversation.

fd.Close() will error out with "already closed", but won't crash, so that may be fine as we don't check / report that error.

I'm fine with this as is now. @jcvenegas, do you agree? If so, feel free to remove the "do-not-merge" label and merge it.

[jcvenegas]

Hey thanks for the clarifications, I agree that will not be harmful in the case if it is called two times. I think is just a matter of different styles but the same result. let's merge it :)

[fidencio]

@jcvenegas was faster than me! ;-)
Adding the DNM label till the fd.Close() comment is addressed.

[codecov]

Codecov Report

Merging #2684 into master will increase coverage by 4.55%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2684      +/-   ##
==========================================
+ Coverage   45.93%   50.48%   +4.55%     
==========================================
  Files         118      118              
  Lines       17653    17139     -514     
==========================================
+ Hits         8109     8653     +544     
+ Misses       8628     7428    -1200     
- Partials      916     1058     +142     

[c3d]

I think you need to remove line

runtime/virtcontainers/qemu.go

Line 680 in 7b269ff

fd.Close()

I disagree. First, it's safe to fd.Close() twice, see other comment above. Second, the existing fd.Close() is clearly intended to not leave a useless fd open while you run the loop following it.