Libguestfs - Jan 2012 - [PATCH 1/3] launch: move the filename checking to a wrapper

Move the filename's comma character checking to a wrapper.

Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
---
 src/launch.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index ca89b63..8eaaac8 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -277,6 +277,16 @@ valid_format_iface (const char *str)
   return 1;
 }
 
+static int
+check_path (guestfs_h *g, const char *filename)
+{
+  if (strchr (filename, ',') != NULL) {
+    error (g, _("filename cannot contain ',' (comma)
character"));
+    return 1;
+  }
+  return 0;
+}
+
 int
 guestfs__add_drive_opts (guestfs_h *g, const char *filename,
                          const struct guestfs_add_drive_opts_argv *optargs)
@@ -287,10 +297,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
   char *name;
   int use_cache_off;
 
-  if (strchr (filename, ',') != NULL) {
-    error (g, _("filename cannot contain ',' (comma)
character"));
+  if (check_path(g, filename))
     return -1;
-  }
 
   readonly = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK
              ? optargs->readonly : 0;
@@ -405,10 +413,8 @@ guestfs__add_drive_ro_with_if (guestfs_h *g, const char
*filename,
 int
 guestfs__add_cdrom (guestfs_h *g, const char *filename)
 {
-  if (strchr (filename, ',') != NULL) {
-    error (g, _("filename cannot contain ',' (comma)
character"));
+  if (check_path(g, filename))
     return -1;
-  }
 
   if (access (filename, F_OK) == -1) {
     perrorf (g, "%s", filename);
-- 
1.7.8

Code cleanup.
Add a goto label to simplify the code.

Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
---
 src/launch.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index 8eaaac8..2840767 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -312,18 +312,12 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
   if (format && !valid_format_iface (format)) {
     error (g, _("%s parameter is empty or contains disallowed
characters"),
            "format");
-    free (format);
-    free (iface);
-    free (name);
-    return -1;
+    goto err_out;
   }
   if (!valid_format_iface (iface)) {
     error (g, _("%s parameter is empty or contains disallowed
characters"),
            "iface");
-    free (format);
-    free (iface);
-    free (name);
-    return -1;
+    goto err_out;
   }
 
   /* For writable files, see if we can use cache=off.  This also
@@ -331,20 +325,13 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
    * to do the check explicitly.
    */
   use_cache_off = readonly ? 0 : test_cache_off (g, filename);
-  if (use_cache_off == -1) {
-    free (format);
-    free (iface);
-    free (name);
-    return -1;
-  }
+  if (use_cache_off == -1)
+    goto err_out;
 
   if (readonly) {
     if (access (filename, R_OK) == -1) {
       perrorf (g, "%s", filename);
-      free (format);
-      free (iface);
-      free (name);
-      return -1;
+      goto err_out;
     }
   }
 
@@ -361,6 +348,12 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
   (*i)->use_cache_off = use_cache_off;
 
   return 0;
+
+err_out:
+  free (format);
+  free (iface);
+  free (name);
+  return -1;
 }
 
 int
-- 
1.7.8

1. Change the g->path to restore a absolute path instead of the mixed.
2. Check that if the adding drive is duplicated with the added drive.

Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
---
 src/launch.c |   13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index 2840767..86e6241 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -335,24 +335,33 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
     }
   }
 
+  char *abs_path = realpath (filename, NULL);
   struct drive **i = &(g->drives);
-  while (*i != NULL) i = &((*i)->next);
+  while (*i != NULL) {
+    if (STREQ((*i)->path, abs_path)) {
+      error (g, _("drive %s can't be added twice"), abs_path);
+      goto err_out;
+    }
+    i = &((*i)->next);
+  }
 
   *i = safe_malloc (g, sizeof (struct drive));
   (*i)->next = NULL;
-  (*i)->path = safe_strdup (g, filename);
+  (*i)->path = safe_strdup (g, abs_path);
   (*i)->readonly = readonly;
   (*i)->format = format;
   (*i)->iface = iface;
   (*i)->name = name;
   (*i)->use_cache_off = use_cache_off;
 
+  free (abs_path);
   return 0;
 
 err_out:
   free (format);
   free (iface);
   free (name);
+  free (abs_path);
   return -1;
 }
 
-- 
1.7.8

On Mon, Jan 09, 2012 at 03:22:41PM +0800, Wanlong Gao
wrote:
> Move the filename's comma character checking to a wrapper.
> 
> Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
> ---
>  src/launch.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/src/launch.c b/src/launch.c
> index ca89b63..8eaaac8 100644
> --- a/src/launch.c
> +++ b/src/launch.c
> @@ -277,6 +277,16 @@ valid_format_iface (const char *str)
>    return 1;
>  }
>  
> +static int
> +check_path (guestfs_h *g, const char *filename)
> +{
> +  if (strchr (filename, ',') != NULL) {
> +    error (g, _("filename cannot contain ',' (comma)
character"));
> +    return 1;
Almost every checking function like this returns -1 on error (and
0 for OK).
> +  }
> +  return 0;
> +}
> +
>  int
>  guestfs__add_drive_opts (guestfs_h *g, const char *filename,
>                           const struct guestfs_add_drive_opts_argv
*optargs)
> @@ -287,10 +297,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
>    char *name;
>    int use_cache_off;
>  
> -  if (strchr (filename, ',') != NULL) {
> -    error (g, _("filename cannot contain ',' (comma)
character"));
> +  if (check_path(g, filename))
So this would have to change also to:

  if (check_path (g, filename) == -1)
>      return -1;
> -  }
>  
>    readonly = optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK
>               ? optargs->readonly : 0;
> @@ -405,10 +413,8 @@ guestfs__add_drive_ro_with_if (guestfs_h *g, const
char *filename,
>  int
>  guestfs__add_cdrom (guestfs_h *g, const char *filename)
>  {
> -  if (strchr (filename, ',') != NULL) {
> -    error (g, _("filename cannot contain ',' (comma)
character"));
> +  if (check_path(g, filename))
>      return -1;
> -  }
>  
>    if (access (filename, F_OK) == -1) {
>      perrorf (g, "%s", filename);
> -- 
> 1.7.8
But basically this patch is fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v