| Message ID | 1714406135-451286-2-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live update: cpr-exec | expand |
Steve Sistare <steven.sistare@oracle.com> writes: +cc dgilbert, marcandre > Define qemu_clear_cloexec, analogous to qemu_set_cloexec. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> A v1 patch with two reviews already, from people from another company and they're not in CC. Looks suspicious. =) Here's a fresh one, hopefully it won't spend another 4 years in the drawer: Reviewed-by: Fabiano Rosas <farosas@suse.de> > --- > include/qemu/osdep.h | 9 +++++++++ > util/oslib-posix.c | 9 +++++++++ > util/oslib-win32.c | 4 ++++ > 3 files changed, 22 insertions(+) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index c7053cd..b58f312 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -660,6 +660,15 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count) > > void qemu_set_cloexec(int fd); > > +/* > + * Clear FD_CLOEXEC for a descriptor. > + * > + * The caller must guarantee that no other fork+exec's occur before the > + * exec that is intended to inherit this descriptor, eg by suspending CPUs > + * and blocking monitor commands. > + */ > +void qemu_clear_cloexec(int fd); > + > /* Return a dynamically allocated directory path that is appropriate for storing > * local state. > * > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index e764416..614c3e5 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -272,6 +272,15 @@ int qemu_socketpair(int domain, int type, int protocol, int sv[2]) > return ret; > } > > +void qemu_clear_cloexec(int fd) > +{ > + int f; > + f = fcntl(fd, F_GETFD); > + assert(f != -1); > + f = fcntl(fd, F_SETFD, f & ~FD_CLOEXEC); > + assert(f != -1); > +} > + > char * > qemu_get_local_state_dir(void) > { > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index b623830..c3e969a 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -222,6 +222,10 @@ void qemu_set_cloexec(int fd) > { > } > > +void qemu_clear_cloexec(int fd) > +{ > +} > + > int qemu_get_thread_id(void) > { > return GetCurrentThreadId();
On Mon, May 06, 2024 at 08:27:15PM -0300, Fabiano Rosas wrote: > Steve Sistare <steven.sistare@oracle.com> writes: > > +cc dgilbert, marcandre > > > Define qemu_clear_cloexec, analogous to qemu_set_cloexec. > > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > A v1 patch with two reviews already, from people from another company > and they're not in CC. Looks suspicious. =) It is ok in this case - the cpr work has been going on a long time and the original series that got partial reviews has been split up somewhat. So its "v1" of this series of patches, but not "v1" of what we've seen posted on qemu-devel in the past > > Here's a fresh one, hopefully it won't spend another 4 years in the > drawer: > > Reviewed-by: Fabiano Rosas <farosas@suse.de> > > > --- > > include/qemu/osdep.h | 9 +++++++++ > > util/oslib-posix.c | 9 +++++++++ > > util/oslib-win32.c | 4 ++++ > > 3 files changed, 22 insertions(+) > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > index c7053cd..b58f312 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -660,6 +660,15 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count) > > > > void qemu_set_cloexec(int fd); > > > > +/* > > + * Clear FD_CLOEXEC for a descriptor. > > + * > > + * The caller must guarantee that no other fork+exec's occur before the > > + * exec that is intended to inherit this descriptor, eg by suspending CPUs > > + * and blocking monitor commands. > > + */ > > +void qemu_clear_cloexec(int fd); > > + > > /* Return a dynamically allocated directory path that is appropriate for storing > > * local state. > > * > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > > index e764416..614c3e5 100644 > > --- a/util/oslib-posix.c > > +++ b/util/oslib-posix.c > > @@ -272,6 +272,15 @@ int qemu_socketpair(int domain, int type, int protocol, int sv[2]) > > return ret; > > } > > > > +void qemu_clear_cloexec(int fd) > > +{ > > + int f; > > + f = fcntl(fd, F_GETFD); > > + assert(f != -1); > > + f = fcntl(fd, F_SETFD, f & ~FD_CLOEXEC); > > + assert(f != -1); > > +} > > + > > char * > > qemu_get_local_state_dir(void) > > { > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > > index b623830..c3e969a 100644 > > --- a/util/oslib-win32.c > > +++ b/util/oslib-win32.c > > @@ -222,6 +222,10 @@ void qemu_set_cloexec(int fd) > > { > > } > > > > +void qemu_clear_cloexec(int fd) > > +{ > > +} > > + > > int qemu_get_thread_id(void) > > { > > return GetCurrentThreadId(); > With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, May 06, 2024 at 08:27:15PM -0300, Fabiano Rosas wrote: >> Steve Sistare <steven.sistare@oracle.com> writes: >> >> +cc dgilbert, marcandre >> >> > Define qemu_clear_cloexec, analogous to qemu_set_cloexec. >> > >> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> A v1 patch with two reviews already, from people from another company >> and they're not in CC. Looks suspicious. =) > > It is ok in this case - the cpr work has been going on a long > time and the original series that got partial reviews has been > split up somewhat. So its "v1" of this series of patches, but > not "v1" of what we've seen posted on qemu-devel in the past I know =) I searched the archives to make sure those r-bs were actually provided by them and I also remember the series from back then. On that topic, but not related to this patch at all, I would prefer if we had a no-preexisting r-bs rule. I don't see any value in an r-b that already comes present in v1 and has not been provided through the list. There's the obvious concern about bad faith, but also that we might lose track of a series and maintainers/tools might take those r-bs as a sign of the code actually being reviewed properly (which may or may not be true). In the case people develop a series inside the company and then post to the list, an sob seems to be adequate enough. > >> >> Here's a fresh one, hopefully it won't spend another 4 years in the >> drawer: >> >> Reviewed-by: Fabiano Rosas <farosas@suse.de> >> >> > --- >> > include/qemu/osdep.h | 9 +++++++++ >> > util/oslib-posix.c | 9 +++++++++ >> > util/oslib-win32.c | 4 ++++ >> > 3 files changed, 22 insertions(+) >> > >> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h >> > index c7053cd..b58f312 100644 >> > --- a/include/qemu/osdep.h >> > +++ b/include/qemu/osdep.h >> > @@ -660,6 +660,15 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count) >> > >> > void qemu_set_cloexec(int fd); >> > >> > +/* >> > + * Clear FD_CLOEXEC for a descriptor. >> > + * >> > + * The caller must guarantee that no other fork+exec's occur before the >> > + * exec that is intended to inherit this descriptor, eg by suspending CPUs >> > + * and blocking monitor commands. >> > + */ >> > +void qemu_clear_cloexec(int fd); >> > + >> > /* Return a dynamically allocated directory path that is appropriate for storing >> > * local state. >> > * >> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> > index e764416..614c3e5 100644 >> > --- a/util/oslib-posix.c >> > +++ b/util/oslib-posix.c >> > @@ -272,6 +272,15 @@ int qemu_socketpair(int domain, int type, int protocol, int sv[2]) >> > return ret; >> > } >> > >> > +void qemu_clear_cloexec(int fd) >> > +{ >> > + int f; >> > + f = fcntl(fd, F_GETFD); >> > + assert(f != -1); >> > + f = fcntl(fd, F_SETFD, f & ~FD_CLOEXEC); >> > + assert(f != -1); >> > +} >> > + >> > char * >> > qemu_get_local_state_dir(void) >> > { >> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c >> > index b623830..c3e969a 100644 >> > --- a/util/oslib-win32.c >> > +++ b/util/oslib-win32.c >> > @@ -222,6 +222,10 @@ void qemu_set_cloexec(int fd) >> > { >> > } >> > >> > +void qemu_clear_cloexec(int fd) >> > +{ >> > +} >> > + >> > int qemu_get_thread_id(void) >> > { >> > return GetCurrentThreadId(); >> > > With regards, > Daniel
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index c7053cd..b58f312 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -660,6 +660,15 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count) void qemu_set_cloexec(int fd); +/* + * Clear FD_CLOEXEC for a descriptor. + * + * The caller must guarantee that no other fork+exec's occur before the + * exec that is intended to inherit this descriptor, eg by suspending CPUs + * and blocking monitor commands. + */ +void qemu_clear_cloexec(int fd); + /* Return a dynamically allocated directory path that is appropriate for storing * local state. * diff --git a/util/oslib-posix.c b/util/oslib-posix.c index e764416..614c3e5 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -272,6 +272,15 @@ int qemu_socketpair(int domain, int type, int protocol, int sv[2]) return ret; } +void qemu_clear_cloexec(int fd) +{ + int f; + f = fcntl(fd, F_GETFD); + assert(f != -1); + f = fcntl(fd, F_SETFD, f & ~FD_CLOEXEC); + assert(f != -1); +} + char * qemu_get_local_state_dir(void) { diff --git a/util/oslib-win32.c b/util/oslib-win32.c index b623830..c3e969a 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -222,6 +222,10 @@ void qemu_set_cloexec(int fd) { } +void qemu_clear_cloexec(int fd) +{ +} + int qemu_get_thread_id(void) { return GetCurrentThreadId();