[PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Leonardo Bras]

During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
introduced, particularly at qio_channel_socket_writev().

Rewrite some of those changes so it's easier to read.
									...
Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 io/channel-socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index dc9c165de1..ef7c7cfbac 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
     size_t fdsize = sizeof(int) * nfds;
     struct cmsghdr *cmsg;
     int sflags = 0;
+    bool zero_copy_enabled = false;
 
     memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
 
@@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
 #ifdef QEMU_MSG_ZEROCOPY
     if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
         sflags = MSG_ZEROCOPY;
+        zero_copy_enabled = true;
     }
 #endif
 
@@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
             return QIO_CHANNEL_ERR_BLOCK;
         case EINTR:
             goto retry;
-#ifdef QEMU_MSG_ZEROCOPY
         case ENOBUFS:
-            if (sflags & MSG_ZEROCOPY) {
+            if (zero_copy_enabled) {
                 error_setg_errno(errp, errno,
                                  "Process can't lock enough memory for using MSG_ZEROCOPY");
                 return -1;
             }
             break;
-#endif
         }
 
         error_setg_errno(errp, errno,
-- 
2.36.1

[PATCH v2 2/2] QIOChannelSocket: Fix zero-copy send so socket flush works

[Leonardo Bras]

Somewhere between v6 and v7 the of the zero-copy-send patchset a crucial
part of the flushing mechanism got missing: incrementing zero_copy_queued.

Without that, the flushing interface becomes a no-op, and there is no
guarantee the buffer is really sent.

This can go as bad as causing a corruption in RAM during migration.

Fixes: 2bc58ffc2926 ("QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX")
Reported-by: 徐闯 <xuchuangxclwt@bytedance.com>
Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 io/channel-socket.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index ef7c7cfbac..ca4cae930f 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -607,6 +607,11 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
                          "Unable to write to socket");
         return -1;
     }
+
+    if (zero_copy_enabled) {
+        sioc->zero_copy_queued++;
+    }
+
     return ret;
 }
 #else /* WIN32 */
-- 
2.36.1

Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Daniel P. Berrangé]

On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote:
> During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
> introduced, particularly at qio_channel_socket_writev().
> 
> Rewrite some of those changes so it's easier to read.
> 									...
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  io/channel-socket.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index dc9c165de1..ef7c7cfbac 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>      size_t fdsize = sizeof(int) * nfds;
>      struct cmsghdr *cmsg;
>      int sflags = 0;
> +    bool zero_copy_enabled = false;
>  
>      memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
>  
> @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>  #ifdef QEMU_MSG_ZEROCOPY
>      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
>          sflags = MSG_ZEROCOPY;
> +        zero_copy_enabled = true;
>      }

There should be a

 #else
    error_setg(errp, "Zero copy not supported on this platform");
    return -1;
 #endif

>  #endif
>  
> @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>              return QIO_CHANNEL_ERR_BLOCK;
>          case EINTR:
>              goto retry;
> -#ifdef QEMU_MSG_ZEROCOPY
>          case ENOBUFS:
> -            if (sflags & MSG_ZEROCOPY) {
> +            if (zero_copy_enabled) {

if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)

avoids the #ifdef without needing to add yet another
variable expressing what's already expressed in both
'flags' and 'sflags'.

>                  error_setg_errno(errp, errno,
>                                   "Process can't lock enough memory for using MSG_ZEROCOPY");
>                  return -1;
>              }
>              break;
> -#endif
>          }
>  
>          error_setg_errno(errp, errno,
> -- 
> 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 v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Leonardo Bras Soares Passos]

Hello Daniel,

On Thu, Jun 9, 2022 at 5:10 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote:
> > During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
> > introduced, particularly at qio_channel_socket_writev().
> >
> > Rewrite some of those changes so it's easier to read.
> >                                                                       ...
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  io/channel-socket.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index dc9c165de1..ef7c7cfbac 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> >      size_t fdsize = sizeof(int) * nfds;
> >      struct cmsghdr *cmsg;
> >      int sflags = 0;
> > +    bool zero_copy_enabled = false;
> >
> >      memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
> >
> > @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> >  #ifdef QEMU_MSG_ZEROCOPY
> >      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> >          sflags = MSG_ZEROCOPY;
> > +        zero_copy_enabled = true;
> >      }
>
> There should be a
>
>  #else
>     error_setg(errp, "Zero copy not supported on this platform");
>     return -1;
>  #endif
>

IIUC, if done as suggested, it will break every non-zero-copy call of
qio_channel_socket_writev();

I think you are suggesting something like :

    if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
#ifdef QEMU_MSG_ZEROCOPY
        sflags = MSG_ZEROCOPY;
        zero_copy_enabled = true; // I know you suggested this out,
just for example purposes
#else
        error_setg(errp, "Zero copy not supported on this platform");
        return -1;
#endif
    }

Which is supposed to fail if QIO_CHANNEL_WRITE_FLAG_ZERO_COPY is specified, but
qemu does not support it at compile time.

If I get the part above correctly, it would not be necessary, as
qio_channel_socket_writev() is
called only by qio_channel_writev_full(), which tests:

    if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) &&
        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) {
        error_setg_errno(errp, EINVAL,
                         "Requested Zero Copy feature is not available");
        return -1;
    }

and QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY is only set in
qio_channel_socket_connect_sync(), and is conditional to QEMU_MSG_ZEROCOPY
being enabled during compile time. Meaning it's the same test as
before mentioned, but
failing earlier.

> >  #endif
> >
> > @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> >              return QIO_CHANNEL_ERR_BLOCK;
> >          case EINTR:
> >              goto retry;
> > -#ifdef QEMU_MSG_ZEROCOPY
> >          case ENOBUFS:
> > -            if (sflags & MSG_ZEROCOPY) {
> > +            if (zero_copy_enabled) {
>
> if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)
>
> avoids the #ifdef without needing to add yet another
> variable expressing what's already expressed in both
> 'flags' and 'sflags'.

Yes, it does, but at the cost of not compiling-out the zero-copy part
when it's not supported,
since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up
meaning there will be at least one extra test for every time this
function is called (the one in the next patch).

An option would be testing sflags & MSG_ZEROCOPY, which would
compile-out zero-copy code
if it's not supported, but there was a bug in some distros where
MSG_ZEROCOPY is not defined,
causing the build to fail.

I understand the idea of reusing those variables instead of creating a
new one, but this boolean
variable will most certainly be compiled-out in this function, and
will allow compiling out the
zero-copy code where it's not supported.

Best regards,
Leo


>
> >                  error_setg_errno(errp, errno,
> >                                   "Process can't lock enough memory for using MSG_ZEROCOPY");
> >                  return -1;
> >              }
> >              break;
> > -#endif
> >          }
> >
> >          error_setg_errno(errp, errno,
> > --
> > 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 v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Daniel P. Berrangé]

On Thu, Jun 09, 2022 at 10:30:19PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Daniel,
> 
> On Thu, Jun 9, 2022 at 5:10 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote:
> > > During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
> > > introduced, particularly at qio_channel_socket_writev().
> > >
> > > Rewrite some of those changes so it's easier to read.
> > >                                                                       ...
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > ---
> > >  io/channel-socket.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > index dc9c165de1..ef7c7cfbac 100644
> > > --- a/io/channel-socket.c
> > > +++ b/io/channel-socket.c
> > > @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > >      size_t fdsize = sizeof(int) * nfds;
> > >      struct cmsghdr *cmsg;
> > >      int sflags = 0;
> > > +    bool zero_copy_enabled = false;
> > >
> > >      memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
> > >
> > > @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > >  #ifdef QEMU_MSG_ZEROCOPY
> > >      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> > >          sflags = MSG_ZEROCOPY;
> > > +        zero_copy_enabled = true;
> > >      }
> >
> > There should be a
> >
> >  #else
> >     error_setg(errp, "Zero copy not supported on this platform");
> >     return -1;
> >  #endif
> >
> 
> IIUC, if done as suggested, it will break every non-zero-copy call of
> qio_channel_socket_writev();
> 
> I think you are suggesting something like :
> 
>     if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> #ifdef QEMU_MSG_ZEROCOPY
>         sflags = MSG_ZEROCOPY;
>         zero_copy_enabled = true; // I know you suggested this out,
> just for example purposes
> #else
>         error_setg(errp, "Zero copy not supported on this platform");
>         return -1;
> #endif
>     }

Yes, that is what I mean.

> 
> Which is supposed to fail if QIO_CHANNEL_WRITE_FLAG_ZERO_COPY is specified, but
> qemu does not support it at compile time.

Correct, the caller should have checked the ZERO_COPY feeature
when they first opened the channel, and if they none the less
pass ZERO_COPY when it isn't supported that is a programmer
error that needs reporting.

> If I get the part above correctly, it would not be necessary, as
> qio_channel_socket_writev() is
> called only by qio_channel_writev_full(), which tests:
> 
>     if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) &&
>         !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) {
>         error_setg_errno(errp, EINVAL,
>                          "Requested Zero Copy feature is not available");
>         return -1;
>     }

Ok, so if it is checked earlier then we merely need an assert.

     if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
 #ifdef QEMU_MSG_ZEROCOPY
         sflags = MSG_ZEROCOPY;
         zero_copy_enabled = true;
 #else
         g_assert_unreachable();
 #endif
>     }



> > > @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > >              return QIO_CHANNEL_ERR_BLOCK;
> > >          case EINTR:
> > >              goto retry;
> > > -#ifdef QEMU_MSG_ZEROCOPY
> > >          case ENOBUFS:
> > > -            if (sflags & MSG_ZEROCOPY) {
> > > +            if (zero_copy_enabled) {
> >
> > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)
> >
> > avoids the #ifdef without needing to add yet another
> > variable expressing what's already expressed in both
> > 'flags' and 'sflags'.
> 
> Yes, it does, but at the cost of not compiling-out the zero-copy part
> when it's not supported,
> since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up
> meaning there will be at least one extra test for every time this
> function is called (the one in the next patch).

The cost of a simple bit test is between negligible-and-non-existant
with branch prediction. I doubt it would be possible to even measure
it.

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 v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Leonardo Bras Soares Passos]

On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>

[...]

> Ok, so if it is checked earlier then we merely need an assert.
>
>      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
>  #ifdef QEMU_MSG_ZEROCOPY
>          sflags = MSG_ZEROCOPY;
>          zero_copy_enabled = true;
>  #else
>          g_assert_unreachable();
>  #endif
> >     }

Ok, I will add that in the next version.

>
>
>
> > > > @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > > >              return QIO_CHANNEL_ERR_BLOCK;
> > > >          case EINTR:
> > > >              goto retry;
> > > > -#ifdef QEMU_MSG_ZEROCOPY
> > > >          case ENOBUFS:
> > > > -            if (sflags & MSG_ZEROCOPY) {
> > > > +            if (zero_copy_enabled) {
> > >
> > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)
> > >
> > > avoids the #ifdef without needing to add yet another
> > > variable expressing what's already expressed in both
> > > 'flags' and 'sflags'.
> >
> > Yes, it does, but at the cost of not compiling-out the zero-copy part
> > when it's not supported,
> > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up
> > meaning there will be at least one extra test for every time this
> > function is called (the one in the next patch).
>
> The cost of a simple bit test is between negligible-and-non-existant
> with branch prediction. I doubt it would be possible to even measure
> it.

Yeah, you are probably right on that.
So the main learning point here is that it's not worth creating a new
boolean for compiling-out
code that should not impact performance ?
I mean, if performance-wise they should be the same, then a new
variable would be just a
bother for the programmer.

Best regards,
Leo






>
> 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 v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Daniel P. Berrangé]

On Mon, Jun 13, 2022 at 06:21:18PM -0300, Leonardo Bras Soares Passos wrote:
> On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> 
> [...]
> 
> > Ok, so if it is checked earlier then we merely need an assert.
> >
> >      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> >  #ifdef QEMU_MSG_ZEROCOPY
> >          sflags = MSG_ZEROCOPY;
> >          zero_copy_enabled = true;
> >  #else
> >          g_assert_unreachable();
> >  #endif
> > >     }
> 
> Ok, I will add that in the next version.
> 
> >
> >
> >
> > > > > @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > > > >              return QIO_CHANNEL_ERR_BLOCK;
> > > > >          case EINTR:
> > > > >              goto retry;
> > > > > -#ifdef QEMU_MSG_ZEROCOPY
> > > > >          case ENOBUFS:
> > > > > -            if (sflags & MSG_ZEROCOPY) {
> > > > > +            if (zero_copy_enabled) {
> > > >
> > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)
> > > >
> > > > avoids the #ifdef without needing to add yet another
> > > > variable expressing what's already expressed in both
> > > > 'flags' and 'sflags'.
> > >
> > > Yes, it does, but at the cost of not compiling-out the zero-copy part
> > > when it's not supported,
> > > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up
> > > meaning there will be at least one extra test for every time this
> > > function is called (the one in the next patch).
> >
> > The cost of a simple bit test is between negligible-and-non-existant
> > with branch prediction. I doubt it would be possible to even measure
> > it.
> 
> Yeah, you are probably right on that.
> So the main learning point here is that it's not worth creating a new
> boolean for compiling-out
> code that should not impact performance ?

As ever "it depends" so there's no hard rule, and sometimes it can
verge on bikeshed colouring :-)

I didn't like the variable in this case, because it introduces a 3rd
variable to the method for representing whether zero copy is need,
which is excessive. I'm not a fan of redundancy as it can often then
lead to inconsistency. So it would need a compelling reason why it is
better, which is difficult for such a simple method. If the code was
more complex, a variable might have benefit of clarity, but in this
case IMHO it was just overkill.

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 v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Leonardo Bras Soares Passos]

On Tue, Jun 14, 2022 at 5:36 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jun 13, 2022 at 06:21:18PM -0300, Leonardo Bras Soares Passos wrote:
> > On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> >
> > [...]
> >
> > > Ok, so if it is checked earlier then we merely need an assert.
> > >
> > >      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> > >  #ifdef QEMU_MSG_ZEROCOPY
> > >          sflags = MSG_ZEROCOPY;
> > >          zero_copy_enabled = true;
> > >  #else
> > >          g_assert_unreachable();
> > >  #endif
> > > >     }
> >
> > Ok, I will add that in the next version.
> >
> > >
> > >
> > >
> > > > > > @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > > > > >              return QIO_CHANNEL_ERR_BLOCK;
> > > > > >          case EINTR:
> > > > > >              goto retry;
> > > > > > -#ifdef QEMU_MSG_ZEROCOPY
> > > > > >          case ENOBUFS:
> > > > > > -            if (sflags & MSG_ZEROCOPY) {
> > > > > > +            if (zero_copy_enabled) {
> > > > >
> > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)
> > > > >
> > > > > avoids the #ifdef without needing to add yet another
> > > > > variable expressing what's already expressed in both
> > > > > 'flags' and 'sflags'.
> > > >
> > > > Yes, it does, but at the cost of not compiling-out the zero-copy part
> > > > when it's not supported,
> > > > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up
> > > > meaning there will be at least one extra test for every time this
> > > > function is called (the one in the next patch).
> > >
> > > The cost of a simple bit test is between negligible-and-non-existant
> > > with branch prediction. I doubt it would be possible to even measure
> > > it.
> >
> > Yeah, you are probably right on that.
> > So the main learning point here is that it's not worth creating a new
> > boolean for compiling-out
> > code that should not impact performance ?
>
> As ever "it depends" so there's no hard rule, and sometimes it can
> verge on bikeshed colouring :-)
>
> I didn't like the variable in this case, because it introduces a 3rd
> variable to the method for representing whether zero copy is need,
> which is excessive. I'm not a fan of redundancy as it can often then
> lead to inconsistency. So it would need a compelling reason why it is
> better, which is difficult for such a simple method. If the code was
> more complex, a variable might have benefit of clarity, but in this
> case IMHO it was just overkill.

I see. Thanks for the clarification!

Best regards,
Leo

>
> 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 :|
>