[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]