10:06 a.m.
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.