[libvirt] [PATCH] check return values of virBufferTrim
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; } -- 1.8.1.5
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. Michal
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? Or we could introduce another version of virBufferTrim, without a return value, to use anywhere we don't care about the return value (i.e. outside the tests :)) Jan
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
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 :|
participants (3)
-
Daniel P. Berrange -
Ján Tomko -
Michal Privoznik