cleanup created bundle when creating failed #172
Conversation
Mashimiao
force-pushed
the
cleanup-bundle
branch
3 times, most recently
from
70e0649 to
c5ed3a8
Compare
image/image.go
Outdated
| defer func() { | ||
| if err != nil { | ||
| if err3 := os.RemoveAll(dest); err3 != nil { | ||
| fmt.Printf("warning: failed to clean up %q\n", dest) |
There was a problem hiding this comment.
Can we wrap err3? I'd like to expose the reason for the failure to the user.
There was a problem hiding this comment.
I think we can't, because we can't return any value in defer function.
There was a problem hiding this comment.
Can you squash it into err? Or at least print it here?
There was a problem hiding this comment.
Let's print it here, updated
Did you forget to push? I still see this PR pointing at c5ed3a8, has a 2017-08-04 commit date.
Mashimiao
force-pushed
the
cleanup-bundle
branch
from
c5ed3a8 to
5e6f503
Compare
image/image.go
Outdated
| @@ -301,6 +301,13 @@ func createBundle(w walker, m *manifest, dest, rootfs string) error { | |||
| if err2 := os.MkdirAll(dest, 0755); err2 != nil { | |||
| return err2 | |||
| } | |||
| defer func() { | |||
| if err != nil { | |||
| if err3 := os.RemoveAll(dest); err3 != nil { | |||
There was a problem hiding this comment.
With this defer, it may be good to use named returns.
There was a problem hiding this comment.
Sorry, I don't know what you mean. What kind of named return?
There was a problem hiding this comment.
func createBundle(w walker, m *manifest, dest, rootfs string) ( retErr error)
and use
defer func() {
if retErr != nil {
}
}()
here
There was a problem hiding this comment.
It seems still can't return err and err3 at same time.
There was a problem hiding this comment.
Signed-off-by: Ma Shimiao <[email protected]>
Mashimiao
force-pushed
the
cleanup-bundle
branch
from
5e6f503 to
4abe1a1
Compare
| @@ -301,6 +301,13 @@ func createBundle(w walker, m *manifest, dest, rootfs string) error { | |||
| if err2 := os.MkdirAll(dest, 0755); err2 != nil { | |||
| return err2 | |||
| } | |||
| defer func() { | |||
| if retErr != nil { | |||
There was a problem hiding this comment.
Can we use err instead of retErr. Otherwise I expect this will always be nil (unless we change to set retErr in the body of createBundle). Or maybe Go does something magic with named returns to set them up before defers fire?
There was a problem hiding this comment.
when return ,go will assign the return value to retErr
|
LGTM |
1 similar comment
|
LGTM |
If any error occurs, we should cleanup the bundle we created. If not, this may confuse users.
Signed-off-by: Ma Shimiao [email protected]