[PATCH libguestfs] generator.ml: constify do_mkdtemp
Not useful from a testing standpoint (since this presumes a metric ton of other changes), but mostly a heads-up where I'm going.
From 62cbebffe9859046019b500e4bcdb0e44e575755 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 11 Aug 2009 17:17:08 +0200 Subject: [PATCH libguestfs] generator.ml: constify do_mkdtemp
* daemon/dir.c (do_mkdtemp): Rewrite for a "const" parameter. * src/generator.ml (mkdtemp): Declare parameter to be of type Pathname. --- daemon/dir.c | 27 ++++++++------------------- src/generator.ml | 8 +++----- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/daemon/dir.c b/daemon/dir.c index ad1c7c9..21ae07e 100644 --- a/daemon/dir.c +++ b/daemon/dir.c @@ -185,31 +185,20 @@ do_is_dir (const char *path) } char * -do_mkdtemp (char *template) +do_mkdtemp (const char *template) { - char *r; - - NEED_ROOT (return NULL); - ABS_PATH (template, return NULL); + char *r = strdup (template); + if (r == NULL) { + reply_with_perror ("strdup"); + return NULL; + } CHROOT_IN; - r = mkdtemp (template); + r = mkdtemp (r); CHROOT_OUT; - if (r == NULL) { + if (r == NULL) reply_with_perror ("mkdtemp: %s", template); - return NULL; - } - /* The caller will free template AND try to free the return value, - * so we must make a copy here. - */ - if (r == template) { - r = strdup (template); - if (r == NULL) { - reply_with_perror ("strdup"); - return NULL; - } - } return r; } diff --git a/src/generator.ml b/src/generator.ml index 8dc9261..3c7512f 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -2594,8 +2594,7 @@ containing C<dir>. It is an interface to the L<scrub(1)> program. See that manual page for more details."); -(* FIXME: make this a WritableString? *) - ("mkdtemp", (RString "dir", [String "template"]), 117, [], + ("mkdtemp", (RString "dir", [Pathname "template"]), 117, [], [InitBasicFS, Always, TestRun ( [["mkdir"; "/tmp"]; ["mkdtemp"; "/tmp/tmpXXXXXX"]])],
On Tue, Aug 11, 2009 at 05:20:37PM +0200, Jim Meyering wrote:
Not useful from a testing standpoint (since this presumes a metric ton of other changes), but mostly a heads-up where I'm going. + char *r = strdup (template); + if (r == NULL) { + reply_with_perror ("strdup"); + return NULL; + }
CHROOT_IN; - r = mkdtemp (template); + r = mkdtemp (r); CHROOT_OUT;
- if (r == NULL) { + if (r == NULL) reply_with_perror ("mkdtemp: %s", template); - return NULL; - }
- /* The caller will free template AND try to free the return value, - * so we must make a copy here. - */ - if (r == template) { - r = strdup (template); - if (r == NULL) { - reply_with_perror ("strdup"); - return NULL; - } - } return r;
I think 'r' gets leaked along the error path (when mkdtemp returns NULL). Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.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/
Richard W.M. Jones wrote:
On Tue, Aug 11, 2009 at 05:20:37PM +0200, Jim Meyering wrote:
Not useful from a testing standpoint (since this presumes a metric ton of other changes), but mostly a heads-up where I'm going. + char *r = strdup (template); + if (r == NULL) { + reply_with_perror ("strdup"); + return NULL; + }
CHROOT_IN; - r = mkdtemp (template); + r = mkdtemp (r); CHROOT_OUT;
- if (r == NULL) { + if (r == NULL) reply_with_perror ("mkdtemp: %s", template); - return NULL; - }
- /* The caller will free template AND try to free the return value, - * so we must make a copy here. - */ - if (r == template) { - r = strdup (template); - if (r == NULL) { - reply_with_perror ("strdup"); - return NULL; - } - } return r;
I think 'r' gets leaked along the error path (when mkdtemp returns NULL).
Good catch! Here's the new version: char * do_mkdtemp (const char *template) { char *writable = strdup (template); if (writable == NULL) { reply_with_perror ("strdup"); return NULL; } CHROOT_IN; char *r = mkdtemp (writable); CHROOT_OUT; if (r == NULL) { reply_with_perror ("mkdtemp: %s", template); free (writable); } return r; }
On Wed, Aug 12, 2009 at 09:36:20AM +0200, Jim Meyering wrote:
Here's the new version:
char * do_mkdtemp (const char *template) { char *writable = strdup (template); if (writable == NULL) { reply_with_perror ("strdup"); return NULL; }
CHROOT_IN; char *r = mkdtemp (writable); CHROOT_OUT;
if (r == NULL) { reply_with_perror ("mkdtemp: %s", template); free (writable); }
return r; }
Looks good to me - ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.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
participants (2)
-
Jim Meyering -
Richard W.M. Jones