On 01/04/2012 09:58 AM, Michal Privoznik wrote:
Currently, virCommand implementation uses FD_ macros from sys/select.h. However, those cannot handle more opened files than FD_SETSIZE. Therefore switch to generalized implementation based on array of integers. --- diff to v1: - Eric's review included
src/util/command.c | 113 ++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 87 insertions(+), 26 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index bdaa88b..41fb020 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -75,9 +75,10 @@ struct _virCommand {
char *pwd;
- /* XXX Use int[] if we ever need to support more than FD_SETSIZE fd's. */ - fd_set preserve; /* FDs to pass to child. */ - fd_set transfer; /* FDs to close in parent. */ + int *preserve; /* FDs to pass to child. */ + int preserve_size; + int *transfer; /* FDs to close in parent. */ + int transfer_size;
Normally, I'd say "use size_t" for array sizes. But, as these are arrays of fds (aka non-negative ints), and there are no duplicates in the array (other than -1 once you have closed an fd), an array sized by int is sufficient. Meanwhile, if we were to worry about transferring a LARGE number of fds, then I'd suggest using VIR_EXPAND_N, which requires tracking both allocation and usage sizes as size_t, but I don't think we are hitting that much overhead in any of our virCommand usage. So no need to change this.
@@ -736,19 +796,21 @@ virCommandNewArgList(const char *binary, ...) static bool virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) { + int ret = 0; + if (!cmd) return fd > STDERR_FILENO;
- if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) { + if (fd <= STDERR_FILENO || + (ret = virCommandFDSet(fd, &cmd->preserve, &cmd->preserve_size)) || + (transfer && (ret = virCommandFDSet(fd, &cmd->transfer, + &cmd->transfer_size)))) { if (!cmd->has_error) - cmd->has_error = -1; + cmd->has_error = ret ? : -1 ;
That's a gcc extension. Spell it out explicitly: cmd->has_error = ret ? ret : -1;
VIR_DEBUG("cannot preserve %d", fd); - return fd > STDERR_FILENO; + return true;
Don't change this line. Otherwise, calling virCommandTransferFD(cmd, 2) (a coding bug) would close stderr, instead of diagnosing the coding bug. ACK with those two lines fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org