[Libguestfs] [PATCH 07/12] btrfs: Update btrfs_subvolume_list to take Mountable_or_Path
Richard W.M. Jones
rjones at redhat.com
Fri Feb 8 12:10:04 UTC 2013
On Thu, Feb 07, 2013 at 03:57:53PM +0000, Matthew Booth wrote:
> + CLEANUP_FREE char *fs_buf = NULL;
> +
> + if (fs->type == MOUNTABLE_PATH) {
> + fs_buf = sysroot_path (fs->device);
> + if (fs_buf == NULL) {
> + reply_with_perror ("malloc");
> +
> + cmderror:
> + if (fs->type != MOUNTABLE_PATH && fs_buf) {
> + CLEANUP_FREE char *err = NULL;
> + if (umount_internal (fs_buf, 1, 0, &err) == -1)
> + fprintf (stderr, "%s\n", err);
> +
> + if (rmdir (fs_buf) == -1 && errno != ENOENT)
> + fprintf (stderr, "rmdir: %m\n");
> + }
> + return NULL;
> + }
> + }
A suggestion for this rather complex cleanup code: There is already a
macro 'CLEANUP_UNLINK_FREE' which unlinks and frees a buffer. I have
no objection to further CLEANUP_* macros for more specialized uses,
eg. unmount + rmdir. Of course it would have to be in a separate
patch.
> + else {
> + fs_buf = strdup ("/tmp/btrfs.XXXXXX");
> + if (fs_buf == NULL) {
> + reply_with_perror ("strdup");
> + goto cmderror;
> + }
> +
> + if (mkdtemp (fs_buf) == NULL) {
> + reply_with_perror ("mkdtemp");
> + goto cmderror;
> + }
> +
> + CLEANUP_FREE char *err = NULL;
> + if (mount_vfs_internal ("", NULL, fs, fs_buf, "<internal>", &err) == -1) {
> + reply_with_error ("%s", err ? err : "malloc");
> + goto cmderror;
> + }
> }
>
> size_t i = 0;
> @@ -328,16 +361,33 @@ do_btrfs_subvolume_list (const char *fs)
> ADD_ARG (argv, i, fs_buf);
> ADD_ARG (argv, i, NULL);
>
> - CLEANUP_FREE char *out = NULL, *err = NULL;
> - int r = commandv (&out, &err, argv);
> + CLEANUP_FREE char *out = NULL, *errout = NULL;
> + int r = commandv (&out, &errout, argv);
> +
> + if (fs->type != MOUNTABLE_PATH) {
> + CLEANUP_FREE char *err = NULL;
> + if (umount_internal (fs_buf, 1, 0, &err) == -1) {
> + reply_with_error ("%s", err ? err : "malloc");
> + goto cmderror;
> + }
> +
> + if (rmdir (fs_buf) == -1 && errno != ENOENT) {
> + reply_with_error ("rmdir: %m\n");
> + goto cmderror;
> + }
> + }
> +
> if (r == -1) {
> - reply_with_error ("%s: %s", fs, err);
> - return NULL;
> + CLEANUP_FREE char *fs_desc = mountable_to_string (fs);
> + if (fs_desc == NULL) {
> + fprintf (stderr, "malloc: %m");
> + }
> + reply_with_error ("%s: %s", fs_desc ? fs_desc : "malloc", errout);
> + goto cmderror;
> }
>
> lines = split_lines (out);
> - if (!lines)
> - return NULL;
> + if (!lines) return NULL;
> }
>
> /* Output is:
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 4f613db..86f58ea 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -9514,7 +9514,7 @@ directory and the name of the snapshot, in the form C</path/to/dest/name>." };
>
> { defaults with
> name = "btrfs_subvolume_list";
> - style = RStructList ("subvolumes", "btrfssubvolume"), [Pathname "fs"], [];
> + style = RStructList ("subvolumes", "btrfssubvolume"), [Mountable_or_Path "fs"], [];
> proc_nr = Some 325;
> optional = Some "btrfs"; camel_name = "BTRFSSubvolumeList";
> tests = [] (* tested in tests/btrfs *);
Patch seems reasonable, so ACK, but better cleanup would make it
easier to understand.
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 KVM guests.
http://libguestfs.org/virt-v2v
More information about the Libguestfs
mailing list