1. Patchew
  2. QEMU
  3. [PATCH V8 00/39] Live Update
  4. View patch

[PATCH V8 13/39] oslib: qemu_clear_cloexec

Steve Sistare posted 39 patches 3 years, 7 months ago
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Alex Williamson <alex.williamson@redhat.com>, Steve Sistare <steven.sistare@oracle.com>, Mark Kanda <mark.kanda@oracle.com>, Peter Xu <peterx@redhat.com>, Juan Quintela <quintela@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <michael.roth@amd.com>, John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>, Beraldo Leal <bleal@redhat.com>, Eric Blake <eblake@redhat.com>, Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Stefan Weil <sw@weilnetz.de>
There is a newer version of this series
[PATCH V8 13/39] oslib: qemu_clear_cloexec
Posted by Steve Sistare 3 years, 7 months ago 
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);
 
 /* 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 7a34c16..421e987 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -261,6 +261,15 @@ void qemu_set_cloexec(int fd)
     assert(f != -1);
 }
 
+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 5723d3e..5bed148 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -226,6 +226,10 @@ void qemu_set_cloexec(int fd)
 {
 }
 
+void qemu_clear_cloexec(int fd)
+{
+}
+
 int qemu_get_thread_id(void)
 {
     return GetCurrentThreadId();
-- 
1.8.3.1
Re: [PATCH V8 13/39] oslib: qemu_clear_cloexec
Posted by Daniel P. Berrangé 3 years, 7 months ago 
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 :|
Re: [PATCH V8 13/39] oslib: qemu_clear_cloexec
Posted by Steven Sistare 3 years, 7 months ago 
On 6/16/2022 12:07 PM, Daniel P. Berrangé wrote:
> 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

Sure thing, will do - Steve
Re: [PATCH V8 13/39] oslib: qemu_clear_cloexec
Posted by Marc-André Lureau 3 years, 7 months ago 
On Wed, Jun 15, 2022 at 7:01 PM Steve Sistare <steven.sistare@oracle.com>
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>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.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);
>
>  /* 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 7a34c16..421e987 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -261,6 +261,15 @@ void qemu_set_cloexec(int fd)
>      assert(f != -1);
>  }
>
> +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 5723d3e..5bed148 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -226,6 +226,10 @@ void qemu_set_cloexec(int fd)
>  {
>  }
>
> +void qemu_clear_cloexec(int fd)
> +{
> +}
> +
>  int qemu_get_thread_id(void)
>  {
>      return GetCurrentThreadId();
> --
> 1.8.3.1
>
>
>

-- 
Marc-André Lureau

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.