fix: miss remove temp file in createSnapshot#2900
fix: miss remove temp file in createSnapshot#2900crosbymichael merged 1 commit intocontainerd:masterfrom
Conversation
```
func foo() error {
defer func() {
if err != nil {
...
}
}()
...
}
```
use defer func to do something when err not nil, if foo() not use
named error, `err != nil` can not catch all errors, since when err
re-defined in if condition, it is a new variable.
Signed-off-by: Ace-Tang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2900 +/- ##
==========================================
- Coverage 43.89% 43.89% -0.01%
==========================================
Files 101 101
Lines 10800 10799 -1
==========================================
- Hits 4741 4740 -1
Misses 5328 5328
Partials 731 731
Continue to review full report at Codecov.
|
| return cleanup, nil | ||
| } | ||
|
|
||
| func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, key, parent string, opts []snapshots.Opt) ([]mount.Mount, error) { |
There was a problem hiding this comment.
Can you point out the branches which are not getting cleaned up correctly because of this? The block from line 398 seems to have redeclared err.
There was a problem hiding this comment.
Yes, the line 399(https://github.com/containerd/containerd/pull/2900/files#diff-adfa4892be9b7d1d353e8eba2cd335d6L399) and line 406 (https://github.com/containerd/containerd/pull/2900/files#diff-adfa4892be9b7d1d353e8eba2cd335d6L406) has new defined err, but this part code always good.
I found the problem when I implement a new snapshotter qcow2 for kata runtime, I re-use lots of overlay code , in debug process, found many uncleaned tmp snapshot directory like
$ sudo ls /var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots
1 11 14 7 new-888620822
There was a problem hiding this comment.
My only concern was previously it required a very explicit setting of err to hit the cleanup section, but I don't see a code path which doesn't expect this block to run. So looks good to have that as the default behavior
|
LGTM |
5 participants
use defer func to do something when err not nil, if foo() not use
named error,
err != nilcan not catch all errors, since when errre-defined in if condition, it is a new variable.
Signed-off-by: Ace-Tang [email protected]