Libguestfs - Jan 2018 - Re: [PATCH v2] lib/info: Remove /dev/fd hacking and pass a true filename to qemu-img info.

v1 posted here:
https://www.redhat.com/archives/libguestfs/2017-December/msg00050.html

v2 fixes the tests by rewriting relative paths as ./filename
(so that filenames like "file:foo" work).

It obscures what's really going on and is no longer necessary
for the original purpose.

This reverts commit d50cb7bbb4cc18f69ea1425e9f5cee9685825f95.

See also:

  https://www.redhat.com/archives/libguestfs/2017-November/thread.html#00226
  https://www.redhat.com/archives/libguestfs/2017-December/thread.html#00044
---
 lib/info.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/lib/info.c b/lib/info.c
index f7378adfd..c58b3ebfb 100644
--- a/lib/info.c
+++ b/lib/info.c
@@ -24,7 +24,6 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <sys/wait.h>
 #include <assert.h>
 #include <string.h>
@@ -166,41 +165,24 @@ static yajl_val
 get_json_output (guestfs_h *g, const char *filename)
 {
   CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
-  int fd, r;
-  char fdpath[64];
+  CLEANUP_FREE char *rel_filename = NULL;
+  int r;
   yajl_val tree = NULL;
-  struct stat statbuf;
-
-  fd = open (filename, O_RDONLY /* NB: !O_CLOEXEC */);
-  if (fd == -1) {
-    perrorf (g, "disk info: %s", filename);
-    return NULL;
-  }
-
-  if (fstat (fd, &statbuf) == -1) {
-    perrorf (g, "disk info: fstat: %s", filename);
-    close (fd);
-    return NULL;
-  }
-  if (S_ISDIR (statbuf.st_mode)) {
-    error (g, "disk info: %s is a directory", filename);
-    close (fd);
-    return NULL;
-  }
-
-  snprintf (fdpath, sizeof fdpath, "/dev/fd/%d", fd);
-  guestfs_int_cmd_clear_close_files (cmd);
 
   guestfs_int_cmd_add_arg (cmd, "qemu-img");
   guestfs_int_cmd_add_arg (cmd, "info");
   guestfs_int_cmd_add_arg (cmd, "--output");
   guestfs_int_cmd_add_arg (cmd, "json");
-  guestfs_int_cmd_add_arg (cmd, fdpath);
+  if (filename[0] == '/')
+    guestfs_int_cmd_add_arg (cmd, filename);
+  else {
+    rel_filename = safe_asprintf (g, "./%s", filename);
+    guestfs_int_cmd_add_arg (cmd, rel_filename);
+  }
   guestfs_int_cmd_set_stdout_callback (cmd, parse_json, &tree,
                                        CMD_STDOUT_FLAG_WHOLE_BUFFER);
   set_child_rlimits (cmd);
   r = guestfs_int_cmd_run (cmd);
-  close (fd);
   if (r == -1)
     return NULL;
   if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
-- 
2.15.1

On Friday, 15 December 2017 09:26:29 CET Richard W.M. Jones
wrote:
> It obscures what's really going on and is no longer necessary
> for the original purpose.
> 
> This reverts commit d50cb7bbb4cc18f69ea1425e9f5cee9685825f95.
> 
> See also:
> 
>  
https://www.redhat.com/archives/libguestfs/2017-November/thread.html#00226
>  
https://www.redhat.com/archives/libguestfs/2017-December/thread.html#00044
> ---
Mostly LGTM -- a couple of notes below.
> -  guestfs_int_cmd_clear_close_files (cmd);
Don't we want this regardless?
> +  if (filename[0] == '/')
> +    guestfs_int_cmd_add_arg (cmd, filename);
> +  else {
> +    rel_filename = safe_asprintf (g, "./%s", filename);
> +    guestfs_int_cmd_add_arg (cmd, rel_filename);
There's guestfs_int_cmd_add_arg_format, so that can be used to avoid
the temporary string.

Thanks,
-- 
Pino Toscano