[systemd-devel] [PATCH 1/1] sd-rtnl: Introduce container parsing

[Susant Sahani]

On 03/24/2014 09:58 PM, Tom Gundersen wrote:
> On Sat, Mar 22, 2014 at 5:13 PM, Susant Sahani <susant at redhat.com> wrote:
>> Introducing generic container parsing . Now  supported for type
>> FLA_INFO_KIND and IFLA_VLAN_ID which can be extended to other
>> container parsing which is based on table based look up.
>> ---
>>   src/libsystemd/sd-rtnl/rtnl-internal.h | 20 +++++++++
>>   src/libsystemd/sd-rtnl/rtnl-message.c  | 79 +++++++++++++++++++++++++++++++---
>>   src/libsystemd/sd-rtnl/rtnl-util.c     | 31 +++++++++++++
>>   src/libsystemd/sd-rtnl/rtnl-util.h     |  2 +
>>   src/libsystemd/sd-rtnl/test-rtnl.c     | 13 +++++-
>>   5 files changed, 138 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libsystemd/sd-rtnl/rtnl-internal.h b/src/libsystemd/sd-rtnl/rtnl-internal.h
>> index f011dbe..eb30682 100644
>> --- a/src/libsystemd/sd-rtnl/rtnl-internal.h
>> +++ b/src/libsystemd/sd-rtnl/rtnl-internal.h
>> @@ -85,6 +85,15 @@ struct sd_rtnl {
>>           sd_event *event;
>>   };
>>
>> +struct rtnl_container {
>> +        unsigned short container_type;
>> +
>> +        size_t *rta_offset_tb;
>> +        unsigned short rta_tb_size;
>> +
>> +        LIST_FIELDS(struct rtnl_container, container);
>> +};
>> +
>>   struct sd_rtnl_message {
>>           RefCount n_ref;
>>
>> @@ -96,6 +105,10 @@ struct sd_rtnl_message {
>>           size_t next_rta_offset; /* offset from hdr to next rta */
>>           size_t *rta_offset_tb;
>>           unsigned short rta_tb_size;
>> +        struct rtnl_container *container_list[RTNL_CONTAINER_DEPTH];
>> +
>> +        LIST_HEAD(struct rtnl_container, containers);
>> +
>>           bool sealed:1;
>>   };
>>
>> @@ -112,6 +125,13 @@ int rtnl_message_parse(sd_rtnl_message *m,
>>                          struct rtattr *rta,
>>                          unsigned int rt_len);
>>
>> +int rtnl_container_new(struct rtnl_container **ret, uint16_t container_type);
>> +int rtnl_message_parse_container(sd_rtnl_message *m,
>> +                                 uint8_t type,
>> +                                 uint8_t tb_size,
>> +                                 struct rtattr *rta,
>> +                                 unsigned int rt_len);
>> +
>>   /* Make sure callbacks don't destroy the rtnl connection */
>>   #define RTNL_DONT_DESTROY(rtnl) \
>>           _cleanup_rtnl_unref_ _unused_ sd_rtnl *_dont_destroy_##rtnl = sd_rtnl_ref(rtnl)
>> diff --git a/src/libsystemd/sd-rtnl/rtnl-message.c b/src/libsystemd/sd-rtnl/rtnl-message.c
>> index e243c7b..c1ade55 100644
>> --- a/src/libsystemd/sd-rtnl/rtnl-message.c
>> +++ b/src/libsystemd/sd-rtnl/rtnl-message.c
>> @@ -58,6 +58,7 @@ int message_new(sd_rtnl *rtnl, sd_rtnl_message **ret, size_t initial_size) {
>>
>>           m->hdr->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
>>           m->sealed = false;
>> +        LIST_HEAD_INIT(m->containers);
>>
>>           if (rtnl)
>>                   m->rtnl = sd_rtnl_ref(rtnl);
>> @@ -276,10 +277,18 @@ sd_rtnl_message *sd_rtnl_message_ref(sd_rtnl_message *m) {
>>   }
>>
>>   sd_rtnl_message *sd_rtnl_message_unref(sd_rtnl_message *m) {
>> +        struct rtnl_container *i, *j;
>> +
>>           if (m && REFCNT_DEC(m->n_ref) <= 0) {
>>                   sd_rtnl_unref(m->rtnl);
>>                   free(m->hdr);
>>                   free(m->rta_offset_tb);
>> +
>> +                LIST_FOREACH_SAFE(container, i, j, m->containers) {
>> +                        free(i->rta_offset_tb);
>> +                        free(i);
>> +                }
>> +
>>                   free(m);
>>           }
>>
>> @@ -752,6 +761,22 @@ int sd_rtnl_message_open_container(sd_rtnl_message *m, unsigned short type) {
>>           return -ENOTSUP;
>>   }
>>
>> +int sd_rtnl_message_enter_container(sd_rtnl_message *m, unsigned short type) {
>> +        struct rtnl_container *itr;
>> +
>> +        LIST_FOREACH(container, itr, m->containers) {
>> +                if (itr->container_type == type)
>> +                        break;
>> +        }
> Hm, so I don't think this is the right thing to do. The type of a
> container (or any message) does not make sense unless you know the
> context (i.e., the types of each of its parent containers). The reason
> being that the types are just integers, and they are only unique
> within a given scope.
   yes correct . After first level parsing rather making generic it's 
better to make the
parser specific to the context , and keep it ready to read. more like 
parent container having
a pointer to child container so that no conflict should come .


>
> I now pushed an alternative patch. Could you have a look if it makes
> sense to you?
If am not wrong *sd_rtnl_message_enter_container* trying to parse 
(rtnl_message_parse)
every time a attribute is requested which is inside nested attribute. It 
would be better to parse only once and keep
the data structures ready for reading so that the parsing does not 
happen each time a attribute is requested. More
like from receiving side from kernel should have some intelligence to 
know what context it's in.


>
> Also, would be great if you could respin the tunnel patch on top of
> this (and following Zbigniew's suggestions for the tests).
I would send the attributes patch first then the test cases since it 
requires
libkmod (src/core/kmod-setup.c) support to load dynamically kernel modules.
> Cheers,
>
> Tom
>
>

Thanks,
Susant