Re: squid3-largeobj squid3/src HttpHdrRange.cc...
At the end I just put quick_abort_min and quick_abort_max to its original behavior. They are storing values in kilobytes and if the units are missing kilobytes are the default. It is very easy to change it and also is easy to make parser to not require spaces between value and units. But a problem I am seeing is that at the begging we just want to simplify the code but now we are talking about correcting others thinks which are not critical or important. Any way, it is not difficult to change the above but the original goal was just to fix the bugs of squid3... I dont know... Regards, Christos Henrik Nordstrom wrote: On fre, 2007-08-03 at 00:35 +1200, Amos Jeffries wrote: Although having said that, the current parser requires whitespace between the value and the units. I'm not certain that is a good thing. Been like that for ever. Or at least as long as Squid has been parsing units.. i.e. Squid-1.1 or something like that. (1997 timeframe). I don't have a problem having the parser changed so that it also accepts specifications without the space however. Regards Henrik
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
Tsantilas Christos wrote: At the end I just put quick_abort_min and quick_abort_max to its original behavior. They are storing values in kilobytes and if the units are missing kilobytes are the default. It is very easy to change it and also is easy to make parser to not require spaces between value and units. But a problem I am seeing is that at the begging we just want to simplify the code but now we are talking about correcting others thinks which are not critical or important. Any way, it is not difficult to change the above but the original goal was just to fix the bugs of squid3... I dont know... Regards, Christos Quite true, Christos. Thanks for the kick. This apparently has been the problem with 3.0 from early on. Far too many 'improvements' popping up. For myself I'm discussing and mostly just keeping an email folder of small ideas for future. When I get my chance at 3.1 a lot will begin to happen. I'm okay with putting the idea on backburner. Right now you are the only one to have put hand to code on this. We are only waiting on two blocker bugs now before 3.0 or PRE7. How soon? and which? I'm just so impatient to know :-) Amos Henrik Nordstrom wrote: On fre, 2007-08-03 at 00:35 +1200, Amos Jeffries wrote: Although having said that, the current parser requires whitespace between the value and the units. I'm not certain that is a good thing. Been like that for ever. Or at least as long as Squid has been parsing units.. i.e. Squid-1.1 or something like that. (1997 timeframe). I don't have a problem having the parser changed so that it also accepts specifications without the space however. Regards Henrik
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On fre, 2007-08-03 at 00:35 +1200, Amos Jeffries wrote: Although having said that, the current parser requires whitespace between the value and the units. I'm not certain that is a good thing. Been like that for ever. Or at least as long as Squid has been parsing units.. i.e. Squid-1.1 or something like that. (1997 timeframe). I don't have a problem having the parser changed so that it also accepts specifications without the space however. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On ons, 2007-08-01 at 16:53 -0600, Alex Rousskov wrote: If there is consensus that units should be required everywhere, I am fine with that (but still worried). I wote for requiring units in all unit based directives. Seeing them without unit just calls for confusion both by the admin and Squid upgrades.. The exception is when an upgrade converts a directive from being just a number to be unit based, but there is no such transitions in Squid-2.5 (or 2.6) - Squid-3. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
Amos Jeffries wrote: Alex Rousskov wrote: On Wed, 2007-08-01 at 20:18 +1200, Amos Jeffries wrote: How about I first dig-up/rewrite some old code I wrote years ago that parses a value nX where n is some integer and X is one of the B/KB/MB/GB/...etc base strings? That would be nice. I think Squid3 already has something similar for some time values, but it is not required. Same for some size values, as Henrik pointed out. Just digging through the code. Yes Henrik is right the byte-units parsing is already there. Although having said that, the current parser requires whitespace between the value and the units. I'm not certain that is a good thing. Amos
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
Alex Rousskov wrote: On Wed, 2007-08-01 at 20:18 +1200, Amos Jeffries wrote: How about I first dig-up/rewrite some old code I wrote years ago that parses a value nX where n is some integer and X is one of the B/KB/MB/GB/...etc base strings? That would be nice. I think Squid3 already has something similar for some time values, but it is not required. Same for some size values, as Henrik pointed out. Just digging through the code. Yes Henrik is right the byte-units parsing is already there. Amos
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
Tsantilas Christos wrote: Henrik Nordstrom wrote: On tis, 2007-07-31 at 21:09 +, chtsanti wrote: Please avoid casting unless needed. The compiler automatically promotes to larger types when needed, and will tell you if you try to do the reverse.. So stop casting things to (int64_t). OK. Sometimes I am using this typecasts just to note that here this operations must be done in 64bits. But yes this is why the comments exists ... Moreover casting maybe it is dangerous in the cases where the signess of an integer changes The exception is the upshifts if the value shifted may be large. But I think these should be changed to store bytes to begin with avoiding the problem. It's really more of a theoretical configuration limitation than a real limitation. It's very unlikely anyone would want to configure readahead_gap or quick_abort_min/max as large as 2 GB or more.. So make Config.readAheadGap and Config.quickAbort.min/max b_size_t instead of kb_size_t, and stop upshifting it to compare... Maybe even should be a b_int64_t. Probably we should even kill the kb_size_t type entirely, converting them all to b_int64_t. I did not change it now because, if I am not wrong, the kb_size_t type has the meaning that the users enters the configuration parameter in Kbytes (eg quick_abort_min 128 is 128Kbytes). But if it is a required I will do it Regards, Christos How about I first dig-up/rewrite some old code I wrote years ago that parses a value nX where n is some integer and X is one of the B/KB/MB/GB/...etc base strings? Then we can go about making all these annoying magic squid.conf values more human readable where they are supposed to be bandwidth amounts or storage sizes. And have the parser determine the minimum values base multiplier. Interest? Amos
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
Henrik Nordstrom wrote: On tis, 2007-07-31 at 21:09 +, chtsanti wrote: Please avoid casting unless needed. The compiler automatically promotes to larger types when needed, and will tell you if you try to do the reverse.. So stop casting things to (int64_t). OK. Sometimes I am using this typecasts just to note that here this operations must be done in 64bits. But yes this is why the comments exists ... Moreover casting maybe it is dangerous in the cases where the signess of an integer changes The exception is the upshifts if the value shifted may be large. But I think these should be changed to store bytes to begin with avoiding the problem. It's really more of a theoretical configuration limitation than a real limitation. It's very unlikely anyone would want to configure readahead_gap or quick_abort_min/max as large as 2 GB or more.. So make Config.readAheadGap and Config.quickAbort.min/max b_size_t instead of kb_size_t, and stop upshifting it to compare... Maybe even should be a b_int64_t. Probably we should even kill the kb_size_t type entirely, converting them all to b_int64_t. I did not change it now because, if I am not wrong, the kb_size_t type has the meaning that the users enters the configuration parameter in Kbytes (eg quick_abort_min 128 is 128Kbytes). But if it is a required I will do it Regards, Christos
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On ons, 2007-08-01 at 10:35 +0300, Tsantilas Christos wrote: OK. Sometimes I am using this typecasts just to note that here this operations must be done in 64bits. But yes this is why the comments exists ... Moreover casting maybe it is dangerous in the cases where the signess of an integer changes And most importantly it makes automatic discovery of mismatches when changing datatypes much harder. I did not change it now because, if I am not wrong, the kb_size_t type has the meaning that the users enters the configuration parameter in Kbytes (eg quick_abort_min 128 is 128Kbytes). True, but only with a big warning.. 2007/08/01 11:32:56| WARNING: No units on 'quick_abort_max 1', assuming 1.00 KB But if it is a required I will do it Not strictly required, but only using bytes makes the code easier to follow and less risk of casting problems.. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On Wed, 2007-08-01 at 20:18 +1200, Amos Jeffries wrote: How about I first dig-up/rewrite some old code I wrote years ago that parses a value nX where n is some integer and X is one of the B/KB/MB/GB/...etc base strings? That would be nice. I think Squid3 already has something similar for some time values, but it is not required. Same for some size values, as Henrik pointed out. Then we can go about making all these annoying magic squid.conf values more human readable where they are supposed to be bandwidth amounts or storage sizes. And have the parser determine the minimum values base multiplier. I think that unitless parameters are wrong, but making size units _required_ worries me for two reasons: 1) Changing this will make switching between Squid2 and Squid3 configs more difficult. 2) This change would have to be done before Squid 3.0 release and may delay it. On the other hand, if size units are optional, then we should add them, but there is no rush to do it now as it will not solve any critical problems. If they take more than a couple of days to write, test, and commit, then we can add them to 3.1, for example. Alex.
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On tor, 2007-08-02 at 01:32 +0300, Tsantilas Christos wrote: At the end I convert Config.readAheadGap and Config.quickAbort.min/max to b_int64_t types. Ok. But this is changes the default units for Config.quickAbort.min/max parameters and make them different than those of squid26 configuration file. Is it any problem with it? Not in my eyes. And certainly not if changing the code to no longer accept unitless specifications. These directives have always been unit based, and at least 2.6 (probably 2.5 as well) gives big fat warnings issued if the user specify them without unit. So the change to make Squid-3 require an unit is minor. My view is that any Squid-2 configurations out in the field without an unit specified is bad configurations and should be fixed, even for Squid-2. So having Squid-3 reject them is actually an improvement. The fixed configuration will work as expected in both versions. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On Thu, 2007-08-02 at 00:40 +0200, Henrik Nordstrom wrote: On tor, 2007-08-02 at 01:32 +0300, Tsantilas Christos wrote: At the end I convert Config.readAheadGap and Config.quickAbort.min/max to b_int64_t types. Ok. But this is changes the default units for Config.quickAbort.min/max parameters and make them different than those of squid26 configuration file. Is it any problem with it? Not in my eyes. And certainly not if changing the code to no longer accept unitless specifications. These directives have always been unit based, and at least 2.6 (probably 2.5 as well) gives big fat warnings issued if the user specify them without unit. So the change to make Squid-3 require an unit is minor. My view is that any Squid-2 configurations out in the field without an unit specified is bad configurations and should be fixed, even for Squid-2. So having Squid-3 reject them is actually an improvement. The fixed configuration will work as expected in both versions. If a unitless configuration is rejected, there is no problem. If it is accepted (with a warning) and then interpreted differently from Squid2, I think there is a problem because many admins will [continue to] ignore the warning. Can you make the units required in this particular case? If there is consensus that units should be required everywhere, I am fine with that (but still worried). Alex.
Re: squid3-largeobj squid3/src HttpHdrRange.cc...
On tis, 2007-07-31 at 21:09 +, chtsanti wrote: - Some convertions of variables to 64bit integers, for which I am not sure if really needed but they used to hold results of 64bit operations and I think it is not safe to downgrade 64bit integers ... Please avoid casting unless needed. The compiler automatically promotes to larger types when needed, and will tell you if you try to do the reverse.. So stop casting things to (int64_t). The exception is the upshifts if the value shifted may be large. But I think these should be changed to store bytes to begin with avoiding the problem. It's really more of a theoretical configuration limitation than a real limitation. It's very unlikely anyone would want to configure readahead_gap or quick_abort_min/max as large as 2 GB or more.. So make Config.readAheadGap and Config.quickAbort.min/max b_size_t instead of kb_size_t, and stop upshifting it to compare... Maybe even should be a b_int64_t. Probably we should even kill the kb_size_t type entirely, converting them all to b_int64_t. Focus on killing casts when not absolutely needed. Casts makes it nearly impossible to find problems using automated methods.. Regards Henrik signature.asc Description: This is a digitally signed message part
