local tmpfile may be not closed when 'DownloadFromContainer' returns err

[guangxuli]

In function copyImageSource, if dockerClient.DownloadFromContainer returns err, tempFile will not be closed.

[oatmealraisin]

Would it be easier to just tempFile.Close() before returning the 270/277 error?

if err != nil {
   tempFile.Close()
   return err
}

[bparees]

we really don't care about this because it's happening inside a container that's about to be thrown away if we get an error, so cleaning up is moot.

[bparees]

but if we do want to make any change, i agree w/ @oatmealraisin that that seems like the simpler fix.

[bparees]

and for that matter do we even know that dockerClient.DownloadFromContainer leaves the file open if it throws an error?

[guangxuli]

thanks @bparees @oatmealraisin yeah, I agree this way can fix this PR more easier. I think it is not good coding style, even through it's happening inside a container, more or less, our code should not too much rely on what environment it will be run.
Another case, if there is another function returns err, do we need also add this:

if err != nil {
tempFile.Close()
return err
}

I read some our code that handle such problem like this. In file host.go
the function “CopyFromHost”, "CopyMasterConfigToHost". I am not sure this is the best way to fix the PR at first also. but it looks like more general. We don't need care when or where the file should be closed. What do you think? thanks.

[bparees]

But I think it is not good coding style, our code should not too much rely on what environment it will be run

taking advantage of the environment the code is going to run in is a reasonable optimization as long as you understand the assumptions you've made.

Another case, if there is another function returns err, do we need also add this:

no because it's already being closed right after the error check. the only possible exit in the code today that doesn't result in closing the file, is if dockerClient.DownloadFromContainer left it open and returned an error. Hence my question to you of whether dockerClient.DownloadFromContainer even leaves the file open on error.

[guangxuli]

@bparees thanks for you suggestion, maybe I'm thinking too much. Yes, the real question is the file not be closed when dockerClient.DownloadFromContainer returned an error. Do you mean add tempFile.Close() when dockerClient.DownloadFromContainer returned an error?

[bparees]

Do you mean add tempFile.Close() when dockerClient.DownloadFromContainer returned an error?

that was @oatmealraisin's original suggestion and i agree with it, yes. Except, again, that before we go adding a close on the file, we should convince ourselves that DownloadFromContainer hasn't already closed the file in the case that it is returning an error.

[guangxuli]

@bparees I think it is difficult to confirm whether it is returning a error when the file not be closed. Maybe the error will never happen. But When I read the code I feel a little confusing, more or less.
I have modifed the code. Do you agree add file close?

[bparees]

@guangxuli i've reviewed the DownloadFromContainer function, it does not close the stream when returning an error.

lgtm

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7835/) (Image: devenv-rhel7_4811)

[openshift-bot]

Evaluated for origin merge up to e7e397a

[openshift-bot]

[Test]ing while waiting on the merge queue

[openshift-bot]

Evaluated for origin test up to e7e397a

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7826/)