search for: commandvf

Use commandrvf() instead of commandvf() to execute e2fsck. A non-zero exit status does not always indicate a failure. Signed-off-by: Nikos Skalkotos <skalkoto@grnet.gr> --- daemon/ext2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/ext2.c b/daemon/ext2.c index 65ddae6..8ef6d5f 100644 --- a/da...

On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik wrote: > CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be > done in the child, which also removes the need to chroot out in the > parent. The CHROOT_IN/OUT business does need to be rewritten. Every instance where we currently do something like: CHROOT_IN; r = stat (fd, &statbuf); CHROOT_OUT...

..._mount (&bind_state) == -1) return NULL; if (enable_network) { @@ -266,8 +279,10 @@ do_command (char *const *argv) return NULL; } + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; + CHROOT_IN; - r = commandv (&out, &err, (const char * const *) argv); + r = commandvf (&out, &err, flags, (const char * const *) argv); CHROOT_OUT; free_bind_state (&bind_state); -- 2.1.0

On Tue, Dec 01, 2015 at 04:58:11PM +0000, Richard W.M. Jones wrote: > On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik wrote: > > CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be > > done in the child, which also removes the need to chroot out in the > > parent. > > The CHROOT_IN/OUT business does need to be rewritten. Every > instance where we currently do something like: > > CHROOT_IN; > r =...

...nable_network) { > @@ -266,8 +279,10 @@ do_command (char *const *argv) > return NULL; > } > nit: same. > + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; > + > CHROOT_IN; > - r = commandv (&out, &err, (const char * const *) argv); > + r = commandvf (&out, &err, flags, (const char * const *) argv); > CHROOT_OUT; > > free_bind_state (&bind_state); According to the analysis in https://bugzilla.redhat.com/show_bug.cgi?id=1280029 the problem was that the target program was being executed in a chroot which did not have...

...(s = va_arg (args, char *)) != NULL) { + const char **p = realloc (argv, sizeof (char *) * (++i)); + if (p == NULL) { + perror ("realloc"); + va_end (args); + return -1; + } + argv = p; + argv[i-2] = s; + argv[i-1] = NULL; + } + + va_end (args); + + r = commandvf (stdoutput, stderror, flags, (const char * const*) argv); + + return r; +} + +/* Same as 'command', but we allow the status code from the + * subcommand to be non-zero, and return that status code. + * We still return -1 if there was some other error. + */ +int +commandrf (char **stdoutput...

...insertion(+), 1 deletion(-) diff --git a/daemon/xfs.c b/daemon/xfs.c index abc2736..7f72e6a 100644 --- a/daemon/xfs.c +++ b/daemon/xfs.c @@ -537,7 +537,7 @@ do_xfs_admin (const char *device, ADD_ARG (argv, i, device); ADD_ARG (argv, i, NULL); - r = commandv (NULL, &err, argv); + r = commandvf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, argv); if (r == -1) { reply_with_error ("%s: %s", device, err); return -1; -- 2.5.0

...const char *name, ...); + const char *name, ...) __attribute__((sentinel)); extern int commandrf (char **stdoutput, char **stderror, int flags, - const char *name, ...); + const char *name, ...) __attribute__((sentinel)); extern int commandvf (char **stdoutput, char **stderror, int flags, char const *const *argv); extern int commandrvf (char **stdoutput, char **stderror, int flags, -- 1.7.11.7

...diff --git a/daemon/ntfs.c b/daemon/ntfs.c index aef45a2..762ca88 100644 --- a/daemon/ntfs.c +++ b/daemon/ntfs.c @@ -94,7 +94,7 @@ do_ntfsresize (const char *device, int64_t size, int force) ADD_ARG (argv, i, device); ADD_ARG (argv, i, NULL); - r = commandv (NULL, &err, argv); + r = commandvf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, argv); if (r == -1) { reply_with_error ("%s: %s", device, err); return -1; -- 2.1.0

...} > > > > nit: same. Both these leaks need to be fixed, thanks for reporting them. > > + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; > > + > > CHROOT_IN; > > - r = commandv (&out, &err, (const char * const *) argv); > > + r = commandvf (&out, &err, flags, (const char * const *) argv); > > CHROOT_OUT; > > > > free_bind_state (&bind_state); > > According to the analysis in > https://bugzilla.redhat.com/show_bug.cgi?id=1280029 the problem was that > the target program was being exec...

...ULL; - } - if (bind_mount (&bind_state) == -1) return NULL; if (enable_network) { @@ -279,11 +268,9 @@ do_command (char *const *argv) return NULL; } - flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | dev_null_fd; + flags = COMMAND_FLAG_DO_CHROOT; - CHROOT_IN; r = commandvf (&out, &err, flags, (const char * const *) argv); - CHROOT_OUT; free_bind_state (&bind_state); free_resolver_state (&resolver_state); diff --git a/daemon/daemon.h b/daemon/daemon.h index 7fbb2a2..af6f68c 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -128,6 +128,7 @@...

...er, do_command sets up /etc/resolv.conf for the "guest". My argument is that executing linux binaries in an environment without properly populated /dev is a recipe for trouble and will one day come back with weird failures or improperly constructed images. > > CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be > > done in the child, which also removes the need to chroot out in the > > parent. > > That could be another way to make the command* functions in the daemon > safer, which wouldn't solve the issue of this email thread: even if...

...int commandf (char **stdoutput, char **stderror, int flags, - const char *name, ...); + const char *name, ...); extern int commandrf (char **stdoutput, char **stderror, int flags, - const char *name, ...); + const char *name, ...); extern int commandvf (char **stdoutput, char **stderror, int flags, char const *const *argv); extern int commandrvf (char **stdoutput, char **stderror, int flags, - char const* const *argv); + char const* const *argv); extern char **split_lines (char *str); diff...

...> if (enable_network) { > @@ -266,8 +279,10 @@ do_command (char *const *argv) > return NULL; > } > > + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; > + > CHROOT_IN; > - r = commandv (&out, &err, (const char * const *) argv); > + r = commandvf (&out, &err, flags, (const char * const *) argv); > CHROOT_OUT; > > free_bind_state (&bind_state); Looks good. How about naming the variable 'dev_null_fd' or something, so we know it's not just a temporary file descriptor variable? Anyway, ACK. Rich. --...

...zilla.redhat.com/1195881). > Filesystems in the container would not be fully populated (see: /dev/mem > & friends), but only have 'container-friendly' files. > > That's definitely a lot of work and I'm not up to the task. Right - patches welcome! > Regardless, commandvf vs chroot usage can be improved without said > significant work. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a...

...> return NULL; > if (enable_network) { > @@ -279,11 +268,9 @@ do_command (char *const *argv) > return NULL; > } > > - flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | dev_null_fd; > + flags = COMMAND_FLAG_DO_CHROOT; > > - CHROOT_IN; > r = commandvf (&out, &err, flags, (const char * const *) argv); > - CHROOT_OUT; > > free_bind_state (&bind_state); > free_resolver_state (&resolver_state); > diff --git a/daemon/daemon.h b/daemon/daemon.h > index 7fbb2a2..af6f68c 100644 > --- a/daemon/daemon.h > +...

...reply_with_error ("passed an empty list"); - return NULL; - } - - if (bind_mount (&bind_state) == -1) - return NULL; - if (enable_network) { - if (set_up_etc_resolv_conf (&resolver_state) == -1) - return NULL; - } - - flags = COMMAND_FLAG_DO_CHROOT; - - r = commandvf (&out, &err, flags, (const char * const *) argv); - - free_bind_state (&bind_state); - free_resolver_state (&resolver_state); - - if (r == -1) { - reply_with_error ("%s", err); - free (out); - return NULL; - } - - return out; /* Caller frees. */ -} - -char...

This is a more working version. Inspection (partially) succeeds on a real guest this time :-) You can test it out on a real guest (in this case, a CentOS disk image located at /tmp/centos-6.img) by doing: $ ./run guestfish -v -x -a /tmp/centos-6.img ><fs> run ><fs> debug sh "guestfs-inspection --verbose" which will print lots of debugging, and at the end the

For background on this change, see: https://rwmj.wordpress.com/2015/12/06/inspection-now-with-added-prolog/ v2 was previously posted here: https://www.redhat.com/archives/libguestfs/2015-December/msg00038.html To test this patch series on a real guest, you can do: $ ./run guestfish -v -x -a /var/tmp/centos-6.img ><fs> run ><fs> debug sh "guestfs-inspection

These two patches are probably not completely independent, but separating them is a lot of work. With *both* patches applied, all the tests and extra-tests pass. That's no guarantee however that there isn't a mistake, so I don't think this patch is a candidate for the 1.16 branch, until it's had a lot more testing in development. Rich.