Re: [PATCH V8 13/39] oslib: qemu_clear_cloexec

[Daniel P . Berrangé]

On Wed, Jun 15, 2022 at 07:52:00AM -0700, Steve Sistare wrote:
> Define qemu_clear_cloexec, analogous to qemu_set_cloexec.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  include/qemu/osdep.h | 1 +
>  util/oslib-posix.c   | 9 +++++++++
>  util/oslib-win32.c   | 4 ++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b1c161c..e916f3b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -548,6 +548,7 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t 
> count)
>      G_GNUC_WARN_UNUSED_RESULT;
>  
>  void qemu_set_cloexec(int fd);
> +void qemu_clear_cloexec(int fd);

I'm a little wary of adding this helper without any accompanying
comment.

It is almost never correct to use this new method in a threaded
program like QEMU, unless you have strong confidence that all
the other threads are idle and not liable to perform a fork+exec
for any other reason.

IIUC, this can be satisfied by the CPR code because it will be
used only immediately before exec'ing the updated QEMU binary,
and it has suspended any other CPUs and not other monitor
commands are concurrently running.

IOW, I just ask that you put a comment with a big warning that
essentially no one should use this method, except CPR code.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|