[Libguestfs] [PATCH] daemon: improve internal commandrvf

Pino Toscano ptoscano at redhat.com
Wed Dec 2 18:35:24 UTC 2015


On Wednesday 02 December 2015 17:44:13 Mateusz Guzik wrote:
> On Wed, Dec 02, 2015 at 02:00:57PM +0100, Pino Toscano wrote:
> > - add a flag to request chroot for the process, which is done only as
> >   very last (before chdir) operation before exec'ing the process in the
> >   child: this avoids using CHROOT_IN & CHROOT_OUT around command*
> >   invocations, and reduces the code spent in chroot mode
> > - add failure checks for dup2 and open done in child, not proceeding to
> >   executing the process if they fail
> > - open /dev/null without O_CLOEXEC, so it stays available for the
> >   exec'ed process, and thus we don't need to provide an own fd for stdin
> > 
> > Followup of commit fd2f175ee79d29df101d353e2f380db27b19553a, thanks also
> > to the notes and hints provided by Mateusz Guzik.
> 
> Looks good, thanks. I only have optional nits.
> 
> > ---
> >  daemon/command.c  | 17 ++---------------
> >  daemon/daemon.h   |  1 +
> >  daemon/guestfsd.c | 39 +++++++++++++++++++++++++++++++--------
> >  3 files changed, 34 insertions(+), 23 deletions(-)
> > 
> > diff --git a/daemon/command.c b/daemon/command.c
> > index 27a4d0c..c4efa5b 100644
> > --- a/daemon/command.c
> > +++ b/daemon/command.c
> > @@ -244,7 +244,7 @@ do_command (char *const *argv)
> >  {
> >    char *out;
> >    CLEANUP_FREE char *err = NULL;
> > -  int r, dev_null_fd, flags;
> > +  int r, flags;
> >    CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false };
> >    CLEANUP_RESOLVER_STATE struct resolver_state resolver_state =
> >      { .mounted = false };
> > @@ -261,17 +261,6 @@ do_command (char *const *argv)
> >      return NULL;
> >    }
> >  
> > -  /* Provide /dev/null as stdin for the command, since we want
> > -   * to make sure processes have an open stdin, and it is not
> > -   * possible to rely on the guest to provide it (Linux guests
> > -   * get /dev dynamically populated at runtime by udev).
> > -   */
> > -  dev_null_fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> > -  if (dev_null_fd == -1) {
> > -    reply_with_perror ("/dev/null");
> > -    return NULL;
> > -  }
> > -
> >    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 @@ extern char **empty_list (void);
> >  #define COMMAND_FLAG_FD_MASK                   (1024-1)
> >  #define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR     1024
> >  #define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048
> > +#define COMMAND_FLAG_DO_CHROOT                 4096
> >  
> 
> Any eason for having this decimal? The standard thing is to use hex.

Mostly that the current code was using that style already.

> >  extern int commandf (char **stdoutput, char **stderror, int flags,
> >                       const char *name, ...) __attribute__((sentinel));
> > diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> > index 0a29aa6..47245f7 100644
> > --- a/daemon/guestfsd.c
> > +++ b/daemon/guestfsd.c
> > @@ -932,21 +932,44 @@ commandrvf (char **stdoutput, char **stderror, int flags,
> >      signal (SIGPIPE, SIG_DFL);
> >      close (0);
> >      if (flag_copy_stdin) {
> > -      dup2 (flag_copy_fd, STDIN_FILENO);
> > +      if (dup2 (flag_copy_fd, STDIN_FILENO) == -1) {
> > +        perror ("dup2/flag_copy_fd");
> > +        _exit (EXIT_FAILURE);
> > +      }
> 
> close(0) explicitly assumes that stdin is 0, which is fine, but dup2
> uses STDIN_FILENO (which itself is also fine), but you should stick to
> one scheme.

Like in the case above, this is what the code before was doing.
I could change it, but in a later patch, as I want to avoid mixing style
and functional changes in the same patch.

> >      ignore_value (chdir ("/"));
> >  
> 
> This could also be error-checked.

Indeed, fixed locally:

+    if (chdir ("/") == -1) {
+      perror ("chdir");
+      _exit (EXIT_FAILURE);
+    }

Thanks for review,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20151202/b2feccac/attachment.sig>


More information about the Libguestfs mailing list