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

Re: [libvirt] [PATCH 2/3] Docs for usage of new command APIs



On 05/25/2010 07:24 AM, Daniel P. Berrange wrote:
> ---
>  docs/Makefile.am       |   12 ++++++++++--
>  docs/internals.html.in |    9 +++++++++
>  docs/sitemap.html.in   |    4 ++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> +++ b/docs/Makefile.am
> @@ -53,7 +53,7 @@ gif = \
>    architecture.gif \
>    node.gif
>  
> -dot_html_in = $(wildcard *.html.in)
> +dot_html_in = $(wildcard *.html.in) $(wildcard internals/*.html.in)
>  dot_html = $(dot_html_in:%.html.in=%.html)

Note to myself: If I ever get time to revisit my VPATH patches, I've got
a merge conflict in this area.

Jim already pointed out the new file omission, but given your first
message had the text-ified version, here's some spelling fixes:

>                Spawning processes / commands from libvirt drivers
> 
>    This page describes the usage of libvirt APIs for spawning processes /
>    commands from libvirt drivers. All code is required to use these APIs
> 
> Problems with standard POSIX APIs
> 
>    The POSIX specification includes a number of APIs for spawning processes /
>    commands, but they suffer from a number of flaws
> 
>      * fork+exec: The lowest & most flexible level, but very hard to use
>        correctly / safely. It is easy to leak file descriptors, have
>        unexpected signal handler behaviour and not handle edge cases

Also, not portable to mingw.

>      * system: Convenient if you don't care about capturing command output,
>        but has the serious downside that the command string is interpreted by
>        the shell. This makes it very dangerous to use, because improperly
>        validated user input can lead to exploits via shell meta characters.

* popen: Similar problems to system; also has issues affecting signal
handling related to other child processes.

>      * posix_spawn: A half-way house between simplicity of system() and the
>        flexibility of fork+exec. It does not allow for a couple of important
>        features though, such as running a hook between the fork+exec stage,
>        or closing all open file descriptors.
> 
>    Due to the problems mentioned with each of these, libvirt driver code must
>    not use any of the above APIs. Historically libvirt provided a higher
>    level API known as virExec. This was wrapper around fork+exec, in a
>    similar style to posix_spawn, but with a few more features.

I didn't see anything in patch 1/3 that deprecated virExec in the public
headers.  Is it worth adding conditional deprecation (guarded by a macro
defined only in command.c), to make sure it is not called anywhere
except in the command.c implementation?

> 
>    This wrapper still suffered from a number of problems. Handling command
>    cleanup via waitpid() is overly complex & error prone for most usage.
>    Building up the argv[] + env[] string arrays is quite cumbersome and error
>    prone, particularly wrt memory leak / OOM handling.
> 
> The libvirt command execution API
> 
>    There is now a high level API that provides a safe and flexible way to
>    spawn commands, which prevents the most common errors & is easy to code

s/&/and/

>    against. This code is provided in the src/util/command.h header which can
>    be imported using #include "command.h"
> 
>   Defining commands in libvirt
> 
>    The first step is to declare what command is to be executed. The command
>    name can be either a fully qualified path, or a bare command name. In the
>    latter case it will be resolved wrt the $PATH environment variable.
> 
>        virCommandPtr cmd = virCommandNew("/usr/bin/dnsmasq");
> 
> 
>    There is no need to check for allocation failure after virCommandNew. This
>    will be detected and reported at a later time.
> 
>   Adding arguments to the command
> 
>    There are a number of APIs for adding arguments to a command. To add a
>    direct string arg
> 
>        virCommandAddArg(cmd, "-strict-order");
> 
> 
>    If an argument takes an attached value of the form -arg=val, then this can
>    be done using
> 
>        virCommandAddArgPair(cmd, "--conf-file", "/etc/dnsmasq.conf");
> 
> 
>    To add a entire NULL terminated array of arguments in one go
> 
>        const char *const args[] = {
>            "--strict-order", "--except-interface",
>            "lo", "--domain", "localdomain", NULL
>        };
>        virCommandAddArgSet(cmd, args);
> 
> 
>    This latter option can also be used at time of initial construction of the
>    virCommandPtr object
> 
>        const char *const args[] = {
>            "--strict-order", "--except-interface",
>            "lo", "--domain", "localdomain", NULL
>        };
>        virCommandPtr cmd = virCommandNewArgs(cmd, args);
> 
> 
>   Setting up the environment
> 
>    By default a command will inherit all environment variables from the
>    current process. Generally this is not desirable and a customized
>    environment will be more suitable. Any customization done via the
>    following APIs will prevent inheritance of any existing environment
>    variables unless explicitly allowed. The first step is usually to pass
>    through a small number of variables from the current process.
> 
>        virCommandAddEnvPassCommon(cmd);
> 
> 
>    This has now set up a clean environment for the child, passing through
>    PATH, LD_PRELOAD, LD_LIBRARY_PATH, HOME, USER, LOGNAME and TMPDIR.
>    Furthermore it will explicitly set LC_ALL=C to avoid unexpected
>    localization of command output. Further variables can be passed through
>    from parent explicitly:
> 
>        virCommandAddEnvPass(cmd, "DISPLAY");
>        virCommandAddEnvPass(cmd, "XAUTHORITY");
> 
> 
>    To define an environment variable in the child with an separate key /
>    value:
> 
>        virCommandAddEnvPair(cmd, "TERM", "xterm");
> 
> 
>    If the key/value pair is pre-formatted in the right format, it can be set
>    directly
> 
>        virCommandAddEnvString(cmd, "TERM=xterm");
> 
> 
>   Miscellaneous other options
> 
>    Normally the spawned command will retain the current process as its
>    parent. If the current process dies, the child will then (usually) be
>    terminated too. If this cleanup is not desired, then the command should be
>    marked as daemonized:
> 
>        virCommandDaemonize(cmd);
> 
> 
>    When daemonizing a command, the PID visible from the caller will be that
>    of the intermediate process, not the actual damonized command. If the PID

s/damonized/daemonized/

>    of the real command is required then a pidfile can be requested
> 
>        virCommandSetPidFile(cmd, "/var/run/dnsmasq.pid");
> 
> 
>    This PID file is guaranteed to be written before the intermediate process
>    exits.
> 
>   Reducing command privileges
> 
>    Normally a command will inherit all privileges of the current process. To
>    restrict what a command can do, it is possible to request that all its
>    capabilities are cleared. With this done it will only be able to access
>    resources for which it has explicit DAC permissions
> 
>        virCommandClearCaps(cmd);
> 
> 
>   Managing file handles
> 
>    To prevent unintended resource leaks to child processes, all open file
>    handles will be closed in the child, and its stdin/out/err all connected
>    to /dev/null. It is possible to allow an open file handle to be passed
>    into the child:
> 
>        virCommandPreserveFD(cmd, 5);
> 
> 
>    With this file descriptor 5 in the current process remains open as file
>    descriptor 5 in the child. For stdin/out/err it is usually neccessary to

s/neccessary/necessary/

>    map a file handle. To attach file descriptor 7 in the current process to
>    stdin in the child:
> 
>        virCommandSetInputFD(cmd, 7);
> 
> 
>    Equivalently to redirect stdout or stderr in the child, pass in a pointer
>    to the desired handle
> 
>        int outfd = open("out.log", "w+");
>        int errfd = open("err.log", "w+");
>        virCommandSetOutputFD(cmd, &outfd);
>        virCommandSetErrorFD(cmd, &errfd);
> 
> 
>    Alternatively it is possible to request that a pipe be created to fetch
>    stdout/err in the parent, by initializing the FD to -1.
> 
>        int outfd = -1;
>        int errfd = -1
>        virCommandSetOutputFD(cmd, &outfd);
>        virCommandSetErrorFD(cmd, &errfd);
> 
> 
>    Once the command is running, outfd and errfd will be initialized with
>    valid file handles that can be read from.
> 
>   Feeding & capturing strings to/from the child
> 
>    Often dealing with file handles for stdin/out/err is unneccessarily

s/unneccessarily/unnecessarily/

>    complex. It is possible to specify a string buffer to act as the data
>    source for the child's stdin
> 
>        const char *input = "Hello World\n";
>        virCommandSetInputBuffer(cmd, input);
> 
> 
>    Similarly it is possible to request that the child's stdout/err be
>    redirected into a string buffer
> 
>        char *output = NULL, *errors = NULL;
>        virCommandSetOutputBuffer(cmd, &output);
>        virCommandSetErrorBuffer(cmd, &errors);
> 
> 
>    Once the command has finished executing, these buffers will contain the
>    output. It is the callers responsibility to free these buffers.
> 
>   Running commands synchronously
> 
>    For most commands, the desired behaviour is to spawn the command, wait for

/me decides not to even bother with the UK vs. US issue.

>    it to complete & exit and then check that its exit status is zero.
> 
>        if (virCommandRun(cmd, NULL) < 0)
>           return -1;
> 
> 
>    Note: if the command has been daemonized this will only block & wait for
>    the intermediate process, not the real command. virCommandRun will report
>    on any errors that have occured upon this point with all previous API

s/occured/occurred/

>    calls. If the command fails to run, or exits with non-zero status an error
>    will be reported via normal libvirt error infrastructure. If a non-zero
>    exit status can represent a success condition, it is possible to request
>    the exit status and perform that check manually instead of letting
>    virCommandRun raise the error
> 
>        int status;
>        if (virCommandRun(cmd, &status) < 0)
>           return -1;
> 
>        if (WEXITSTATUS(status) ...) {
>          ...do stuff...
>        }
> 
> 
>   Running commands asynchronously
> 
>    In certain complex scenarios, particularly special I/O handling is
>    required for the child's stdin/err/out it will be neccessary to run the

s/neccessary/necessary/

>    command asynchronously and wait for completion separately.
> 
>        pid_t pid;
>        if (virCommandRunAsync(cmd, &pid) < 0)
>           return -1;
> 
>        ... do something while pid is running ...
> 
>        int status;
>        if (virCommandWait(cmd, &status) < 0)
>           return -1;
> 
>        if (WEXITSTATUS(status)...) {
>           ..do stuff..
>        }
> 
> 
>    As with virCommandRun, the status arg for virCommandWait can be omitted,
>    in which case it will validate that exit status is zero and raise an error
>    if not.
> 
>   Releasing resources
> 
>    Once the command has been executed, or if execution has been abandoned, it
>    is neccessary to release resources associated with the virCommandPtr
>    object. This is done with:
> 
>        virCommandFree(cmd);
> 
> 
>    There is no need to check if cmd is NULL before calling virCommandFree.
>    This scenario is handled automatically. If the command is still running,
>    it will be forcably killed and cleaned up (via waitpid).

s/forcably/forcibly/

> 
> Complete examples
> 
>    This shows a complete example usage of the APIs roughly using the libvirt
>    source src/util/hooks.c
> 
>      int runhook(const char *drvstr, const char *id,
>                  const char *opstr, const char *subopstr,
>                  const char *extra) {
>        int ret;
>        char *path;
>        virCommandPtr cmd;
> 
>        ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr);
>        if ((ret < 0) || (path == NULL)) {
>            virHookReportError(VIR_ERR_INTERNAL_ERROR,
>                               _("Failed to build path for %s hook"),
>                               drvstr);
>            return -1;
>        }
> 
>        cmd = virCommandNew(path);
>        VIR_FREE(path);
> 
>        virCommandAddEnvPassCommon(cmd);
> 
>        virCommandAddArg(cmd, id);
>        virCommandAddArg(cmd, opstr);
>        virCommandAddArg(cmd, subopstr);
>        virCommandAddArg(cmd, extra);
> 
>        virCommandSetInputBuffer(cmd, input);
> 
>        ret = virCommandRun(cmd, NULL);
> 
>        virCommandFree(cmd);
> 
>        return ret;
> 
> 
>    In this example, the command is being run synchronously. A pre-formatted
>    string is being fed to the command as its stdin. The command takes four
>    arguments, and has a minimal set of environment variables passed down. In
>    this example, the code does not require any error checking. All errors are
>    reported by the virCommandRun method, and the exit status from this is
>    returned to the caller to handle as desired.
> 

Also, see my comments to 0/3 regarding semantic issues (this review only
did spelling/grammar issues).

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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