Re: [libvirt] [PATCH] command: Discard FD_SETSIZE limit for opened files
On Tue, Jan 03, 2012 at 09:33:03AM -0700, Eric Blake wrote:
On 01/03/2012 04:14 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.
> ---
> src/util/command.c | 108 ++++++++++++++++++++++++++++++++++++++++------------
> 1 files changed, 83 insertions(+), 25 deletions(-)
NACK. While I agree with the idea, I'd rather use virBitmap rather than
open-coding an int array ourselves.
Is it worth considering the efficiency implications of virBitmap
in this use case ?
Michal's initial patch is pretty time & space efficient, since we
only need to store & iterate over an array that is equal in size
to the number of preserved FDs.
If we use virBitmap, and have libvirtd configured to have a very
large max_files ulimit, then the virBitmap mask will need to be
pretty large and very sparse. eg, if we have ulimit=65536 and want
to pass through of FD=60000 to a guest, we'll end up with a virBitmap
8192 bytes in size, and have to test 5999 empty bits before we
find the FD we're passing through. This seems pretty wasteful to
me.
virBitmap is good where we need to store a large number of bits
which are mostly contiguous. In virCommand, any single virCommandPtr
instance though has very few bits, which are quite sparse.
So while code reuse is good, IMHO, virBitmap is not really the
best choice here.
Now if we want to create a new 'virSet' class for efficiently dealing
with sparse sets.....otherwise I'm inclined to say this patch is fine
as it is.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|