We lose O_SYNC (for both functions) with this change. https://cs.opensource.google/go/go/+/refs/tags/go1.20.1:src/os/tempfile.go;l=44

use f.Sync() resolve

Sorry. I don't get your point here. The tempPath has temp prefix, but it doesn't mean it is named randomly.
The WriteAddress is to create temp file named .X and then rename it to X, just in case that the X content is corrupted after the node's power outage or out of disk space.

I can't tell any optimization from your patch. The original OpenFile is very straightforward. But this patch will pick up a unused name and open it with 0600 permission. And then it files other syscall to chmod to 0666....
One syscall -> More than two syscalls.

Please point it out if I miss somethings. Thanks

Oop, I never submitted my review besides my stray comment. I mostly have the same feedback here, I don't get what this is optimizing. Do you have numbers to share/could you put in the description what the optimization is?

1. use os.CreateTemp() to ensure that the temporary file name does not conflict with the existing file name, more secure and reliable

2. use f.Write() to write the pid instead of fmt.Fprintf() because the former does not require string formatting and is therefore faster

3. use defer f.Close() to guarantee that the file will be closed if opened in any case

Thanks for the quick reply.

Basically, if Write{PidFile,Address} fails, it won't retry and the bundle will be deleted. So, it won't be conflict issue.

For the part 2, it can be accepted. Thanks

And for part 3 , it's based on the statements. Since the Close after write, it is safe (at least to me).

My idea is that temporary data may be written to an existing file, resulting in the original file data being coverage, although the probability is not high.

It's based on the usecase. In shim, the temporary file is .X. The prefix . means temporary.
But in common case, your change is good. We've already provided it as AtomicWriteFile. I think we can handle the close in defer.

https://github.com/containerd/continuity/blob/v0.3.0/ioutils.go#L28

got it
(Sorry, I am a containerd novice and do not know this. I am currently learning containerd. I hope I can participate in community development through some of my own ideas.😀

Neven mind! Welcome to contribute to container project. The code review can recall memory for the code. :p

Sorry for the unclear comment in #8151 (comment)

I don't want to change the original atomic write here, because we don't need random filename here and it won't be called twice for a container.

But we can use WriteString(strconv.Itoa(pid)) here.

got it, as soon as I can

@helen-frank: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Just want to ask a question, what thresholds do you need to become a member of the containerd community? @fuweid
( I may have been studying containerd recently, so I may make some contributions and hope to pass the online test faster.😀

please checkout this one https://github.com/containerd/project/blob/main/GOVERNANCE.md#adding-maintainers~

got it, It seems that I still have a long way to go.😀

I need ok-to-test @fuweid

Why not just have an f.Close() before this line, adding a defer and extra variable increases code complexity just to handle a single condition. I don't think there is ever a need to add more logic in between the write and close that would necessitate using the defer pattern.

+1. Since we need to call f.Close() anyway, why don't we keep it the same as before:

_, err = f.WriteString(strconv.Itoa(pid))
closeErr := f.Close()
if err != nil {
  return err
}
if closeErr != nil {
  return closeErr
}

closeErr is only added to align with the behavior of this PR. We don't even need it if we want to keep the behavior the same as the original code (i.e. ignoring the error from f.Close()).

got it
(Sorry, there are some other things being dealt with recently.)

nice. I didn't know this was faster than fmt.Sprintf