api: Move close function in condition body

[mi4r]

The close is moved in the body of the error condition.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Does this PR introduce a user-facing change?

   None

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[baude]

approved not having tests

[baude]

in looking at this PR, I saw the the error is not handled or logged. Then looking at just this file more broadly, sometimes it is and sometimes not. Some are deferred. If we have a consensus on how these should handled in the whole file, I would prefer that. I did not look close enough to verify that this is a possibility.

@mheon @Luap99 wdyt

[Luap99]

I suggested this but it only makes sense if the error on tmpfile.Close() above source = tmpfile.Name() is checked and so that there was no errors during write.

Using defer in this context is not helpful because we want the tmpfile to be closed before we pass the file path to the import API.

[mheon]

I don't think there's value in logging the error from close. Maybe do an explicit _ = xyz.Close() so it's clear we are actually ignoring it.

[Luap99]

I don't think there's value in logging the error from close. Maybe do an explicit _ = xyz.Close() so it's clear we are actually ignoring it.

Read the close() man page, there absolute is a reason to check for errors (even when super unlikely)

A careful programmer will check the return value of close(), since it is quite possible that errors on a previous write(2) operation are reported only on the final close() that releases the open file description. Failing to
check the return value when closing a file may lead to silent loss of data. This can especially be observed with NFS and with disk quota.

I know we ignore this pretty much elsewhere but here we are actually writing something potential large that my increase the odds of hitting such cases.

[baude]

This PR has been marked for 5.3 inclusion but it must be merged prior to Nov 5 for inclusion in 5.3 RC3. PRs not merged by that date are considered on a case by case basis for backporting.

[mi4r]

Once again I would like to clarify what needs to be done. Do you want me to add if err = tmpfile.Close(); err != nill {log} or do you want me to fix this everywhere in the file, or would it be better to make this a separate pr and then accept this one without processing this error?

[Luap99]

I would want this

diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go
index 6b5e5abb3..3e35a0dbb 100644
--- a/pkg/api/handlers/libpod/images.go
+++ b/pkg/api/handlers/libpod/images.go
@@ -410,7 +410,10 @@ func ImagesImport(w http.ResponseWriter, r *http.Request) {
                        return
                }
 
-               tmpfile.Close()
+               if err := tmpfile.Close(); err != nil {
+                       utils.Error(w, http.StatusInternalServerError, fmt.Errorf("unable to close tempfile: %w", err))
+                       return
+               }
                source = tmpfile.Name()
        }
 

ignoring the close error on the copy error path is fine but on the good path we should make sure we closed the file successfully to avoid io errors

[mheon]

LGTM

[rhatdan]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mi4r, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

Thanks @mi4r