[libvirt] [PATCH v2] command: Discard FD_SETSIZE limit for opened files

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120104/8477742f/attachment-0001.sig>