Close pipe in overlay2 graphdriver

[keloyang]

If cmd.Start() fail in mountFrom, we will have pipe not closed.

Reported-by:Qiang Ning ningqiang1@huawei.com
Signed-off-by: Shukui Yang yangshukui@huawei.com

[thaJeztah]

LGTM see #34863 (comment)

ping @dmcgowan

Also, should this be cherry-picked in a release?

[yongtang]

LGTM

[vieux]

ping @kolyshkin

[kolyshkin]

LGTM

[dmcgowan]

The close was explicitly called before Wait to ensure that the process is not waiting on additional input.

[thaJeztah]

In that case, should a w.Close() be put inside the err ifs?

[thaJeztah]

And probably a comment added to prevent someone from changing to a defer

[dmcgowan]

@thaJeztah I think it is more a matter of style, I prefer to have w.Close inside the if statements but calling w.Close multiple times (one before Wait, and one on defer) is not a problem.

Dismissing per #34863 (comment)

[yongtang]

My fault. Didn't realized w.Close() has to be done before the Wait().

@keloyang Can you update the PR based on @dmcgowan 's comment?

[kolyshkin]

So, before putting my LGTM I wrote a test program to check whether json decoder would wait on input if it's not closed. Apparently as soon as json data goes through it proceeds even if stdin is still open.

[keloyang]

@dmcgowan @kolyshkin @thaJeztah @yongtang @vieux thanks for your reviews.

I see go's code for cmd.Wait(), I think process will not block if reexec process exit, becuase fd.Close() will not block.

func (c *Cmd) Wait() 
	c.Process.Wait()
		syscall.Wait4
	c.closeDescriptors(c.closeAfterWait)
	   fd.Close()

I also have the following code to test,

//add to mount.go
//func MountFrom(dir, device, target, mType string, flags uintptr, label string) error {
//	return mountFrom(dir, device, target, mType, flags, label)
//}

package main

import (
	"fmt"
	"os"
	
	"github.com/docker/docker/pkg/reexec"
	"github.com/docker/docker/daemon/graphdriver/overlay2"
)

func main(){
	if reexec.Init() {
		return
	}
	
	dir, _:=  os.Getwd()
	if err := overlay2.MountFrom(dir, "overlay", "merged", "overlay", 
				0, "lowerdir=lower,upperdir=upper,workdir=work"); err != nil {
		fmt.Printf("overlay mount to [%v]\n", err)
	}
}

the output:

[root@localhost test]# ls 
lower  merged  mountFrom  mountFrom.go  upper  work
[root@localhost test]# ls merged/
[root@localhost test]# time ./mountFrom

real    0m0.013s
user    0m0.006s
sys     0m0.010s
[root@localhost test]# ls merged/
a

So I agree with @kolyshkin, can we still use defer close ,WDYT ?

[kolyshkin]

It works without explicit Close(), as json.Decode() finishes as long as the document is complete (last closing bracket is received). Still, an extra Close() before Wait() looks cleaner to me.

[keloyang]

ping @dmcgowan @kolyshkin @thaJeztah @yongtang @vieux

[thaJeztah]

LGTM

[dmcgowan]

LGTM

[yongtang]

LGTM, thanks!

[thaJeztah]

all green; merging

[thaJeztah]

LOL looks like we pressed the button at the same time

[yongtang]

😄 👍 🍺