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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list