Re: [PATCH V1 01/26] oslib: qemu_clear_cloexec

[Fabiano Rosas <farosas@suse.de>]

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