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

[Maros Zatko]

On 05/21/2015 12:16 PM, Pino Toscano wrote:
> 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.
Issue was speed. Without inspection (i.e. -m is given) it takes
3 seconds on my machine. With inspection it takes for 6-8 seconds.
If this is a non-issue, I'll add it. Only inspect_os needed for this to 
work.
>
>>   
>>     /* 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.
I've specifically took it out to it's own function. It's more clear to 
me like this.
> Another option could be factor out this code somewhere (like windows.c).
I like this idea better. cat/cat.c could maybe use it too.