2:26 a.m.
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).
2:26 a.m.
New subject: [PATCH v2] lib/info: Remove /dev/fd hacking and pass a true filename to qemu-img info.
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
8:32 a.m.
New subject: [PATCH v2] lib/info: Remove /dev/fd hacking and pass a true filename to qemu-img info.
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
1:37 p.m.
New subject: [PATCH v2] lib/info: Remove /dev/fd hacking and pass a true filename to qemu-img info.
On Fri, Jan 12, 2018 at 03:32:31PM +0100, Pino Toscano wrote:
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?
I don't think so. Previously we needed it because we wanted to pass
the open fd file descriptor to the qemu-img subprocess. However as
we're now passing the filename I don't believe it is needed (ie. we
_do_ need to close other file descriptors).
> + 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.
Ah, good point.
I'm going to push this with the second change but not the first.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
2309
days inactive
2348
days old
3 comments
2 participants
participants (2)
-
Pino Toscano -
Richard W.M. Jones