[libvirt] [PATCH] check return values of virBufferTrim

[Michal Privoznik]

On 17.06.2013 14:53, Ján Tomko wrote:
> On 06/17/2013 02:15 PM, 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:
> 
> Is it really an error if you can't find a string to trim?

No, but is an error if you don't provide a string and provide a len,
that is you call:

virBufferTrim(&buf, NULL, -1);

All other combinations are valid == no error is set. So I think we need
something like this:

diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 693e4b2..9004b35 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -669,9 +669,14 @@ virBufferTrim(virBufferPtr buf, const char *str,
int len)
 {
     size_t len2 = 0;

-    if (!buf || buf->error || (!str && len < 0))
+    if (!buf || buf->error)
         return -1;

+    if (!str && len < 0) {
+        virBufferSetError(buf, -1);
+        return -1;
+    }
+
     if (len > 0 && len > buf->use)
         return 0;
     if (str) {


I think this will both meet the scheme as I've pointed out above and
silent coverity.

Michal