diff mbox series

[V1,01/26] oslib: qemu_clear_cloexec

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

Commit Message

Steven Sistare April 29, 2024, 3:55 p.m. UTC
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>
---
 include/qemu/osdep.h | 9 +++++++++
 util/oslib-posix.c   | 9 +++++++++
 util/oslib-win32.c   | 4 ++++
 3 files changed, 22 insertions(+)

Comments

Fabiano Rosas May 6, 2024, 11:27 p.m. UTC | #1
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();
Daniel P. Berrangé May 7, 2024, 8:56 a.m. UTC | #2
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
Fabiano Rosas May 7, 2024, 1:54 p.m. UTC | #3
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 mbox series

Patch

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();