On Wed, Apr 13, 2011 at 05:29:16PM -0600, Eric Blake wrote:
On 04/13/2011 12:13 PM, Daniel P. Berrange wrote:
The libvirt APIs reserve any negative value for indicating an error. Thus checks
if (virXXXX() == -1)
Should instead be
if (virXXXX() < 0)
* daemon/remote.c: s/ == -1/ < 0/ --- daemon/remote.c | 339 +++++++++++++++++++++++++++++-------------------------- 1 files changed, 180 insertions(+), 159 deletions(-)
Why the change in lines?
@@ -3661,6 +3662,7 @@ remoteDispatchListDefinedNetworks(struct qemud_server *server ATTRIBUTE_UNUSED, remote_list_defined_networks_ret *ret) { int rv = -1; + int len;
if (!conn) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); @@ -3679,12 +3681,12 @@ remoteDispatchListDefinedNetworks(struct qemud_server *server ATTRIBUTE_UNUSED, goto cleanup; }
- ret->names.names_len = - virConnectListDefinedNetworks(conn, + len = virConnectListDefinedNetworks(conn, ret->names.names_val, args->maxnames); - if (ret->names.names_len == -1) { + if (len < 0) { goto cleanup; } + ret->names.names_len = len;
Oh, I see. You didn't want to change ret until you know the value is valid.
It is a little worse than that. 'ret->names.names_len' is actually an 'unsigned int', and the old code was comparing that to '-1'. It worked, but it is kinda nasty to rely on signed/unsigned casts in this way. So I decided it was nicer to check for "< -1" with a signed variable, before assigning to the unsigned variable.
ACK.
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 :|