Re: fd patch to squid

Thanks for your review, I'll check all your suggestions. And feel free
contact me if you have any remarks to package distributed with fedora...

Regards,

Martin

Henrik Nordstrom wrote:
> ons 2006-07-19 klockan 15:13 +0200 skrev Martin Stransky:
>
>>Hello guys,
>>
>>As you probably know, we have one RH specific patch to squid.
>
>
> Yes, found this out by accident some time ago..
>
>
>> It's for
>>dynamic configuration of filedescriptors number, via. squid.conf. I
>>reviewed it for the 2.6 branch, fixed some bugs (which comes with squid
>> reconfiguration) so I think it's stable enough now.
>
>
> A quick review of the patch indicates it's a bit overcomplicated
>
> And I'm not that convinced you really need it either.. A simple
> directive limiting Squid_MaxFD and a high limit specified to configure
> would do nearly as good.
>
> The parts eleminating SQUID_MAXFD constant dependencies is good.
>
> Note: The patch does not account for comm_select_win32.c, causing
> problems for the win32 plaform..
>
> The parts messing with comm_select.c doesn't feel warranted. Systems
> using select() is generally not good at handling more than FD_SETSIZE
> connections anyway, and the select interface as such crumbles a bit when
> used for many concurrent connections. Also not that keen on using a
> custom type for the fd_set given to select(), even if we already make
> very strict assumptions about fd_set being a bit array.. (but I have
> heard of some systems using an array of ints...)
>
> The bit array stuff doesn't feel needed, as it's only used by the
> changes to comm_select.c.
>
> Renaming Squid_MaxFD to SQUID_NUMFD does not feel right. Our naming
> convention uses all caps for preprocessor constants only, and even with
> the patch it's still the max limit this Squid can handle after startup..
> (same as Squid_MaxFD).
>
>
> What I'd like to see for doing this proper is
>
> * The stuff eleminating SQUID_MAXFD dependencies in arrays etc.
>
> * The directive for setting the fd limit
>
> * Checks in comm_select_init or similar to lower Squid_MaxFD if it's
> above what the select loop implementation can handle. This means
> comm_select.c and comm_select_win32.c limiting Squid_MaxFD to
> FD_SETSIZE. (eleminating the similar check from main.c). Some thinking
> needed to get rid of comm_select dependencies.. (probably a new
> comm_select_fdlimit call needed before comm_init)
>
> * Moving the FD_SETSIZE override from squid.h to comm_select.c before
> include of squid.h..
>
> * After this SQUID_MAXFD should only be referenced from the FD_SETSIZE
> override in comm_select.c.
>
> Regards
> Henrik
Received on Thu Jul 20 2006 - 03:14:51 MDT