[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@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@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
On Mon, Jan 09, 2012 at 03:22:42PM +0800, Wanlong Gao wrote:
Code cleanup. Add a goto label to simplify the code.
Thanks, I've applied this. 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
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@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:43PM +0800, Wanlong Gao wrote:
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.
We were trying to work out if there is a valid case where you might add the same drive twice. I can't think of one at the moment ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On 01/09/2012 06:28 PM, Richard W.M. Jones wrote:
On Mon, Jan 09, 2012 at 03:22:43PM +0800, Wanlong Gao wrote:
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.
We were trying to work out if there is a valid case where you might add the same drive twice. I can't think of one at the moment ...
But, adding the same drive twice or more will cause many problems in qemu, So, why not just add only once, it'll be better and be consistent with the real drives. If not, at least, IMHO, store a absolute path is better then the origin filename we passed to the add_drive_* functions. Agree? Thanks -Wanlong Gao
Rich.
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@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
From: Wanlong Gao <gaowanlong@cn.fujitsu.com> Move the filename's comma character checking to a wrapper. Signed-off-by: Wanlong Gao <gaowanlong@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) == -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) == -1) return -1; - } if (access (filename, F_OK) == -1) { perrorf (g, "%s", filename); -- 1.7.8
Thanks, I've pushed all of these, with some modifications to fix things found by gcc warnings and the tests. Please use './configure --enable-gcc-warnings' and 'make check'! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On 01/09/2012 10:09 PM, Richard W.M. Jones wrote:
Thanks, I've pushed all of these, with some modifications to fix things found by gcc warnings and the tests.
Please use './configure --enable-gcc-warnings' and 'make check'!
oooooooops, I see, will do next time. Thanks a lot Rich. -Wanlong Gao
Rich.
participants (3)
-
Richard W.M. Jones -
Wanlong Gao -
Wanlong Gao