Re: [libvirt PATCH v2 07/10] esx_stream: Use automatic mutex management

On Fri, Mar 04, 2022 at 06:28:37PM +0100, Tim Wiederhake wrote: > Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; > --- >  src/esx/esx_stream.c | 65 ++++++++++++++-------------------------- > ---- >  1 file changed, 21 insertions(+), 44 deletions(-) > > diff --git a/src/esx/esx_stream.c b/src/esx/esx_stream.c > index 5b20804bb1..2b49c8dd12 100644 > --- a/src/esx/esx_stream.c > +++ b/src/esx/esx_stream.c > @@ -198,9 +198,8 @@ esxStreamTransfer(esxStreamPrivate *priv, bool > blocking) >  static int >  esxStreamSend(virStreamPtr stream, const char *data, size_t > nbytes) >  { > -    int result = -1; >      esxStreamPrivate *priv = stream->privateData; > -    int status; > +    VIR_LOCK_GUARD lock = virLockGuardLock(&priv->curl->lock); Coverity discovered that this change is not correct: /src/esx/esx_stream.c: 207 in esxStreamSend() 201         esxStreamPrivate *priv = stream->privateData; 202         VIR_LOCK_GUARD lock = virLockGuardLock(&priv->curl- >lock); 203 204         if (nbytes == 0) 205             return 0; 206 > > >     CID 389978:  Null pointer dereferences  (REVERSE_INULL) > > >     Null-checking "priv" suggests that it may be null, but it > > > has already been dereferenced on all paths leading to the > > > check. 207         if (!priv) { 208             virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Stream is not open")); 209             return -1; 210         } 211 212         if (priv->mode != ESX_STREAM_MODE_UPLOAD) { Previously the mutex was initialized after the !priv check, now we will dereference priv before this check. >      if (nbytes == 0) >          return 0; > @@ -215,38 +214,29 @@ esxStreamSend(virStreamPtr stream, const char > *data, size_t nbytes) >          return -1; >      } >   > -    virMutexLock(&priv->curl->lock); > - >      priv->buffer = (char *)data; >      priv->buffer_size = nbytes; >      priv->buffer_used = nbytes; >   >      if (stream->flags & VIR_STREAM_NONBLOCK) { >          if (esxStreamTransfer(priv, false) < 0) > -            goto cleanup; > +            return -1; >   > -        if (priv->buffer_used < priv->buffer_size) > -            result = priv->buffer_size - priv->buffer_used; > -        else > -            result = -2; > +        if (priv->buffer_used >= priv->buffer_size) > +            return -2; >      } else /* blocking */ { >          do { > -            status = esxStreamTransfer(priv, true); > +            int status = esxStreamTransfer(priv, true); >   >              if (status < 0) > -                goto cleanup; > +                return -1; >   >              if (status > 0) >                  break; >          } while (priv->buffer_used > 0); > - > -        result = priv->buffer_size - priv->buffer_used; >      } >   > - cleanup: > -    virMutexUnlock(&priv->curl->lock); > - > -    return result; > +    return priv->buffer_size - priv->buffer_used; >  } >   >  static int > @@ -255,9 +245,8 @@ esxStreamRecvFlags(virStreamPtr stream, >                     size_t nbytes, >                     unsigned int flags) >  { > -    int result = -1; >      esxStreamPrivate *priv = stream->privateData; > -    int status; > +    VIR_LOCK_GUARD lock = virLockGuardLock(&priv->curl->lock); And same happens here. Pavel

>   >      virCheckFlags(0, -1); >   > @@ -274,8 +263,6 @@ esxStreamRecvFlags(virStreamPtr stream, >          return -1; >      } >   > -    virMutexLock(&priv->curl->lock); > - >      priv->buffer = data; >      priv->buffer_size = nbytes; >      priv->buffer_used = 0; > @@ -291,33 +278,25 @@ esxStreamRecvFlags(virStreamPtr stream, >                  priv->backlog_used - priv->buffer_used); >   >          priv->backlog_used -= priv->buffer_used; > -        result = priv->buffer_used; >      } else if (stream->flags & VIR_STREAM_NONBLOCK) { >          if (esxStreamTransfer(priv, false) < 0) > -            goto cleanup; > +            return -1; >   > -        if (priv->buffer_used > 0) > -            result = priv->buffer_used; > -        else > -            result = -2; > +        if (priv->buffer_used <= 0) > +            return -2; >      } else /* blocking */ { >          do { > -            status = esxStreamTransfer(priv, true); > +            int status = esxStreamTransfer(priv, true); >   >              if (status < 0) > -                goto cleanup; > +                return -1; >   >              if (status > 0) >                  break; >          } while (priv->buffer_used < priv->buffer_size); > - > -        result = priv->buffer_used; >      } >   > - cleanup: > -    virMutexUnlock(&priv->curl->lock); > - > -    return result; > +    return priv->buffer_used; >  } >   >  static int > @@ -348,18 +327,16 @@ esxStreamClose(virStreamPtr stream, bool > finish) >      if (!priv) >          return 0; >   > -    virMutexLock(&priv->curl->lock); > +    VIR_WITH_MUTEX_LOCK_GUARD(&priv->curl->lock) { > +        if (finish && priv->backlog_used > 0) { > +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                           _("Stream has untransferred data > left")); > +            result = -1; > +        } >   > -    if (finish && priv->backlog_used > 0) { > -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                       _("Stream has untransferred data left")); > -        result = -1; > +        stream->privateData = NULL; >      } >   > -    stream->privateData = NULL; > - > -    virMutexUnlock(&priv->curl->lock); > - >      esxFreeStreamPrivate(&priv); >   >      return result; > -- > 2.31.1 >