[libvirt] [PATCH] check return values of virBufferTrim

[Daniel P. Berrange]

On Mon, Jun 17, 2013 at 02:15:17PM +0200, Michal Privoznik wrote:
> On 17.06.2013 10:34, Ján Tomko wrote:
> > Just to silence Coverity:
> > 
> > Event check_return:
> > Calling function "virBufferTrim(virBufferPtr, char const *, int)"
> > without checking return value (as is done elsewhere 5 out of 6 times).
> > ---
> >  src/node_device/node_device_udev.c | 5 ++---
> >  src/rpc/virnetsshsession.c         | 3 +--
> >  2 files changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index bb58415..a37989a 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -370,9 +370,8 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
> >      const char *format = NULL;
> >  
> >      virBufferAdd(&buf, fmt, -1);
> > -    virBufferTrim(&buf, "\n", -1);
> > -
> > -    format = virBufferContentAndReset(&buf);
> > +    if (virBufferTrim(&buf, "\n", -1) >= 0)
> > +        format = virBufferContentAndReset(&buf);
> >  
> >      virLogVMessage(VIR_LOG_FROM_LIBRARY,
> >                     virLogPriorityFromSyslog(priority),
> > diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
> > index b6aedc8..2299871 100644
> > --- a/src/rpc/virnetsshsession.c
> > +++ b/src/rpc/virnetsshsession.c
> > @@ -362,9 +362,8 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess)
> >               * we have to use a *MAGIC* constant. */
> >              for (i = 0; i < 16; i++)
> >                      virBufferAsprintf(&buff, "%02hhX:", keyhash[i]);
> > -            virBufferTrim(&buff, ":", 1);
> >  
> > -            if (virBufferError(&buff) != 0) {
> > +            if (virBufferTrim(&buff, ":", 1) < 0) {
> >                  virReportOOMError();
> >                  return -1;
> >              }
> > 
> 
> Shouldn't we make virBufferTrim call virBufferSetError instead? I think
> it's a better approach, as it fits to calling scheme of other virBuffer*
> functions better:
> 
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> 
> virBufferSomething(&buf, ...);
> virBufferSomethingElse(&buf, ...);
> 
> if (virBufferError(&buf) != 0) {
>    virReportError(...);
> } else {
>    char *tmp = virBufferContentAndReset(&buf);
>    ...
> }
> 
> I guess it will shut the coverity up as well.

Yep, I think virBufferTrim should be 'void' as the other functions
are. We should only check return errors with virBufferError.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|