| Message ID | 20180320173324.30844-1-fl@n621.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] os: truncate pidfile on creation | expand |
On 03/20/2018 12:33 PM, Florian Larysch wrote: > qemu_create_pidfile does not truncate the pidfile when it creates it, > but rather overwrites its contents with the new pid. This works fine as > long as the length of the pid doesn't decrease, but this might happen in > case of wraparounds, causing pidfiles to contain trailing garbage which > breaks operations such as 'kill $(cat pidfile)'. > > Instead, always truncate the file before writing it. > > Note that the order is important here: We cannot simply use O_TRUNC in > the open() call because another qemu process might truncate the pidfile > of a process that is still running before reaching the lockf() barrier. > > The Windows version suffers from a similar problem, but as it does not > provide effective mutual exclusion anyway (because the file handle is > closed immediately after writing to it), adopting this behavior still > seems to be an improvement, as it at least prevents garbled pidfiles. > > Signed-off-by: Florian Larysch <fl@n621.de> > --- Here after the --- is a nice place to summarize how v3 differs from v2, to save reviewers some time. In this particular case, it looks like all you did was correct a commit message typo in v2; at which point, since I gave Reviewed-by on v2, you could have pasted that into your amended v3 commit message to make it obvious that you didn't change the code. At any rate, my R-b still stands. Also, if all that is wrong is a typo in the commit message, a maintainer is often willing to fix that up without you having to send a new revision, especially since the maintainer has to edit the commit message anyways to add their Signed-off-by. But you are also correct that a new revision CAN save a maintainer time, in part because adding S-o-b can be done by machine alone, so it is not a worthless exercise on your part to send a respin just for typos. > os-posix.c | 6 ++++++ > os-win32.c | 2 +- > 2 files changed, 7 insertions(+), 1 deletion(-) >
On Tue, Mar 20, 2018 at 01:00:40PM -0500, Eric Blake wrote: > Here after the --- is a nice place to summarize how v3 differs from > v2, to save reviewers some time. The triviality of the change didn't seem to warrant that, but in retrospect, I realize that searching for the sole trivial change when reviewing a patch is also (or even more of) a burden. I'll keep that in mind for the future. > Also, if all that is wrong is a typo in the commit message, a > maintainer is often willing to fix that up without you having to > send a new revision I interpreted your comment on the v2 as a request for me to fix that. Sorry for the churn. Florian
On 03/20/2018 01:11 PM, Florian Larysch wrote: > On Tue, Mar 20, 2018 at 01:00:40PM -0500, Eric Blake wrote: >> Here after the --- is a nice place to summarize how v3 differs from >> v2, to save reviewers some time. > > The triviality of the change didn't seem to warrant that, but in > retrospect, I realize that searching for the sole trivial change when > reviewing a patch is also (or even more of) a burden. I'll keep that in > mind for the future. > >> Also, if all that is wrong is a typo in the commit message, a >> maintainer is often willing to fix that up without you having to >> send a new revision > > I interpreted your comment on the v2 as a request for me to fix that. > Sorry for the churn. No problem - it's okay to learn as you go (we were all once new contributors), and some things get easier the longer you've been on the list to see the balance between saving yourself time and saving reviewers/maintainers time.
diff --git a/os-posix.c b/os-posix.c index b9c2343b1e..f2318aef55 100644 --- a/os-posix.c +++ b/os-posix.c @@ -309,6 +309,12 @@ int qemu_create_pidfile(const char *filename) close(fd); return -1; } + + if (ftruncate(fd, 0)) { + close(fd); + return -1; + } + len = snprintf(buffer, sizeof(buffer), FMT_pid "\n", getpid()); if (write(fd, buffer, len) != len) { close(fd); diff --git a/os-win32.c b/os-win32.c index 586a7c7d49..85dbad7af8 100644 --- a/os-win32.c +++ b/os-win32.c @@ -108,7 +108,7 @@ int qemu_create_pidfile(const char *filename) memset(&overlap, 0, sizeof(overlap)); file = CreateFile(filename, GENERIC_WRITE, FILE_SHARE_READ, NULL, - OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); + CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); if (file == INVALID_HANDLE_VALUE) { return -1;
qemu_create_pidfile does not truncate the pidfile when it creates it, but rather overwrites its contents with the new pid. This works fine as long as the length of the pid doesn't decrease, but this might happen in case of wraparounds, causing pidfiles to contain trailing garbage which breaks operations such as 'kill $(cat pidfile)'. Instead, always truncate the file before writing it. Note that the order is important here: We cannot simply use O_TRUNC in the open() call because another qemu process might truncate the pidfile of a process that is still running before reaching the lockf() barrier. The Windows version suffers from a similar problem, but as it does not provide effective mutual exclusion anyway (because the file handle is closed immediately after writing to it), adopting this behavior still seems to be an improvement, as it at least prevents garbled pidfiles. Signed-off-by: Florian Larysch <fl@n621.de> --- os-posix.c | 6 ++++++ os-win32.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)