On 05/25/2010 07:24 AM, Daniel P. Berrange wrote:
We have had great success with our APIs for memory allocation and string buffer management, removing most of the potential for incorrect API usage, thus avoiding many common errors. It is time todo the same for command execution.
Yes, this makes a good addition.
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.
The $PATH of the parent process, or the $PATH of the environment passed to the child process? That can make a subtle difference, if one uses virCommandAddEnvString(cmd, "PATH=...").
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");
Does this create the two arguments "--conf-file /etc/dnsmasq.conf" or the one argument "--conf-file=/etc/dnsmasq.conf"?
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");
If the same env-var is added more than once, are we guaranteeing that the last one wins? In other words, it should be easy to call virCommandAddEnvPassCommon(cmd) && virCommandAddEnvString(cmd, "PATH=...") to override just PATH. Should there be an easy way to specify that a particular child process should keep the localization settings of the parent, rather than the LC_ALL=C of virCommandAddEnvPassCommon?
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 of the real command is required then a pidfile can be requested
virCommandSetPidFile(cmd, "/var/run/dnsmasq.pid");
Is this worth a printf-style varargs call, to make it easier to cobble together components? Perhaps like: virCommandSetPidFile(cmd, "%s/%s.pid", LOCAL_STATE_DIR, "dnsmasq") On the other hand, it's relatively easy to build up a string using virBuffer APIs, so we might as well keep the virCommand API simple.
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 map a file handle. To attach file descriptor 7 in the current process to stdin in the child:
virCommandSetInputFD(cmd, 7);
Does the child see fd 7 closed in this case?
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.
Any restrictions on when (or even if) the parent process may/must call close(outfd)? In other words, must the parent's side of the output fd remain open until the exit status of the child has been collected, and does collecting the child's status automatically close the parent's side?
Feeding & capturing strings to/from the child
Often dealing with file handles for stdin/out/err is unneccessarily 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);
Any limitations to be aware of to avoid I/O deadlock when set up as a bi-directional pipe to the child?
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);
Where is input declared?
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.
Overall, this looks like a nice addition. More specific comments to the various patches themselves. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org