A few new uses of already-checked functions were added recently, and I've added xmlXPathFreeObject to the list, so that exposed a bunch more. ----------------------------------------------------------- Remove more useless if tests before "free"-like functions. * build-aux/useless-if-before-free: Rename from ... * build-aux/find-unnecessary-if-before-free: ... this. Remove file. Also changed it so that new names are no longer hard-coded in the script. Instead, they're supplied via options: * Makefile.cfg (useless_free_options): Define. Add xmlXPathFreeObject to the list of free-like functions it detects. * Makefile.maint (sc_avoid_if_before_free): Reflect script renaming. * .x-sc_avoid_if_before_free: Likewise. * src/openvz_conf.c (openvzParseXML): Remove useless "if"-before-free. * src/qemu_conf.c (qemudParseXML, qemudParseNetworkXML): Likewise. * src/virsh.c (cmdVNCDisplay, cmdTTYConsole, cmdDetachInterface): (cmdDetachDisk): Likewise. * src/xm_internal.c (xenXMConfigSetIntFromXPath): Likewise. (xenXMConfigSetStringFromXPath, xenXMParseXMLToConfig): Likewise. (xenXMDomainAttachDevice, xenXMAttachDisk, xenXMAttachInterface): (xenXMDomainDetachDevice): Likewise. * src/xml.c (virXPathString): Likewise. * tests/xmlrpctest.c (checkRequestValue): Likewise. Signed-off-by: Jim Meyering <meyering(a)redhat.com&gt; --- .x-sc_avoid_if_before_free | 2 +- Makefile.cfg | 5 + Makefile.maint | 5 +- build-aux/find-unnecessary-if-before-free | 42 ---------- build-aux/useless-if-before-free | 118 +++++++++++++++++++++++++++++ src/openvz_conf.c | 3 +- src/qemu_conf.c | 44 ++++------- src/virsh.c | 12 +-- src/xm_internal.c | 60 +++++---------- src/xml.c | 3 +- tests/xmlrpctest.c | 5 +- 11 files changed, 170 insertions(+), 129 deletions(-) delete mode 100755 build-aux/find-unnecessary-if-before-free create mode 100755 build-aux/useless-if-before-free diff --git a/.x-sc_avoid_if_before_free b/.x-sc_avoid_if_before_free index f83ae7b..5093ef6 100644 --- a/.x-sc_avoid_if_before_free +++ b/.x-sc_avoid_if_before_free @@ -1,5 +1,5 @@ ^gnulib/lib/getaddrinfo\.c$ ^gnulib/lib/printf-parse\.c$ ^gnulib/lib/vasnprintf\.c$ -^build-aux/find-unnecessary-if-before-free$ +^build-aux/useless-if-before-free$ ^ChangeLog$ diff --git a/Makefile.cfg b/Makefile.cfg index a9578ab..4543ebd 100644 --- a/Makefile.cfg +++ b/Makefile.cfg @@ -52,3 +52,8 @@ local-checks-to-skip = \ patch-check \ check-AUTHORS \ changelog-check + +useless_free_options = \ + --name=sexpr_free \ + --name=xmlXPathFreeContext \ + --name=xmlXPathFreeObject diff --git a/Makefile.maint b/Makefile.maint index 923c422..8624328 100644 --- a/Makefile.maint +++ b/Makefile.maint @@ -40,7 +40,8 @@ syntax-check: $(local-check) ## --------------- ## sc_avoid_if_before_free: - @$(srcdir)/build-aux/find-unnecessary-if-before-free \ + @$(srcdir)/build-aux/useless-if-before-free \ + $(useless_free_options) \ $$($(CVS_LIST_EXCEPT)) && \ { echo '$(ME): found useless "if" before "free" above' 1>&2; \ exit 1; } || : @@ -288,7 +289,7 @@ sc_two_space_separator_in_usage: 1>&2; exit 1; } || : err_func_re = \ -(DISABLE_fprintf|(xmlRpc|vir(Xend|XML|Hash|Conf|Test|LibConn))Error) +(DISABLE_fprintf|qemudLog|(xmlRpc|vir(Xend|XML|Hash|Conf|Test|LibConn))Error) # Look for diagnostics that aren't marked for translation. # This won't find any for which error's format string is on a separate line. diff --git a/build-aux/find-unnecessary-if-before-free b/build-aux/find-unnecessary-if-before-free deleted file mode 100755 index 0cd38eb..0000000 --- a/build-aux/find-unnecessary-if-before-free +++ /dev/null @@ -1,42 +0,0 @@ -#!/usr/bin/perl -# Detect instances of "if (p) free (p);". -# Likewise for "if (p != NULL) free (p);". -# Exit status is like grep: 0 for no match. 1 for any number. - -# Note: giving line numbers isn't practical, since I've reset the -# input record separator. -use strict; -use warnings; -(my $ME = $0) =~ s|.*/||; - -{ - # Use ';' as the input record separator. - $/ = ';'; - - my $found_match = 0; - foreach my $file (@ARGV) - { - open FH, '<', $file - or die "$ME: can't open `$file' for reading: $!\n"; - while (defined (my $line = <FH>)) - { - if ($line =~ - /\b(if\s*\(\s*(\S+?)(?:\s*!=\s*NULL)?\s*\) - \s+(?:xmlXPathFreeContext|(?:sexpr_)?free)\s*\(\s*\2\s*\))/sx) - { - print "$file: $1\n"; - $found_match = 1; - } - } - close FH; - } - exit !$found_match; -} - -my $foo = <<'EOF'; -# The above is to *find* them. -# This adjusts them, removing the unnecessary "if (p)" part. - -git ls-files -z --exclude=find-unnecessary-if-before-free |xargs -0 \ -perl -0x3b -pi -e 's/\bif\s*\(\s*(\S+?)(?:\s*!=\s*NULL)?\s*\)\s+((?:xmlXPathFreeContext|(?:sexpr_)?free)\s*\(\s*\1\s*\))/$2/s' -EOF diff --git a/build-aux/useless-if-before-free b/build-aux/useless-if-before-free new file mode 100755 index 0000000..57040a3 --- /dev/null +++ b/build-aux/useless-if-before-free @@ -0,0 +1,118 @@ +#!/usr/bin/perl -T +# Detect instances of "if (p) free (p);". +# Likewise for "if (p != NULL) free (p);". + +my $VERSION = '2008-02-04 22:25'; # UTC +# The definition above must lie within the first 8 lines in order +# for the Emacs time-stamp write hook (at end) to update it. +# If you change this file with Emacs, please let the write hook +# do its job. Otherwise, update this string manually. + +# Exit status is like grep: 0 for no match. 1 for any number. +# Note: giving line numbers isn't practical, since I've reset the +# input record separator. +use strict; +use warnings; +use Getopt::Long; + +(my $ME = $0) =~ s|.*/||; + +my $debug = 0; +my $verbose = 0; + +sub usage ($) +{ + my ($exit_code) = @_; + my $STREAM = ($exit_code == 0 ? *STDOUT : *STDERR); + if ($exit_code != 0) + { + print $STREAM "Try `$ME --help' for more information.\n"; + } + else + { + print $STREAM <<EOF; +Usage: $ME [OPTIONS] FILE... + +OPTIONS: + + --name=N add name N to the list of `free'-like functions to detect; + may be repeated + + --help display this help and exit + --version output version information and exit + --verbose generate verbose output + +EXAMPLE: + + git ls-files -z |xargs -0 $ME + +EOF + } + exit $exit_code; +} + +{ + my @name = qw(free); + GetOptions + ( + debug => \$debug, + verbose => \$verbose, + help => sub { usage 0 }, + version => sub { print "$ME version $VERSION\n"; exit }, + 'name=s@' => \@name, + ) or usage 1; + + # Make sure we have the right number of non-option arguments. + # Always tell the user why we fail. + @ARGV < 1 + and (warn "$ME: missing FILE argument\n"), usage 1; + + my $or = join '|', @name; + my $regexp = qr/(?:$or)/; + + # Set the input record separator. + $/ = '"'; + + my $found_match = 0; + foreach my $file (@ARGV) + { + open FH, '<', $file + or die "$ME: can't open `$file' for reading: $!\n"; + while (defined (my $line = <FH>)) + { + if ($line =~ + /\b(if\s*\(\s*(\S+?)(?:\s*!=\s*NULL)?\s*\) + (?: \s*$regexp\s*\(\s*\2\s*\)| + \s*\{\s*$regexp\s*\(\s*\2\s*\)\s*;\s*\}))/sx) + { + print "$file: $1\n"; + $found_match = 1; + } + } + close FH; + } + exit !$found_match; +} + +my $foo = <<'EOF'; +# The above is to *find* them. +# This adjusts them, removing the unnecessary "if (p)" part. + +# FIXME: do something like this as an option. +git ls-files -z --exclude=$ME |xargs -0 \ +perl -0x3b -pi -e 's/\bif\s*\(\s*(\S+?)(?:\s*!=\s*NULL)?\s*\)\s+((?:sexpr_)?free\s*\(\s*\1\s*\))/$2/s' + +When modifying files, refuse to process anything other than a regular file. + +# Or this one-liner to detect them: +git ls-files -z |xargs -0 perl -00 -ne '/\b(if\s*\(\s*(\S+?)(?:\s*!=\s*NULL)?\s*\)(?:\s*(?:sexpr_)?free\s*\(\s*\2\s*\)|\s*\{\s*(?:sexpr_)?free\s*\(\s*\2\s*\)\s*;\s*\}))/sx and print "$1\n"' +EOF + +## Local Variables: +## indent-tabs-mode: nil +## eval: (add-hook 'write-file-hooks 'time-stamp) +## time-stamp-start: "my $VERSION = '" +## time-stamp-format: "%:y-%02m-%02d %02H:%02M" +## time-stamp-time-zone: "UTC" +## time-stamp-end: "'; # UTC" +## End: diff --git a/src/openvz_conf.c b/src/openvz_conf.c index ebed9b2..482de82 100644 --- a/src/openvz_conf.c +++ b/src/openvz_conf.c @@ -495,8 +495,7 @@ static struct openvz_vm_def bail_out: free(prop); - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); openvzFreeVMDef(def); diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 39cfdc1..aba8890 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1,7 +1,7 @@ /* * config.c: VM configuration management * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -1004,8 +1004,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, goto error; } } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); /* Extract domain memory */ @@ -1023,8 +1022,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, goto error; } } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); /* Extract domain vcpu info */ obj = xmlXPathEval(BAD_CAST "string(/domain/vcpu[1])", ctxt); @@ -1039,8 +1037,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, goto error; } } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); /* See if ACPI feature is requested */ obj = xmlXPathEval(BAD_CAST "/domain/features/acpi", ctxt); @@ -1062,8 +1059,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, else def->noReboot = 0; } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); /* See if we set clock to localtime */ obj = xmlXPathEval(BAD_CAST "string(/domain/clock/@offset)", ctxt); @@ -1076,8 +1072,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, else def->localtime = 0; } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); /* Extract OS type info */ @@ -1111,8 +1106,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, } strcpy(def->os.arch, (const char *)obj->stringval); } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); obj = xmlXPathEval(BAD_CAST "string(/domain/os/type[1]/@machine)", ctxt); if ((obj == NULL) || (obj->type != XPATH_STRING) || @@ -1134,8 +1128,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, } strcpy(def->os.machine, (const char *)obj->stringval); } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); obj = xmlXPathEval(BAD_CAST "string(/domain/os/kernel[1])", ctxt); @@ -1147,8 +1140,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, } strcpy(def->os.kernel, (const char *)obj->stringval); } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); obj = xmlXPathEval(BAD_CAST "string(/domain/os/initrd[1])", ctxt); @@ -1160,8 +1152,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, } strcpy(def->os.initrd, (const char *)obj->stringval); } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); obj = xmlXPathEval(BAD_CAST "string(/domain/os/cmdline[1])", ctxt); @@ -1173,8 +1164,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, } strcpy(def->os.cmdline, (const char *)obj->stringval); } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); /* analysis of the disk devices */ @@ -1224,8 +1214,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, } strcpy(def->os.binary, (const char *)obj->stringval); } - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); obj = xmlXPathEval(BAD_CAST "/domain/devices/graphics", ctxt); if ((obj == NULL) || (obj->type != XPATH_NODESET) || @@ -1382,8 +1371,7 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, error: free(prop); - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); qemudFreeVMDef(def); return NULL; @@ -2389,10 +2377,8 @@ static struct qemud_network_def *qemudParseNetworkXML(virConnectPtr conn, error: /* XXX free all the stuff in the qemud_network struct, or leave it upto the caller ? */ - if (obj) - xmlXPathFreeObject(obj); - if (tmp) - xmlXPathFreeObject(tmp); + xmlXPathFreeObject(obj); + xmlXPathFreeObject(tmp); xmlXPathFreeContext(ctxt); qemudFreeNetworkDef(def); return NULL; diff --git a/src/virsh.c b/src/virsh.c index 36808ed..9521eff 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -2931,8 +2931,7 @@ cmdVNCDisplay(vshControl * ctl, vshCmd * cmd) ret = TRUE; cleanup: - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); if (xml) xmlFreeDoc(xml); @@ -2993,8 +2992,7 @@ cmdTTYConsole(vshControl * ctl, vshCmd * cmd) vshPrint(ctl, "%s\n", (const char *)obj->stringval); cleanup: - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); if (xml) xmlFreeDoc(xml); @@ -3338,8 +3336,7 @@ cmdDetachInterface(vshControl * ctl, vshCmd * cmd) cleanup: if (dom) virDomainFree(dom); - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); if (xml) xmlFreeDoc(xml); @@ -3611,8 +3608,7 @@ cmdDetachDisk(vshControl * ctl, vshCmd * cmd) ret = TRUE; cleanup: - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); if (xml) xmlFreeDoc(xml); diff --git a/src/xm_internal.c b/src/xm_internal.c index b3a9f3a..fbef462 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -1556,8 +1556,7 @@ int xenXMConfigSetIntFromXPath(virConnectPtr conn, ret = 0; error: - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); return ret; } @@ -1591,8 +1590,7 @@ int xenXMConfigSetStringFromXPath(virConnectPtr conn, ret = 0; error: - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); return ret; } @@ -2278,8 +2276,7 @@ virConfPtr xenXMParseXMLToConfig(virConnectPtr conn, const char *xml) { virConfFree(conf); if (prop != NULL) xmlFree(prop); - if (obj != NULL) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); if (doc != NULL) xmlFreeDoc(doc); @@ -2578,8 +2575,7 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) { (obj->stringval) && (STREQ((char *)obj->stringval, "hvm"))) hvm = 1; - if (ctxt) - xmlXPathFreeContext(ctxt); + xmlXPathFreeContext(ctxt); ctxt = NULL; if (doc) xmlFreeDoc(doc); @@ -2617,12 +2613,9 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) { ret = 0; cleanup: - if (domxml) - free(domxml); - if (obj) - xmlXPathFreeObject(obj); - if (ctxt) - xmlXPathFreeContext(ctxt); + free(domxml); + xmlXPathFreeObject(obj); + xmlXPathFreeContext(ctxt); if (doc) xmlFreeDoc(doc); @@ -2736,8 +2729,7 @@ xenXMAttachDisk(virDomainPtr domain, xmlXPathContextPtr ctxt, int hvm, prev->next = list_val; } else { /* configure */ - if (list_val->str) - free(list_val->str); + free(list_val->str); list_val->str = dev; } @@ -2745,12 +2737,9 @@ xenXMAttachDisk(virDomainPtr domain, xmlXPathContextPtr ctxt, int hvm, goto cleanup; cleanup: - if (type) - free(type); - if (source) - free(source); - if (target) - free(target); + free(type); + free(source); + free(target); return (ret); } @@ -2823,8 +2812,7 @@ xenXMAttachInterface(virDomainPtr domain, xmlXPathContextPtr ctxt, int hvm, if (!(strcmp(dommac, (const char *) mac))) { if (autoassign) { - if (mac) - free(mac); + free(mac); mac = NULL; if (!(mac = (xmlChar *)xenXMAutoAssignMac())) goto cleanup; @@ -2912,8 +2900,7 @@ xenXMAttachInterface(virDomainPtr domain, xmlXPathContextPtr ctxt, int hvm, prev->next = list_val; } else { /* configure */ - if (list_val->str) - free(list_val->str); + free(list_val->str); list_val->str = dev; } @@ -2928,12 +2915,9 @@ xenXMAttachInterface(virDomainPtr domain, xmlXPathContextPtr ctxt, int hvm, if (text_node) xmlFree(text_node); cleanup: - if (type) - free(type); - if (source) - free(source); - if (mac) - free(mac); + free(type); + free(source); + free(mac); return (ret); } @@ -3137,16 +3121,12 @@ xenXMDomainDetachDevice(virDomainPtr domain, const char *xml) { ret = 0; cleanup: - if (ctxt) - xmlXPathFreeContext(ctxt); + xmlXPathFreeContext(ctxt); if (doc) xmlFreeDoc(doc); - if (domdevice) - free(domdevice); - if (key) - free(key); - if (list_val) - free(list_val); + free(domdevice); + free(key); + free(list_val); return (ret); } diff --git a/src/xml.c b/src/xml.c index 614deb0..4ac1765 100644 --- a/src/xml.c +++ b/src/xml.c @@ -479,8 +479,7 @@ virXPathString(const char *xpath, xmlXPathContextPtr ctxt) obj = xmlXPathEval(BAD_CAST xpath, ctxt); if ((obj == NULL) || (obj->type != XPATH_STRING) || (obj->stringval == NULL) || (obj->stringval[0] == 0)) { - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); return (NULL); } ret = strdup((char *) obj->stringval); diff --git a/tests/xmlrpctest.c b/tests/xmlrpctest.c index 9297004..5c16b0b 100644 --- a/tests/xmlrpctest.c +++ b/tests/xmlrpctest.c @@ -1,7 +1,7 @@ /* * xmlrpctest.c: simple client for XML-RPC tests * - * Copyright (C) 2005 Red Hat, Inc. + * Copyright (C) 2005, 2008 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -119,8 +119,7 @@ checkRequestValue(const char *xmlstr, const char *xpath, int type, void *expecte ret = 0; error: - if (obj) - xmlXPathFreeObject(obj); + xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); if (xml) xmlFreeDoc(xml); -- 1.5.4.30.g7dbe