Libguestfs - May 2015 - Re: [PATCH v3 1/3] virt-ls: support drive letters on Windows

Fixes RHBZ#845234.

v3 changes: Drive letters works if inspection is enabled (-m is not given)
v2 changes: Ammended so it doesn't do inspection for every dir to list.

Maros Zatko (3):
  virt-ls: support drive letters on Windows
  virt-ls: update usage for win drive letters
  docs: amend virt-ls manpage with win drive letters

 cat/ls.c        | 41 +++++++++++++++++++++++++++++++++++++----
 cat/virt-ls.pod |  7 ++++++-
 2 files changed, 43 insertions(+), 5 deletions(-)

-- 
1.9.3

Directory name can include Windows drive letter if guest
is Windows and inspection is enabled (i.e. option -m is not given).

Fixes: RHBZ#845234
---
 cat/ls.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/cat/ls.c b/cat/ls.c
index 9161fb6..1a164b3 100644
--- a/cat/ls.c
+++ b/cat/ls.c
@@ -37,6 +37,7 @@
 
 #include "options.h"
 #include "visit.h"
+#include "windows.h"
 
 /* Currently open libguestfs handle. */
 guestfs_h *g;
@@ -76,6 +77,8 @@ static void output_int64_uid (int64_t);
 static void output_string (const char *);
 static void output_string_link (const char *);
 
+static const char *get_windows_root ();
+
 static void __attribute__((noreturn))
 usage (int status)
 {
@@ -176,6 +179,7 @@ main (int argc, char *argv[])
 #define MODE_LS_R  2
 #define MODE_LS_LR (MODE_LS_L|MODE_LS_R)
   int mode = 0;
+  CLEANUP_FREE const char * win_root;
 
   g = guestfs_create ();
   if (g == NULL) {
@@ -362,19 +366,28 @@ main (int argc, char *argv[])
   if (guestfs_launch (g) == -1)
     exit (EXIT_FAILURE);
 
-  if (mps != NULL)
+  if (mps != NULL) {
     mount_mps (mps);
-  else
+  } else {
     inspect_mount ();
+    win_root = get_windows_root ();
+  }
 
   /* Free up data structures, no longer needed after this point. */
   free_drives (drvs);
   free_mps (mps);
 
+
   unsigned errors = 0;
 
   while (optind < argc) {
-    const char *dir = argv[optind];
+    CLEANUP_FREE const char *dir;
+
+    if (inspector == 1 && win_root != NULL) {
+      dir = windows_path (g, win_root, argv[optind], 1 /* readonly */ );
+    } else {
+      dir = safe_strdup (g, argv[optind]);
+    }
 
     switch (mode) {
     case 0:                     /* no -l or -R option */
@@ -409,6 +422,24 @@ main (int argc, char *argv[])
   exit (errors == 0 ? EXIT_SUCCESS : EXIT_FAILURE);
 }
 
+static const char *
+get_windows_root (void)
+{
+  CLEANUP_FREE_STRING_LIST char **roots = NULL;
+  const char *root;
+
+  roots = guestfs_inspect_get_roots (g);
+  assert (roots);
+  assert (roots[0] != NULL);
+  assert (roots[1] == NULL);
+  root = safe_strdup(g, roots[0]);
+
+  if (is_windows (g, root))
+    return root;
+  else
+    return NULL;
+}
+
 static int
 do_ls (const char *dir)
 {
-- 
1.9.3

Relates to: RHBZ#845234
---
 cat/ls.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cat/ls.c b/cat/ls.c
index 1a164b3..e3abdda 100644
--- a/cat/ls.c
+++ b/cat/ls.c
@@ -106,7 +106,9 @@ usage (int status)
              "  --keys-from-stdin    Read passphrases from stdin\n"
              "  -l|--long            Long listing\n"
              "  -m|--mount dev[:mnt[:opts[:fstype]]]\n"
-             "                       Mount dev on mnt (if omitted,
/)\n"
+             "                       Mount dev on mnt (if omitted,
/).\n"
+             "                       This also disables inspection so
Windows\n"
+             "                       drive letters cannot be used.\n"
              "  -R|--recursive       Recursive listing\n"
              "  --times              Display file times\n"
              "  --time-days          Display file times as days before
now\n"
-- 
1.9.3

Relates to: RHBZ#845234
---
 cat/virt-ls.pod | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/cat/virt-ls.pod b/cat/virt-ls.pod
index aa7919d..e96c065 100644
--- a/cat/virt-ls.pod
+++ b/cat/virt-ls.pod
@@ -22,6 +22,9 @@ and more from a virtual machine or disk image.
 Multiple directory names can be given, in which case the output from
 each is concatenated.
 
+Directory name can include Windows drive letter if guest is Windows
+and option I<-m> is not given.
+
 To list directories from a libvirt guest use the I<-d> option to
 specify the name of the guest.  For a disk image, use the I<-a>
 option.
@@ -368,7 +371,9 @@ If the mountpoint is omitted, it defaults to C</>.
 Specifying any mountpoint disables the inspection of the guest and
 the mount of its root and all of its mountpoints, so make sure
 to mount all the mountpoints needed to work with the filenames
-given as arguments.
+given as arguments. Consequence of disabled inspection is faster
+execution and inability to use Windows drive letters in directory
+names.
 
 If you don't know what filesystems a disk image contains, you can
 either run guestfish without this option, then list the partitions,
-- 
1.9.3

On Wednesday 20 May 2015 19:41:47 Maros Zatko wrote:
> Directory name can include Windows drive letter if guest
> is Windows and inspection is enabled (i.e. option -m is not given).
> 
> Fixes: RHBZ#845234
Please move the bug notice directly in the first line of the commit
message, e.g.:

 virt-ls: support drive letters on Windows (RHBZ#845234)
> ---
>  cat/ls.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/cat/ls.c b/cat/ls.c
> index 9161fb6..1a164b3 100644
> --- a/cat/ls.c
> +++ b/cat/ls.c
> @@ -37,6 +37,7 @@
>  
>  #include "options.h"
>  #include "visit.h"
> +#include "windows.h"
>  
>  /* Currently open libguestfs handle. */
>  guestfs_h *g;
> @@ -76,6 +77,8 @@ static void output_int64_uid (int64_t);
>  static void output_string (const char *);
>  static void output_string_link (const char *);
>  
> +static const char *get_windows_root ();
> +
>  static void __attribute__((noreturn))
>  usage (int status)
>  {
> @@ -176,6 +179,7 @@ main (int argc, char *argv[])
>  #define MODE_LS_R  2
>  #define MODE_LS_LR (MODE_LS_L|MODE_LS_R)
>    int mode = 0;
> +  CLEANUP_FREE const char * win_root;
Take care of setting CLEANUP_FREE variables to NULL, otherwise the
cleanup routine will try to free a garbage pointer.
>    g = guestfs_create ();
>    if (g == NULL) {
> @@ -362,19 +366,28 @@ main (int argc, char *argv[])
>    if (guestfs_launch (g) == -1)
>      exit (EXIT_FAILURE);
>  
> -  if (mps != NULL)
> +  if (mps != NULL) {
>      mount_mps (mps);
> -  else
> +  } else {
>      inspect_mount ();
> +    win_root = get_windows_root ();
> +  }
Hm why doing that only when inspecting? Other tools (e.g. virt-cat)
do this also when specifying mount points.
>  
>    /* Free up data structures, no longer needed after this point. */
>    free_drives (drvs);
>    free_mps (mps);
>  
> +
>    unsigned errors = 0;
>  
>    while (optind < argc) {
> -    const char *dir = argv[optind];
> +    CLEANUP_FREE const char *dir;
> +
> +    if (inspector == 1 && win_root != NULL) {
> +      dir = windows_path (g, win_root, argv[optind], 1 /* readonly */ );
> +    } else {
> +      dir = safe_strdup (g, argv[optind]);
> +    }
>  
>      switch (mode) {
>      case 0:                     /* no -l or -R option */
> @@ -409,6 +422,24 @@ main (int argc, char *argv[])
>    exit (errors == 0 ? EXIT_SUCCESS : EXIT_FAILURE);
>  }
>  
> +static const char *
Given a new string is returned, the return value needs to not be const.
> +get_windows_root (void)
> +{
> +  CLEANUP_FREE_STRING_LIST char **roots = NULL;
> +  const char *root;
> +
> +  roots = guestfs_inspect_get_roots (g);
> +  assert (roots);
> +  assert (roots[0] != NULL);
> +  assert (roots[1] == NULL);
> +  root = safe_strdup(g, roots[0]);
safe_strdup could be delayed until the actual return later, otherwise
if is_windows returns false that string is leaked.
> +
> +  if (is_windows (g, root))
> +    return root;
> +  else
> +    return NULL;
> +}
> +
I'd inline the whole get_windows_root just before the
"while (optind < argc)", like done in cat/cat.c:do_cat.

Another option could be factor out this code somewhere (like windows.c).

-- 
Pino Toscano

On Wednesday 20 May 2015 19:41:46 Maros Zatko wrote:
> Fixes RHBZ#845234.
> 
> v3 changes: Drive letters works if inspection is enabled (-m is not given)
> v2 changes: Ammended so it doesn't do inspection for every dir to list.
> 
> Maros Zatko (3):
>   virt-ls: support drive letters on Windows
>   virt-ls: update usage for win drive letters
>   docs: amend virt-ls manpage with win drive letters
Common note regarding patches 2 & 3: documentation changes IMHO fit in
the same commit that does the change they are describing, as it is more
like a single change.

-- 
Pino Toscano

On 05/21/2015 12:18 PM, Pino Toscano wrote:
> On Wednesday 20 May 2015 19:41:46 Maros Zatko wrote:
>> Fixes RHBZ#845234.
>>
>> v3 changes: Drive letters works if inspection is enabled (-m is not
given)
>> v2 changes: Ammended so it doesn't do inspection for every dir to
list.
>>
>> Maros Zatko (3):
>>    virt-ls: support drive letters on Windows
>>    virt-ls: update usage for win drive letters
>>    docs: amend virt-ls manpage with win drive letters
> Common note regarding patches 2 & 3: documentation changes IMHO fit in
> the same commit that does the change they are describing, as it is more
> like a single change.
>
I've been told a few times I should split patches/commits like this one.
So, if you insist I can merge it, but I'm getting a bit confused about 
this :)

Thanks for review!

-m