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

[Pino Toscano]

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