Re: [PATCH net-next] bonding: convert delay parameters to unsigned integers

[Nikolay Aleksandrov <nikolay@redhat.com>]

On 06/11/12 20:39, Nikolay Aleksandrov wrote:
> On 06/11/12 20:03, Jay Vosburgh wrote:
>> Nikolay Aleksandrov<nikolay@redhat.com>  wrote:
>>
>>> This patch converts the following parameters from int to unsigned 
>>> int in order
>>> to remove the unnecessary<  0 checks in the code and make it less 
>>> error prone.
>>> The functionality remains unchanged as before it wasn't allowed to 
>>> have these
>>> be less than zero too. Their maximum value was INT_MAX, now it will 
>>> be UINT_MAX.
>>> It also fixes how min_links parameter is treated since it is 
>>> unsigned int
>>> already but was treated as signed integer.
>>     Is this patch fixing any actual bugs, and if so, what?
> I don't claim that it fixes any bugs, it is a cosmetic change that 
> allows to remove
> < 0 checks in the code without affecting functionality. The only "fix" 
> there is, is
> how min_links is treated when changed (everywhere it was used as int 
> but it
> is in fact unsigned int), although this didn't present any problems it 
> was more
> of an inconsistency.
>>> struct bond_params members:
>>> updelay downdelay miimon arp_interval
>>>
>>> The delay variable is initialized from updelay and downdelay 
>>> parameters and
>>> counted down to zero so it should never be<  0.
>>>
>>> struct slave members:
>>> delay
>>>
>>> The change to struct ifbond's miimon parameter (s32 ->  u32) will 
>>> probably
>>> affect some user-space software but it should be all right in 99 % 
>>> of the
>>> cases as I don't think anyone uses>  INT_MAX for that.
>>     The struct ifbond is used to pass information from the kernel to
>> user space in response to an SIOCBONDINFOQUERY ioctl.  It is not used to
>> set values within the kernel.  In any event, I don't believe it is
>> acceptable to modify a user-visible structure like this, even to change
>> types from signed to unsigned of the same size.
> I should've explained more about the "limited" part. This is exactly 
> what I meant,
> if this variable type can/should be changed. But I don't know who 
> would use this
> at such high values (it loses any meaning IMO). If it is a problem I 
> will re-post this
> patch without this change, I did it only to be consistent with the 
> miimon type, but
> then upon returning the ifbond miimon variable might have wrong value.
>>> Limited testing of this patch was done in VMs.
>>     If this is not actually fixing any bugs (i.e., is a cosmetic
>> change only), then I think it's reasonable to expect the changes to be
>> fully tested by you to insure there are no regressions introduced.
>>
>>     In particular, if a negative number is passed in after your
>> patch is applied, is it accepted or rejected, and if it is accepted,
>> what is the value?  For example, if the user specifies "miimon=-1" does
>> that fail, or is miimon set to 4294967295?
>>
>>     -J
> The unsigned int changes have been tested and to answer your question,
> when you define a module parameter to be of a certain type, that type is
> enforced by the way kernel_param_ops are defined for that type. In this
> case if you specify a negative value the parameter won't be set and if 
> it is
> on the command line (and not through sysfs) the module won't get loaded
> as before.
Sorry, not as before, before it was set to a default value if < 0, now 
it won't get loaded
due to the wrong parameter. So this actually changes the behaviour, if such
change is wrong, then ignore this patch.

Nik
> (Information based on tests, kernel/params.c & kernel/module.c)
>> --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com 
> Best regards,
> Nikolay Aleksandrov
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html