[PATCH] qdisc: add hfsc qdisc support

[Cong Wang]

On Sat, Jul 26, 2014 at 8:22 AM, Thomas Haller <thaller at redhat.com> wrote:
> Hi Cong,
>
>
>
>
>> diff --git a/include/netlink/route/qdisc/hfsc.h b/include/netlink/route/qdisc/hfsc.h
>> new file mode 100644
>> index 0000000..7cf1c76
>> --- /dev/null
>> +extern uint32_t      rtnl_hfsc_get_defcls(struct rtnl_qdisc *);
>> +extern int   rtnl_hfsc_set_defcls(struct rtnl_qdisc *, uint32_t);
>> +
>> +extern int rtnl_hfsc_get_rsc(struct rtnl_class *class, struct tc_service_curve *tsc);
>> +extern int rtnl_hfsc_set_rsc(struct rtnl_class *class, const struct tc_service_curve *tsc);
>> +extern int rtnl_hfsc_get_fsc(struct rtnl_class *class, struct tc_service_curve *tsc);
>> +extern int rtnl_hfsc_set_fsc(struct rtnl_class *class, const struct tc_service_curve *tsc);
>> +extern int rtnl_hfsc_get_usc(struct rtnl_class *class, struct tc_service_curve *tsc);
>> +extern int rtnl_hfsc_set_usc(struct rtnl_class *class, const struct tc_service_curve *tsc);
>
>
> I wonder, is it correct that all functions have rtnl_hfsc_* as prefix,
> but some operate on rtnl_qdisc, others on rtnl_class.
>
> How about a separate prefix for the function names?
>

I am fine with your suggestion, just want to point out that
htb API has the same problem. :)


>
>
>> diff --git a/lib/route/qdisc/hfsc.c b/lib/route/qdisc/hfsc.c
>> new file mode 100644
>> index 0000000..c83f1fa
>> --- /dev/null
>> +++ b/lib/route/qdisc/hfsc.c
>
>> +/** @cond SKIP */
>> +#define SCH_HFSC_HAS_RSC             0x001
>> +#define SCH_HFSC_HAS_FSC             0x002
>> +#define SCH_HFSC_HAS_USC             0x004
>> +
>> +#define SCH_HFSC_HAS_DEFCLS          0x01
>> +/** @endcond */
>
>
> Same here. How about different prefixes for these names?
>

Agreed.

>
>
>> +int rtnl_hfsc_set_defcls(struct rtnl_qdisc *qdisc, uint32_t defcls)
>> +{
>> +     struct rtnl_hfsc_qdisc *hfsc;
>> +
>> +     if (!(hfsc = hfsc_qdisc_data(qdisc)))
>> +             return -NLE_OPNOTSUPP;
>
> shouldn't the set function allocate some data if it is not allocated yet
> (and possibly fail with -NLE_NOMEM)?
>
> And the same for other _set_ functions.
>

Makes sense, will change them.

Thanks!