1. Patchew
  2. QEMU
  3. [PATCH v3 00/15] Misc cleanups
  4. View patch

[PATCH v3 05/15] osdep: export qemu_open_cloexec()

marcandre.lureau@redhat.com posted 15 patches 3 years, 9 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>, Alexander Bulekov <alxndr@bu.edu>, Paolo Bonzini <pbonzini@redhat.com>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Thomas Huth <thuth@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>, Laurent Vivier <lvivier@redhat.com>, Stefan Weil <sw@weilnetz.de>
There is a newer version of this series
[PATCH v3 05/15] osdep: export qemu_open_cloexec()
Posted by marcandre.lureau@redhat.com 3 years, 9 months ago 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Used in the next patch, to simplify qga code.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 67cc465416..64f51cfb7a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
  */
 int qemu_open_old(const char *name, int flags, ...);
 int qemu_open(const char *name, int flags, Error **errp);
+int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
diff --git a/util/osdep.c b/util/osdep.c
index 60fcbbaebe..545a88e1fd 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
 }
 #endif
 
-static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
+int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
 {
+    ERRP_GUARD();
     int ret;
+
 #ifdef O_CLOEXEC
     ret = open(name, flags | O_CLOEXEC, mode);
 #else
@@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
         qemu_set_cloexec(ret);
     }
 #endif
+    if (ret == -1) {
+        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
+    }
+
     return ret;
 }
 
@@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
     }
 #endif
 
-    ret = qemu_open_cloexec(name, flags, mode);
+    ret = qemu_open_cloexec(name, flags, mode, NULL);
 
     if (ret == -1) {
         const char *action = flags & O_CREAT ? "create" : "open";
-- 
2.36.1
Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
Posted by Daniel P. Berrangé 3 years, 8 months ago 
On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Used in the next patch, to simplify qga code.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 67cc465416..64f51cfb7a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
>   */
>  int qemu_open_old(const char *name, int flags, ...);
>  int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);

I don't think we should be exporting this - it is just a variant of the
'qemu_open_old' method that we wanted callers to stop using in favour
of explicitly deciding between 'qemu_open' and 'qemu_create'.

>  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
> diff --git a/util/osdep.c b/util/osdep.c
> index 60fcbbaebe..545a88e1fd 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>  }
>  #endif
>  
> -static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
>  {
> +    ERRP_GUARD();
>      int ret;
> +
>  #ifdef O_CLOEXEC
>      ret = open(name, flags | O_CLOEXEC, mode);
>  #else
> @@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
>          qemu_set_cloexec(ret);
>      }
>  #endif
> +    if (ret == -1) {
> +        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
> +    }
> +

This will mean that qemu_open_internal() caller will now be overwriting
an existing error message.

>      return ret;
>  }
>  
> @@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>      }
>  #endif
>  
> -    ret = qemu_open_cloexec(name, flags, mode);
> +    ret = qemu_open_cloexec(name, flags, mode, NULL);
>  
>      if (ret == -1) {
>          const char *action = flags & O_CREAT ? "create" : "open";
> -- 
> 2.36.1
> 
> 

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 v3 05/15] osdep: export qemu_open_cloexec()
Posted by Marc-André Lureau 3 years, 8 months ago 
Hi

On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Used in the next patch, to simplify qga code.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/qemu/osdep.h |  1 +
> >  util/osdep.c         | 10 ++++++++--
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 67cc465416..64f51cfb7a 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> >   */
> >  int qemu_open_old(const char *name, int flags, ...);
> >  int qemu_open(const char *name, int flags, Error **errp);
> > +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error
> **errp);
>
> I don't think we should be exporting this - it is just a variant of the
> 'qemu_open_old' method that we wanted callers to stop using in favour
> of explicitly deciding between 'qemu_open' and 'qemu_create'.
>


qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools don't
need.

(qemu_open_old() doesn't set CLOEXEC)


>
> >  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
> >  int qemu_close(int fd);
> >  int qemu_unlink(const char *name);
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 60fcbbaebe..545a88e1fd 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start,
> int64_t len, bool exclusive)
> >  }
> >  #endif
> >
> > -static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> > +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error
> **errp)
> >  {
> > +    ERRP_GUARD();
> >      int ret;
> > +
> >  #ifdef O_CLOEXEC
> >      ret = open(name, flags | O_CLOEXEC, mode);
> >  #else
> > @@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int
> flags, mode_t mode)
> >          qemu_set_cloexec(ret);
> >      }
> >  #endif
> > +    if (ret == -1) {
> > +        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
> > +    }
> > +
>
> This will mean that qemu_open_internal() caller will now be overwriting
> an existing error message.
>

NULL is passed as errp argument of qemu_open_cloexec() in
qemu_open_internal().


>
> >      return ret;
> >  }
> >
> > @@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags,
> mode_t mode, Error **errp)
> >      }
> >  #endif
> >
> > -    ret = qemu_open_cloexec(name, flags, mode);
> > +    ret = qemu_open_cloexec(name, flags, mode, NULL);
> >
>

here


> >      if (ret == -1) {
> >          const char *action = flags & O_CREAT ? "create" : "open";
> > --
> > 2.36.1
> >
> >
>
> 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 :|
>
>
>

-- 
Marc-André Lureau
Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
Posted by Daniel P. Berrangé 3 years, 8 months ago 
On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> > wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Used in the next patch, to simplify qga code.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  include/qemu/osdep.h |  1 +
> > >  util/osdep.c         | 10 ++++++++--
> > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 67cc465416..64f51cfb7a 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> > >   */
> > >  int qemu_open_old(const char *name, int flags, ...);
> > >  int qemu_open(const char *name, int flags, Error **errp);
> > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error
> > **errp);
> >
> > I don't think we should be exporting this - it is just a variant of the
> > 'qemu_open_old' method that we wanted callers to stop using in favour
> > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> >
> 
> 
> qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools don't
> need.

Right, but exporting this as 'qemu_open_cloexec' is going to mislead
people into thinking it is a better version of 'qemu_open'. This will
cause us to loose support for /dev/fdset in places where we actually
need it.

It is pretty harmless to have /dev/fdset there, even if the tool does
not need it - that's been the case with many QEMU tools for many years.
If we think it is actually a real problem though, we should just have
a way to toggle it on/off from the existing APIs.

eg put  'bool allow_fdset = true"   in softmmu/vl.c, and
'bool allow_fdset = false' in stubs/open.c, and then make
qemu_open_internal conditionalize itself on this global 
variable, so only the system emulators get fdset support
activated.

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 v3 05/15] osdep: export qemu_open_cloexec()
Posted by Marc-André Lureau 3 years, 8 months ago 
Hi

On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > Used in the next patch, to simplify qga code.
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >  include/qemu/osdep.h |  1 +
> > > >  util/osdep.c         | 10 ++++++++--
> > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > index 67cc465416..64f51cfb7a 100644
> > > > --- a/include/qemu/osdep.h
> > > > +++ b/include/qemu/osdep.h
> > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> > > >   */
> > > >  int qemu_open_old(const char *name, int flags, ...);
> > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> Error
> > > **errp);
> > >
> > > I don't think we should be exporting this - it is just a variant of the
> > > 'qemu_open_old' method that we wanted callers to stop using in favour
> > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > >
> >
> >
> > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> don't
> > need.
>
> Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> people into thinking it is a better version of 'qemu_open'. This will
> cause us to loose support for /dev/fdset in places where we actually
> need it.
>

> It is pretty harmless to have /dev/fdset there, even if the tool does
> not need it - that's been the case with many QEMU tools for many years.
> If we think it is actually a real problem though, we should just have
> a way to toggle it on/off from the existing APIs.
>
>
It's a bit problematic to make qemu-ga standalone, and have a common shared
subproject/library.

Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any better
idea ?

eg put  'bool allow_fdset = true"   in softmmu/vl.c, and
> 'bool allow_fdset = false' in stubs/open.c, and then make
> qemu_open_internal conditionalize itself on this global
> variable, so only the system emulators get fdset support
> activated.
>
> 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 :|
>
>

-- 
Marc-André Lureau
Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
Posted by Daniel P. Berrangé 3 years, 8 months ago 
On Mon, May 23, 2022 at 08:02:45PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com
> > > > wrote:
> > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > >
> > > > > Used in the next patch, to simplify qga code.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > ---
> > > > >  include/qemu/osdep.h |  1 +
> > > > >  util/osdep.c         | 10 ++++++++--
> > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > index 67cc465416..64f51cfb7a 100644
> > > > > --- a/include/qemu/osdep.h
> > > > > +++ b/include/qemu/osdep.h
> > > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
> > > > >   */
> > > > >  int qemu_open_old(const char *name, int flags, ...);
> > > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> > Error
> > > > **errp);
> > > >
> > > > I don't think we should be exporting this - it is just a variant of the
> > > > 'qemu_open_old' method that we wanted callers to stop using in favour
> > > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > > >
> > >
> > >
> > > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> > don't
> > > need.
> >
> > Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> > people into thinking it is a better version of 'qemu_open'. This will
> > cause us to loose support for /dev/fdset in places where we actually
> > need it.
> >
> 
> > It is pretty harmless to have /dev/fdset there, even if the tool does
> > not need it - that's been the case with many QEMU tools for many years.
> > If we think it is actually a real problem though, we should just have
> > a way to toggle it on/off from the existing APIs.
> >
> >
> It's a bit problematic to make qemu-ga standalone, and have a common shared
> subproject/library.
> 
> Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any better
> idea ?

If we want to make qemu-ga standalone, then IMHO we should be
aggressively switching it to use as many GLib APIs as possible,
eliminating its reliance on any of QEMU's home-grown portability
functions. All the 'FILE *' / 'open' scenarios could be replaced
with GIO's GFile/GInputStream/GOutputStream for example.

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 v3 05/15] osdep: export qemu_open_cloexec()
Posted by Marc-André Lureau 3 years, 8 months ago 
Hi

On Mon, May 23, 2022 at 8:11 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, May 23, 2022 at 08:02:45PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > > > Hi
> > > >
> > > > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <
> berrange@redhat.com>
> > > > wrote:
> > > >
> > > > > On Fri, May 13, 2022 at 08:08:11PM +0200,
> marcandre.lureau@redhat.com
> > > > > wrote:
> > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > >
> > > > > > Used in the next patch, to simplify qga code.
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > >  include/qemu/osdep.h |  1 +
> > > > > >  util/osdep.c         | 10 ++++++++--
> > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > > index 67cc465416..64f51cfb7a 100644
> > > > > > --- a/include/qemu/osdep.h
> > > > > > +++ b/include/qemu/osdep.h
> > > > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction
> *action,
> > > > > >   */
> > > > > >  int qemu_open_old(const char *name, int flags, ...);
> > > > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> > > Error
> > > > > **errp);
> > > > >
> > > > > I don't think we should be exporting this - it is just a variant
> of the
> > > > > 'qemu_open_old' method that we wanted callers to stop using in
> favour
> > > > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > > > >
> > > >
> > > >
> > > > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> > > don't
> > > > need.
> > >
> > > Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> > > people into thinking it is a better version of 'qemu_open'. This will
> > > cause us to loose support for /dev/fdset in places where we actually
> > > need it.
> > >
> >
> > > It is pretty harmless to have /dev/fdset there, even if the tool does
> > > not need it - that's been the case with many QEMU tools for many years.
> > > If we think it is actually a real problem though, we should just have
> > > a way to toggle it on/off from the existing APIs.
> > >
> > >
> > It's a bit problematic to make qemu-ga standalone, and have a common
> shared
> > subproject/library.
> >
> > Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any
> better
> > idea ?
>
> If we want to make qemu-ga standalone, then IMHO we should be
> aggressively switching it to use as many GLib APIs as possible,
> eliminating its reliance on any of QEMU's home-grown portability
> functions. All the 'FILE *' / 'open' scenarios could be replaced
> with GIO's GFile/GInputStream/GOutputStream for example.
>

I am not too eager to do that kind of refactoring. Even rewriting in Rust
seems a bit pointless to me, even if I would have more motivation.

Also there are times you do open() for things that are not stream-related.
And glib sadly doesn't really offer a solution for open(CLOEXEC).

I guess I can simply add an open_cloexec() helper function in qemu-ga alone
for now.
Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
Posted by Peter Maydell 3 years, 8 months ago 
On Fri, 13 May 2022 at 19:16, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Used in the next patch, to simplify qga code.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 67cc465416..64f51cfb7a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
>   */
>  int qemu_open_old(const char *name, int flags, ...);
>  int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);

Where we make a function global and put it in a header
we should generally add a documentation comment describing it,
please.

thanks
-- PMM
Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec()
Posted by Markus Armbruster 3 years, 8 months ago 
marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Used in the next patch, to simplify qga code.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

The commit mesage makes me expect a change to external linkage plus a
declaration in the header, but ...

> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 67cc465416..64f51cfb7a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
>   */
>  int qemu_open_old(const char *name, int flags, ...);
>  int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
> diff --git a/util/osdep.c b/util/osdep.c
> index 60fcbbaebe..545a88e1fd 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>  }
>  #endif
>  
> -static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> +int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
>  {
> +    ERRP_GUARD();

Looks redundant to me.

>      int ret;
> +
>  #ifdef O_CLOEXEC
>      ret = open(name, flags | O_CLOEXEC, mode);
>  #else
> @@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
>          qemu_set_cloexec(ret);
>      }
>  #endif
> +    if (ret == -1) {
> +        error_setg_errno(errp, errno, "Failed to open file '%s'", name);
> +    }
> +
>      return ret;
>  }
>  
> @@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>      }
>  #endif
>  
> -    ret = qemu_open_cloexec(name, flags, mode);
> +    ret = qemu_open_cloexec(name, flags, mode, NULL);
>  
>      if (ret == -1) {
>          const char *action = flags & O_CREAT ? "create" : "open";

... the patch additionally changes how errors are communicated.  Please
cover that in the commit message.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.