subject:"\[PATCH v4 2\/6\] util\: refactor qemu_open_old to split off variadic args handling"

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

2020-08-26 Thread Markus Armbruster

Daniel P. Berrangé  writes:

> On Tue, Aug 25, 2020 at 04:56:40PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé  writes:
>> 
>> > This simple refactoring prepares for future patches. The variadic args
>> > handling is split from the main bulk of the open logic. The duplicated
>> > calls to open() are removed in favour of updating the "flags" variable
>> > to have O_CLOEXEC.
>> >
>> > Signed-off-by: Daniel P. Berrangé 
>> > ---
>> >  util/osdep.c | 40 +++-
>> >  1 file changed, 27 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/util/osdep.c b/util/osdep.c
>> > index 9df1b6adec..9ff92551e7 100644
>> > --- a/util/osdep.c
>> > +++ b/util/osdep.c
>> > @@ -22,6 +22,7 @@
>> >   * THE SOFTWARE.
>> >   */
>> >  #include "qemu/osdep.h"
>> > +#include "qapi/error.h"
>> >  
>> >  /* Needed early for CONFIG_BSD etc. */
>> >  
>> > @@ -282,10 +283,10 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t 
>> > len, bool exclusive)
>> >  /*
>> >   * Opens a file with FD_CLOEXEC set
>> >   */
>> > -int qemu_open_old(const char *name, int flags, ...)
>> > +static int
>> > +qemu_open_internal(const char *name, int flags, mode_t mode)
>> >  {
>> >  int ret;
>> > -int mode = 0;
>> >  
>> >  #ifndef _WIN32
>> >  const char *fdset_id_str;
>> > @@ -323,22 +324,35 @@ int qemu_open_old(const char *name, int flags, ...)
>> >  }
>> >  #endif
>> >  
>> > -if (flags & O_CREAT) {
>> > -va_list ap;
>> > -
>> > -va_start(ap, flags);
>> > -mode = va_arg(ap, int);
>> > -va_end(ap);
>> > -}
>> > -
>> >  #ifdef O_CLOEXEC
>> > -ret = open(name, flags | O_CLOEXEC, mode);
>> > -#else
>> > +flags |= O_CLOEXEC;
>> > +#endif /* O_CLOEXEC */
>> > +
>> >  ret = open(name, flags, mode);
>> > +
>> > +#ifndef O_CLOEXEC
>> >  if (ret >= 0) {
>> >  qemu_set_cloexec(ret);
>> >  }
>> > -#endif
>> > +#endif /* ! O_CLOEXEC */
>> 
>> I dislike this part, to be honest.  I find
>> 
>> #ifdef O_CLOEXEC
>> flags |= O_CLOEXEC;
>> #endif /* O_CLOEXEC */
>> 
>> ret = open(name, flags, mode);
>> 
>> #ifndef O_CLOEXEC
>> if (ret >= 0) {
>> qemu_set_cloexec(ret);
>> }
>> #endif /* ! O_CLOEXEC */
>> 
>> harder to read than
>> 
>> #ifdef O_CLOEXEC
>> ret = open(name, flags | O_CLOEXEC, mode);
>> #else
>> ret = open(name, flags, mode);
>> if (ret >= 0) {
>> qemu_set_cloexec(ret);
>> }
>> #endif
>
> We're re-using 'flags' variable again in a later patch and want to
> have O_CLOEXEC present in it then too.

I guess you mean PATCH 5.  I'll get back the the issue there.

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

2020-08-25 Thread Daniel P . Berrangé

On Tue, Aug 25, 2020 at 04:56:40PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > This simple refactoring prepares for future patches. The variadic args
> > handling is split from the main bulk of the open logic. The duplicated
> > calls to open() are removed in favour of updating the "flags" variable
> > to have O_CLOEXEC.
> >
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  util/osdep.c | 40 +++-
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 9df1b6adec..9ff92551e7 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -22,6 +22,7 @@
> >   * THE SOFTWARE.
> >   */
> >  #include "qemu/osdep.h"
> > +#include "qapi/error.h"
> >  
> >  /* Needed early for CONFIG_BSD etc. */
> >  
> > @@ -282,10 +283,10 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t 
> > len, bool exclusive)
> >  /*
> >   * Opens a file with FD_CLOEXEC set
> >   */
> > -int qemu_open_old(const char *name, int flags, ...)
> > +static int
> > +qemu_open_internal(const char *name, int flags, mode_t mode)
> >  {
> >  int ret;
> > -int mode = 0;
> >  
> >  #ifndef _WIN32
> >  const char *fdset_id_str;
> > @@ -323,22 +324,35 @@ int qemu_open_old(const char *name, int flags, ...)
> >  }
> >  #endif
> >  
> > -if (flags & O_CREAT) {
> > -va_list ap;
> > -
> > -va_start(ap, flags);
> > -mode = va_arg(ap, int);
> > -va_end(ap);
> > -}
> > -
> >  #ifdef O_CLOEXEC
> > -ret = open(name, flags | O_CLOEXEC, mode);
> > -#else
> > +flags |= O_CLOEXEC;
> > +#endif /* O_CLOEXEC */
> > +
> >  ret = open(name, flags, mode);
> > +
> > +#ifndef O_CLOEXEC
> >  if (ret >= 0) {
> >  qemu_set_cloexec(ret);
> >  }
> > -#endif
> > +#endif /* ! O_CLOEXEC */
> 
> I dislike this part, to be honest.  I find
> 
> #ifdef O_CLOEXEC
> flags |= O_CLOEXEC;
> #endif /* O_CLOEXEC */
> 
> ret = open(name, flags, mode);
> 
> #ifndef O_CLOEXEC
> if (ret >= 0) {
> qemu_set_cloexec(ret);
> }
> #endif /* ! O_CLOEXEC */
> 
> harder to read than
> 
> #ifdef O_CLOEXEC
> ret = open(name, flags | O_CLOEXEC, mode);
> #else
> ret = open(name, flags, mode);
> if (ret >= 0) {
> qemu_set_cloexec(ret);
> }
> #endif

We're re-using 'flags' variable again in a later patch and want to
have O_CLOEXEC present in it then too.

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 v4 2/6] util: refactor qemu_open_old to split off variadic args handling

2020-08-25 Thread Markus Armbruster

Daniel P. Berrangé  writes:

> This simple refactoring prepares for future patches. The variadic args
> handling is split from the main bulk of the open logic. The duplicated
> calls to open() are removed in favour of updating the "flags" variable
> to have O_CLOEXEC.
>
> Signed-off-by: Daniel P. Berrangé 
> ---
>  util/osdep.c | 40 +++-
>  1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 9df1b6adec..9ff92551e7 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  
>  /* Needed early for CONFIG_BSD etc. */
>  
> @@ -282,10 +283,10 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t 
> len, bool exclusive)
>  /*
>   * Opens a file with FD_CLOEXEC set
>   */
> -int qemu_open_old(const char *name, int flags, ...)
> +static int
> +qemu_open_internal(const char *name, int flags, mode_t mode)
>  {
>  int ret;
> -int mode = 0;
>  
>  #ifndef _WIN32
>  const char *fdset_id_str;
> @@ -323,22 +324,35 @@ int qemu_open_old(const char *name, int flags, ...)
>  }
>  #endif
>  
> -if (flags & O_CREAT) {
> -va_list ap;
> -
> -va_start(ap, flags);
> -mode = va_arg(ap, int);
> -va_end(ap);
> -}
> -
>  #ifdef O_CLOEXEC
> -ret = open(name, flags | O_CLOEXEC, mode);
> -#else
> +flags |= O_CLOEXEC;
> +#endif /* O_CLOEXEC */
> +
>  ret = open(name, flags, mode);
> +
> +#ifndef O_CLOEXEC
>  if (ret >= 0) {
>  qemu_set_cloexec(ret);
>  }
> -#endif
> +#endif /* ! O_CLOEXEC */

I dislike this part, to be honest.  I find

#ifdef O_CLOEXEC
flags |= O_CLOEXEC;
#endif /* O_CLOEXEC */

ret = open(name, flags, mode);

#ifndef O_CLOEXEC
if (ret >= 0) {
qemu_set_cloexec(ret);
}
#endif /* ! O_CLOEXEC */

harder to read than

#ifdef O_CLOEXEC
ret = open(name, flags | O_CLOEXEC, mode);
#else
ret = open(name, flags, mode);
if (ret >= 0) {
qemu_set_cloexec(ret);
}
#endif

> +
> +return ret;
> +}
> +
> +
> +int qemu_open_old(const char *name, int flags, ...)
> +{
> +va_list ap;
> +mode_t mode = 0;
> +int ret;
> +
> +va_start(ap, flags);
> +if (flags & O_CREAT) {
> +mode = va_arg(ap, int);
> +}
> +va_end(ap);
> +
> +ret = qemu_open_internal(name, flags, mode);
>  
>  #ifdef O_DIRECT
>  if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {

[PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

2020-08-21 Thread Daniel P . Berrangé

This simple refactoring prepares for future patches. The variadic args
handling is split from the main bulk of the open logic. The duplicated
calls to open() are removed in favour of updating the "flags" variable
to have O_CLOEXEC.

Signed-off-by: Daniel P. Berrangé 
---
 util/osdep.c | 40 +++-
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index 9df1b6adec..9ff92551e7 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 
 /* Needed early for CONFIG_BSD etc. */
 
@@ -282,10 +283,10 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, 
bool exclusive)
 /*
  * Opens a file with FD_CLOEXEC set
  */
-int qemu_open_old(const char *name, int flags, ...)
+static int
+qemu_open_internal(const char *name, int flags, mode_t mode)
 {
 int ret;
-int mode = 0;
 
 #ifndef _WIN32
 const char *fdset_id_str;
@@ -323,22 +324,35 @@ int qemu_open_old(const char *name, int flags, ...)
 }
 #endif
 
-if (flags & O_CREAT) {
-va_list ap;
-
-va_start(ap, flags);
-mode = va_arg(ap, int);
-va_end(ap);
-}
-
 #ifdef O_CLOEXEC
-ret = open(name, flags | O_CLOEXEC, mode);
-#else
+flags |= O_CLOEXEC;
+#endif /* O_CLOEXEC */
+
 ret = open(name, flags, mode);
+
+#ifndef O_CLOEXEC
 if (ret >= 0) {
 qemu_set_cloexec(ret);
 }
-#endif
+#endif /* ! O_CLOEXEC */
+
+return ret;
+}
+
+
+int qemu_open_old(const char *name, int flags, ...)
+{
+va_list ap;
+mode_t mode = 0;
+int ret;
+
+va_start(ap, flags);
+if (flags & O_CREAT) {
+mode = va_arg(ap, int);
+}
+va_end(ap);
+
+ret = qemu_open_internal(name, flags, mode);
 
 #ifdef O_DIRECT
 if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
-- 
2.26.2

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

[PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling

4 matches

Site Navigation

Mail list logo

Footer information