[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Libvir] Remove more useless if tests before "free"-like functions.



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 redhat com>
---
 .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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]