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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list