Re: [PATCH 6/7] Make qdisc changeable.

[Corey Hickey <bugfood-ml@fatooh.org>]

Patrick McHardy wrote:
> Corey Hickey wrote:
>> Re-implement sfq_change() and enable Qdisc_opts.change so "tc qdisc
>> change" will work.
>>
>> Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
>> ---
>>  net/sched/sch_sfq.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 50 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
>> index e6a6a21..e042cd0 100644
>> --- a/net/sched/sch_sfq.c
>> +++ b/net/sched/sch_sfq.c
>> @@ -485,6 +485,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
>>  	return 0;
>>  }
>>  
>> +static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
>> +{
>> +	struct sfq_sched_data *q = qdisc_priv(sch);
>> +	struct sfq_sched_data tmp;
>> +	struct sk_buff *skb;
>> +	int err;
>> +	
>> +	/* set up tmp queue */
>> +	memset(&tmp, 0, sizeof(struct sfq_sched_data));
>> +	tmp.quantum = psched_mtu(sch->dev); /* default */
> 
> 
> If no value is given it should use the old value instead of
> reinitializing to the default.
> 
>> +	if ((err = sfq_q_init(&tmp, opt)))
>> +		return err;
> 
> 
> This will also use defaults for all unspecified values. It would
> be more consistent with other qdiscs to only change those values
> that are actually specified, so something like "tc qdisc change ...
> perturb 10" will *only* change the perturbation parameter.

I somehow had it in my head that a qdisc change was supposed to work 
that way. I'll change my patch.

> I'm not sure reusing the initialization function and copying the
> parameters is the best way to do this.

I'm not sure either, but I do like that it's conceptually simple and it 
keeps the parameter handling in one place. I've thought and stared for a 
while, and I don't see a better way, but that could of course be due to 
my limited understanding and general newbiehood in that regard.

Thanks again for the review. I'll try to get a new batch of patches sent 
off tomorrow.

-Corey