[Libguestfs] [PATCH 3/2] lib: remove all temporary directories may remained last launching

[Richard W.M. Jones]

On Fri, Dec 23, 2011 at 10:38:10PM +0800, Wanlong Gao wrote:
> On 12/23/2011 10:22 PM, Wanlong Gao wrote:
> 
> > On 12/23/2011 10:17 PM, Richard W.M. Jones wrote:
> > 
> >> On Fri, Dec 23, 2011 at 10:06:37PM +0800, Wanlong Gao wrote:
> >>> If guestfish or other progs which launching appliance was aborted or
> >>> killed last time, the temp dir will be remained, so delete it when
> >>> this time launching.
> >>> Prevent the possible waste of disk space.
> >>>
> >>> Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
> >>> ---
> >>>  src/appliance.c |   10 ++++++++++
> >>>  1 files changed, 10 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/src/appliance.c b/src/appliance.c
> >>> index 57ff38f..4ce8405 100644
> >>> --- a/src/appliance.c
> >>> +++ b/src/appliance.c
> >>> @@ -36,6 +36,8 @@
> >>>  #include <sys/types.h>
> >>>  #endif
> >>>  
> >>> +#include "ignore-value.h"
> >>> +
> >>>  #include "guestfs.h"
> >>>  #include "guestfs-internal.h"
> >>>  #include "guestfs-internal-actions.h"
> >>> @@ -437,6 +439,14 @@ build_supermin_appliance (guestfs_h *g,
> >>>     */
> >>>    size_t len = strlen (tmpdir) + 128;
> >>>  
> >>> +  /* If guestfish or other progs which launching appliance was aborted or
> >>> +   * killed last time, the temp dir will be remained, so delete it when
> >>> +   * this time launching.
> >>> +   */
> >>> +  char cmd[len];
> >>> +  snprintf (cmd, len, "rm -rf %s/guestfs.??????", tmpdir);
> >>> +  ignore_value (system (cmd));
> >>> +
> >>>    /* Build the appliance into a temporary directory. */
> >>>    char tmpcd[len];
> >>>    snprintf (tmpcd, len, "%s/guestfs.XXXXXX", tmpdir);
> >>
> >> This isn't safe.  It'll remove unrelated guestfs.?????? directories
> >> that might be in use by other processes.
> > 
> > 
> > Yeah, you are right.
> > 
> >>
> >> Almost every Linux system out there has a /tmp cleaner process which
> >> will clean up anything that the current system doesn't catch.
> > 
> > 
> > But I can just produce this by kill the guestfish when it's launching.
> > 
> > And, can we get the status of which tempdir is in use?
> 
> 
> Can we take a lock here?

I think the real place to fix this is by keeping the tmpdir in the
handle, and cleaning it up during handle cleanup.  Handle cleanup
happens even if you ^C the process (since there is an atexit handler).

Now it's a bit more complicated than this ...

At the moment, each libguestfs handle can use one or two temporary
directories.

The "normal" g->tmpdir is created early on and the directory name is
kept in the handle, and it is cleaned up when the handle is closed.
This is used to store the virtio-serial socket and some temporary
files like the Windows Registry during inspection.

The normal g->tmpdir is not used for appliance building however.  For
appliance building another tmpdir is created -- and this is the one
which RHBZ#769680 is talking about.

The reason the appliance builder doesn't use g->tmpdir is because the
appliance tmpdir must be on the same filesystem as the filesystem we
use to store the cached appliance.  The cached appliance is stored in
/var/tmp/.guestfs-<UUID>.  g->tmpdir is created in /tmp by default.
So we have to create this second appliance-building tmpdir in
/var/tmp. [1]

So a solution to this would be to add g->persistent_tmpdir to the
handle.  It would act similarly to g->tmpdir, being cleaned up in
guestfs_close.

Anyway, this solution is quite complicated to implement, or at least,
more complicated than I was prepared to do for now.

Rich.

[1] Both tmpdir paths can be changed by setting TMPDIR, and the reason
for using separate paths is because of the LSB -- see commit
78f1405de05ef1f2efebafd8245658d1707e59ef.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top