[RFC,1/6] batman-adv: Generalize DAT in order to support any type of data, not only IPv4

  From: Mihail Costea <mihail.costea90@gmail.com>

Mades DAT support more types by making its data a void*, adding type field
to dat_entry and adding data_type to necessary functions.
This change is needed in order to make DAT support any type of data, like IPv6 too.

Adds generic function for transforming DAT data to string.
The function is used in order to avoid defining different debug messages
for different DAT data types. For example, if we had IPv6 as a DAT data,
then "%pI4" should be "%pI6c", but all
the other text of the debug message would be the same.
Also everything is memorized in a struct in order to avoid further
switch cases for all types.

Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>

---
 distributed-arp-table.c |  197 +++++++++++++++++++++++++++++++++++------------
 distributed-arp-table.h |    1 +
 types.h                 |   24 +++++-
 3 files changed, 169 insertions(+), 53 deletions(-)
  

On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
> From: Mihail Costea <mihail.costea90@gmail.com>

Hi Mihail,

sorry for the very long delay but I'm currently out of my office, so I'm not
spending a lot of time on batman-adv.

Anyway, I'll try to review your patches as soon as possible!

Cheers,
  

On 23 July 2013 00:27, Antonio Quartulli <ordex@autistici.org> wrote:
> On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
>> From: Mihail Costea <mihail.costea90@gmail.com>
>
> Hi Mihail,
>
> sorry for the very long delay but I'm currently out of my office, so I'm not
> spending a lot of time on batman-adv.
>
> Anyway, I'll try to review your patches as soon as possible!
>

Hi Antonio,

No problem, there's no hurry.

Cheers,
Mihail
  

Hi Mihail,

sorry for the very looong delay :)
I'm finally sitting at my new location and I can give you some feedback on your
nice work.
Comments are inline.

On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
> From: Mihail Costea <mihail.costea90@gmail.com>
> 
> Mades DAT support more types by making its data a void*, adding type field
> to dat_entry and adding data_type to necessary functions.
> This change is needed in order to make DAT support any type of data, like IPv6 too.
> 
> Adds generic function for transforming DAT data to string.
> The function is used in order to avoid defining different debug messages
> for different DAT data types. For example, if we had IPv6 as a DAT data,
> then "%pI4" should be "%pI6c", but all
> the other text of the debug message would be the same.
> Also everything is memorized in a struct in order to avoid further
> switch cases for all types.
> 
> Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
> Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
> Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>
> 
> ---
>  distributed-arp-table.c |  197 +++++++++++++++++++++++++++++++++++------------
>  distributed-arp-table.h |    1 +
>  types.h                 |   24 +++++-
>  3 files changed, 169 insertions(+), 53 deletions(-)
> 
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index f2543c2..90565d0 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -31,9 +31,32 @@
>  #include "types.h"
>  #include "translation-table.h"
>  
> +static struct batadv_dat_type_info batadv_dat_types_info[] = {
> +	{
> +		.size = sizeof(__be32),
> +		.str_fmt = "%pI4",
> +	},
> +};
> +
>  static void batadv_dat_purge(struct work_struct *work);
>  
>  /**
> + * batadv_dat_data_to_str: transforms DAT data to string
> + * @data: the DAT data
> + * @type: type of data
> + * @buf: the buf where the data string is stored
> + * @buf_len: buf length
> + *
> + * Returns buf.
> + */
> +static char *batadv_dat_data_to_str(void *data, uint8_t type,
> +				    char *buf, size_t buf_len)
> +{
> +	snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data);
> +return buf;

alignment.
Remember to check your patches with "checkpatch.pl --strict" before sending. It
will tell you about these hidden mistakes.

> +}
> +
> +/**
>   * batadv_dat_start_timer - initialise the DAT periodic worker
>   * @bat_priv: the bat priv with all the soft interface information
>   */
> @@ -45,6 +68,19 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv)
>  }
>  
>  /**
> + * batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
> + * @rcu: the dat entry rcu
> + */
> +static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu)
> +{
> +	struct batadv_dat_entry *dat_entry;
> +
> +	dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
> +	kfree(dat_entry->data);
> +	kfree(dat_entry);
> +}
> +
> +/**
>   * batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
>   * free it
>   * @dat_entry: the entry to free
> @@ -52,7 +88,7 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv)
>  static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry)
>  {
>  	if (atomic_dec_and_test(&dat_entry->refcount))
> -		kfree_rcu(dat_entry, rcu);
> +		call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);
>  }
>  

since you are not using the kfree_rcu() function anymore, you should also delete
the compatibility function we have in compat.c (and its declaration in
compat.h).

>  /**
> @@ -136,12 +172,21 @@ static void batadv_dat_purge(struct work_struct *work)
>   *
>   * Returns 1 if the two entries are the same, 0 otherwise.
>   */
> -static int batadv_compare_dat(const struct hlist_node *node, const void *data2)
> +static int  batadv_compare_dat(const struct hlist_node *node, const void *data2)
>  {
> -	const void *data1 = container_of(node, struct batadv_dat_entry,
> -					 hash_entry);
> +	struct batadv_dat_entry *dat_entry1 =
> +			container_of(node, struct batadv_dat_entry,
> +				     hash_entry);
> +	struct batadv_dat_entry *dat_entry2 =
> +			container_of(data2,
> +				     struct batadv_dat_entry, data);

These assignments are really ugly :-P.
Please make the assignments after the declarations. They will look much better.

> +	size_t data_size = batadv_dat_types_info[dat_entry1->type].size;
>  
> -	return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
> +	if (dat_entry1->type != dat_entry2->type)
> +		return 0;
> +
> +	return (memcmp(dat_entry1->data, dat_entry2->data,
> +			data_size) == 0 ? 1 : 0);
>  }
>  
>  /**
> @@ -198,8 +243,9 @@ static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size)
>  }
>  
>  /**
> - * batadv_hash_dat - compute the hash value for an IP address
> + * batadv_hash_dat - compute the hash value for a DAT data
>   * @data: data to hash
> + * @data_type: type of data
>   * @size: size of the hash table
>   *
>   * Returns the selected index in the hash table for the given data.
> @@ -209,7 +255,8 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
>  	uint32_t hash = 0;
>  	const struct batadv_dat_entry *dat = data;
>  
> -	hash = batadv_hash_bytes(hash, &dat->ip, sizeof(dat->ip));
> +	hash = batadv_hash_bytes(hash, dat->data,
> +				 batadv_dat_types_info[dat->type].size);
>  	hash = batadv_hash_bytes(hash, &dat->vid, sizeof(dat->vid));
>  
>  	hash += (hash << 3);
> @@ -223,32 +270,40 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
>   * batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
>   * table
>   * @bat_priv: the bat priv with all the soft interface information
> - * @ip: search key
> + * @data: search key
> + * @data_type: type of data
>   * @vid: VLAN identifier
>   *
>   * Returns the dat_entry if found, NULL otherwise.
>   */
>  static struct batadv_dat_entry *
> -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
> -			   unsigned short vid)
> +batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data,
> +			   uint8_t data_type, unsigned short vid)
>  {
>  	struct hlist_head *head;
>  	struct batadv_dat_entry to_find, *dat_entry, *dat_entry_tmp = NULL;
>  	struct batadv_hashtable *hash = bat_priv->dat.hash;
> -	uint32_t index;
> +	uint32_t index, data_size = batadv_dat_types_info[data_type].size;
>  
>  	if (!hash)
>  		return NULL;
>  
> -	to_find.ip = ip;
> +	to_find.data = kmalloc(data_size, GFP_ATOMIC);
> +	if (!to_find.data)
> +		return NULL;
> +	memcpy(to_find.data, data, data_size);

why do you create a copy of the data? It is going to be read only by the hashing
function. I think you can reuse the same data passed as argument no? You can
directly assign "to_find.data = data;", can't you?

> +	to_find.type = data_type;
>  	to_find.vid = vid;
>  
>  	index = batadv_hash_dat(&to_find, hash->size);
>  	head = &hash->table[index];
> +	kfree(to_find.data);
>  

..consequently, this kfree can be deleted.

>  	rcu_read_lock();
>  	hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
> -		if (dat_entry->ip != ip)
> +		if (dat_entry->type != data_type)
> +			continue;
> +		if (memcmp(dat_entry->data, data, data_size))
>  			continue;
>  
>  		if (!atomic_inc_not_zero(&dat_entry->refcount))
> @@ -265,25 +320,30 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
>  /**
>   * batadv_dat_entry_add - add a new dat entry or update it if already exists
>   * @bat_priv: the bat priv with all the soft interface information
> - * @ip: ipv4 to add/edit
> - * @mac_addr: mac address to assign to the given ipv4
> + * @data: the data to add/edit
> + * @data_type: type of the data added to DAT
> + * @mac_addr: mac address to assign to the given data
>   * @vid: VLAN identifier
>   */
> -static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
> -				 uint8_t *mac_addr, unsigned short vid)
> +static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
> +				 uint8_t data_type, uint8_t *mac_addr,
> +				 unsigned short vid)

here I realised we have a small conceptual problem.
what we are calling "data" is not the real data, but is the "key".
The data instead is supposed to be what we store in the entry: the mac address
(that could possibly be generalised too..).

I think generalising the data is not very important now. We have an ARP/ND table
for now, so we can assume we only want to store MAC addresses, but I would
suggest to rename the member (and so the variables) from "data" to "key".
It makes much more sense in terms of DHT. (Tell me if you think I am not right).


From now on I'll try to speed up the reviews so that we can get this thing done
soonish ;)


Cheers,
  

Hi Antonio,

Thanks for the update.
I will do the changes as soon as possible. I haven't sync in a while
with the code base, so I think the whole changes might take a while to
implement (+ I'm also working full time and trying to finish the paper
for my Diploma Project).
I hope that in 2 weeks I will finish everything because I can mostly
work on my weekends. I don't have to finish everything by the time I
present the Diploma Project, but once I have presented it, I can
dedicate all my time here (at that time I would have finished my
internship too).

But maybe that is done by then :).

Thanks,
Mihail

On 10 August 2013 04:03, Antonio Quartulli <ordex@autistici.org> wrote:
> Hi Mihail,
>
> sorry for the very looong delay :)
> I'm finally sitting at my new location and I can give you some feedback on your
> nice work.
> Comments are inline.
>
> On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
>> From: Mihail Costea <mihail.costea90@gmail.com>
>>
>> Mades DAT support more types by making its data a void*, adding type field
>> to dat_entry and adding data_type to necessary functions.
>> This change is needed in order to make DAT support any type of data, like IPv6 too.
>>
>> Adds generic function for transforming DAT data to string.
>> The function is used in order to avoid defining different debug messages
>> for different DAT data types. For example, if we had IPv6 as a DAT data,
>> then "%pI4" should be "%pI6c", but all
>> the other text of the debug message would be the same.
>> Also everything is memorized in a struct in order to avoid further
>> switch cases for all types.
>>
>> Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
>> Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
>> Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>
>>
>> ---
>>  distributed-arp-table.c |  197 +++++++++++++++++++++++++++++++++++------------
>>  distributed-arp-table.h |    1 +
>>  types.h                 |   24 +++++-
>>  3 files changed, 169 insertions(+), 53 deletions(-)
>>
>> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
>> index f2543c2..90565d0 100644
>> --- a/distributed-arp-table.c
>> +++ b/distributed-arp-table.c
>> @@ -31,9 +31,32 @@
>>  #include "types.h"
>>  #include "translation-table.h"
>>
>> +static struct batadv_dat_type_info batadv_dat_types_info[] = {
>> +     {
>> +             .size = sizeof(__be32),
>> +             .str_fmt = "%pI4",
>> +     },
>> +};
>> +
>>  static void batadv_dat_purge(struct work_struct *work);
>>
>>  /**
>> + * batadv_dat_data_to_str: transforms DAT data to string
>> + * @data: the DAT data
>> + * @type: type of data
>> + * @buf: the buf where the data string is stored
>> + * @buf_len: buf length
>> + *
>> + * Returns buf.
>> + */
>> +static char *batadv_dat_data_to_str(void *data, uint8_t type,
>> +                                 char *buf, size_t buf_len)
>> +{
>> +     snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data);
>> +return buf;
>
> alignment.
> Remember to check your patches with "checkpatch.pl --strict" before sending. It
> will tell you about these hidden mistakes.
>
>> +}
>> +
>> +/**
>>   * batadv_dat_start_timer - initialise the DAT periodic worker
>>   * @bat_priv: the bat priv with all the soft interface information
>>   */
>> @@ -45,6 +68,19 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv)
>>  }
>>
>>  /**
>> + * batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
>> + * @rcu: the dat entry rcu
>> + */
>> +static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu)
>> +{
>> +     struct batadv_dat_entry *dat_entry;
>> +
>> +     dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
>> +     kfree(dat_entry->data);
>> +     kfree(dat_entry);
>> +}
>> +
>> +/**
>>   * batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
>>   * free it
>>   * @dat_entry: the entry to free
>> @@ -52,7 +88,7 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv)
>>  static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry)
>>  {
>>       if (atomic_dec_and_test(&dat_entry->refcount))
>> -             kfree_rcu(dat_entry, rcu);
>> +             call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);
>>  }
>>
>
> since you are not using the kfree_rcu() function anymore, you should also delete
> the compatibility function we have in compat.c (and its declaration in
> compat.h).
>
>>  /**
>> @@ -136,12 +172,21 @@ static void batadv_dat_purge(struct work_struct *work)
>>   *
>>   * Returns 1 if the two entries are the same, 0 otherwise.
>>   */
>> -static int batadv_compare_dat(const struct hlist_node *node, const void *data2)
>> +static int  batadv_compare_dat(const struct hlist_node *node, const void *data2)
>>  {
>> -     const void *data1 = container_of(node, struct batadv_dat_entry,
>> -                                      hash_entry);
>> +     struct batadv_dat_entry *dat_entry1 =
>> +                     container_of(node, struct batadv_dat_entry,
>> +                                  hash_entry);
>> +     struct batadv_dat_entry *dat_entry2 =
>> +                     container_of(data2,
>> +                                  struct batadv_dat_entry, data);
>
> These assignments are really ugly :-P.
> Please make the assignments after the declarations. They will look much better.
>
>> +     size_t data_size = batadv_dat_types_info[dat_entry1->type].size;
>>
>> -     return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
>> +     if (dat_entry1->type != dat_entry2->type)
>> +             return 0;
>> +
>> +     return (memcmp(dat_entry1->data, dat_entry2->data,
>> +                     data_size) == 0 ? 1 : 0);
>>  }
>>
>>  /**
>> @@ -198,8 +243,9 @@ static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size)
>>  }
>>
>>  /**
>> - * batadv_hash_dat - compute the hash value for an IP address
>> + * batadv_hash_dat - compute the hash value for a DAT data
>>   * @data: data to hash
>> + * @data_type: type of data
>>   * @size: size of the hash table
>>   *
>>   * Returns the selected index in the hash table for the given data.
>> @@ -209,7 +255,8 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
>>       uint32_t hash = 0;
>>       const struct batadv_dat_entry *dat = data;
>>
>> -     hash = batadv_hash_bytes(hash, &dat->ip, sizeof(dat->ip));
>> +     hash = batadv_hash_bytes(hash, dat->data,
>> +                              batadv_dat_types_info[dat->type].size);
>>       hash = batadv_hash_bytes(hash, &dat->vid, sizeof(dat->vid));
>>
>>       hash += (hash << 3);
>> @@ -223,32 +270,40 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
>>   * batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
>>   * table
>>   * @bat_priv: the bat priv with all the soft interface information
>> - * @ip: search key
>> + * @data: search key
>> + * @data_type: type of data
>>   * @vid: VLAN identifier
>>   *
>>   * Returns the dat_entry if found, NULL otherwise.
>>   */
>>  static struct batadv_dat_entry *
>> -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
>> -                        unsigned short vid)
>> +batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data,
>> +                        uint8_t data_type, unsigned short vid)
>>  {
>>       struct hlist_head *head;
>>       struct batadv_dat_entry to_find, *dat_entry, *dat_entry_tmp = NULL;
>>       struct batadv_hashtable *hash = bat_priv->dat.hash;
>> -     uint32_t index;
>> +     uint32_t index, data_size = batadv_dat_types_info[data_type].size;
>>
>>       if (!hash)
>>               return NULL;
>>
>> -     to_find.ip = ip;
>> +     to_find.data = kmalloc(data_size, GFP_ATOMIC);
>> +     if (!to_find.data)
>> +             return NULL;
>> +     memcpy(to_find.data, data, data_size);
>
> why do you create a copy of the data? It is going to be read only by the hashing
> function. I think you can reuse the same data passed as argument no? You can
> directly assign "to_find.data = data;", can't you?
>
>> +     to_find.type = data_type;
>>       to_find.vid = vid;
>>
>>       index = batadv_hash_dat(&to_find, hash->size);
>>       head = &hash->table[index];
>> +     kfree(to_find.data);
>>
>
> ..consequently, this kfree can be deleted.
>
>>       rcu_read_lock();
>>       hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
>> -             if (dat_entry->ip != ip)
>> +             if (dat_entry->type != data_type)
>> +                     continue;
>> +             if (memcmp(dat_entry->data, data, data_size))
>>                       continue;
>>
>>               if (!atomic_inc_not_zero(&dat_entry->refcount))
>> @@ -265,25 +320,30 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
>>  /**
>>   * batadv_dat_entry_add - add a new dat entry or update it if already exists
>>   * @bat_priv: the bat priv with all the soft interface information
>> - * @ip: ipv4 to add/edit
>> - * @mac_addr: mac address to assign to the given ipv4
>> + * @data: the data to add/edit
>> + * @data_type: type of the data added to DAT
>> + * @mac_addr: mac address to assign to the given data
>>   * @vid: VLAN identifier
>>   */
>> -static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
>> -                              uint8_t *mac_addr, unsigned short vid)
>> +static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
>> +                              uint8_t data_type, uint8_t *mac_addr,
>> +                              unsigned short vid)
>
> here I realised we have a small conceptual problem.
> what we are calling "data" is not the real data, but is the "key".
> The data instead is supposed to be what we store in the entry: the mac address
> (that could possibly be generalised too..).
>
> I think generalising the data is not very important now. We have an ARP/ND table
> for now, so we can assume we only want to store MAC addresses, but I would
> suggest to rename the member (and so the variables) from "data" to "key".
> It makes much more sense in terms of DHT. (Tell me if you think I am not right).
>
>
> From now on I'll try to speed up the reviews so that we can get this thing done
> soonish ;)
>
>
> Cheers,
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara
  

On Sat, Aug 10, 2013 at 12:01:02PM -0700, Mihail Costea wrote:
> Hi Antonio,
> 
> Thanks for the update.
> I will do the changes as soon as possible. I haven't sync in a while
> with the code base, so I think the whole changes might take a while to
> implement (+ I'm also working full time and trying to finish the paper
> for my Diploma Project).
> I hope that in 2 weeks I will finish everything because I can mostly
> work on my weekends. I don't have to finish everything by the time I
> present the Diploma Project, but once I have presented it, I can
> dedicate all my time here (at that time I would have finished my
> internship too).
> 
> But maybe that is done by then :).

Oh, ok, then good luck for your Diploma Project and your internship!
Remember that you can also join the IRC channel if you want to ask/discuss
whatever you may need for the implementation!

Cheers,
  

Hi Antonio,

Is it possible to send the new model for the generalization as a patch
first (the part without IPv6), or maybe everything as a patch as once?
Having 5-6 patches to rewrite every time something changes makes the
development harder.

Thanks,
Mihail

On 10 August 2013 23:36, Antonio Quartulli <ordex@autistici.org> wrote:
> On Sat, Aug 10, 2013 at 12:01:02PM -0700, Mihail Costea wrote:
>> Hi Antonio,
>>
>> Thanks for the update.
>> I will do the changes as soon as possible. I haven't sync in a while
>> with the code base, so I think the whole changes might take a while to
>> implement (+ I'm also working full time and trying to finish the paper
>> for my Diploma Project).
>> I hope that in 2 weeks I will finish everything because I can mostly
>> work on my weekends. I don't have to finish everything by the time I
>> present the Diploma Project, but once I have presented it, I can
>> dedicate all my time here (at that time I would have finished my
>> internship too).
>>
>> But maybe that is done by then :).
>
> Oh, ok, then good luck for your Diploma Project and your internship!
> Remember that you can also join the IRC channel if you want to ask/discuss
> whatever you may need for the implementation!
>
> Cheers,
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara
  

On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
> Hi Antonio,
> 
> Is it possible to send the new model for the generalization as a patch
> first (the part without IPv6), or maybe everything as a patch as once?
> Having 5-6 patches to rewrite every time something changes makes the
> development harder.

Which patches do you want to merge?
If they are ready it is better to send them as PATCH to the ml and then base
your work on top of them assuming they will be merged at some point.

Cheers,

p.s. welcome to the rebase nightmare ;)
  

On 9 September 2013 17:53, Antonio Quartulli <antonio@meshcoding.com> wrote:
> On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
>> Hi Antonio,
>>
>> Is it possible to send the new model for the generalization as a patch
>> first (the part without IPv6), or maybe everything as a patch as once?
>> Having 5-6 patches to rewrite every time something changes makes the
>> development harder.
>
> Which patches do you want to merge?
> If they are ready it is better to send them as PATCH to the ml and then base
> your work on top of them assuming they will be merged at some point.
>

I took a small rest last week and now I'm redoing everything.
I was thinking about sending the first part for merging (the one with
generalization the DAT).
That is the one that needs most rewriting every time because it
affects the most existing code.
The rest I think I can send them together.

> Cheers,
>
> p.s. welcome to the rebase nightmare ;)
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

Thanls,
Mihail
  

On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
> On 9 September 2013 17:53, Antonio Quartulli <antonio@meshcoding.com> wrote:
> > On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
> >> Hi Antonio,
> >>
> >> Is it possible to send the new model for the generalization as a patch
> >> first (the part without IPv6), or maybe everything as a patch as once?
> >> Having 5-6 patches to rewrite every time something changes makes the
> >> development harder.
> >
> > Which patches do you want to merge?
> > If they are ready it is better to send them as PATCH to the ml and then base
> > your work on top of them assuming they will be merged at some point.
> >
> 
> I took a small rest last week and now I'm redoing everything.
> I was thinking about sending the first part for merging (the one with
> generalization the DAT).
> That is the one that needs most rewriting every time because it
> affects the most existing code.
> The rest I think I can send them together.

I understood. Well, the problem is also that this period is a sort of
"transition" because batman-adv is getting changed in some of its most important
part
and we would like all the "new features" that are not essential to come after
these changes.
We still need to merge two (or two and a bit) patchsets before we can start
merging other things.

This means that before your patchset gets merged we have to wait a bit more.
I think it would be better to do this:
- for a while you don't care about rebasing on top  of master
- when you have a some code ready to be reviewed you can put in on a remote git
  repo that we can check (e.g. github?)
- we/I review the code so that we make it ready to be sent as PATCH
- when these two (and a bit) patchsets are merged you can do the final rebase
  and send them to the ml for merging.

What do you think?
In this way we same some painful rebase cycles, but we can continue preparing
the code.

Cheers,
  

On 10 September 2013 08:38, Antonio Quartulli <antonio@meshcoding.com> wrote:
> On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
>> On 9 September 2013 17:53, Antonio Quartulli <antonio@meshcoding.com> wrote:
>> > On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
>> >> Hi Antonio,
>> >>
>> >> Is it possible to send the new model for the generalization as a patch
>> >> first (the part without IPv6), or maybe everything as a patch as once?
>> >> Having 5-6 patches to rewrite every time something changes makes the
>> >> development harder.
>> >
>> > Which patches do you want to merge?
>> > If they are ready it is better to send them as PATCH to the ml and then base
>> > your work on top of them assuming they will be merged at some point.
>> >
>>
>> I took a small rest last week and now I'm redoing everything.
>> I was thinking about sending the first part for merging (the one with
>> generalization the DAT).
>> That is the one that needs most rewriting every time because it
>> affects the most existing code.
>> The rest I think I can send them together.
>
> I understood. Well, the problem is also that this period is a sort of
> "transition" because batman-adv is getting changed in some of its most important
> part
> and we would like all the "new features" that are not essential to come after
> these changes.
> We still need to merge two (or two and a bit) patchsets before we can start
> merging other things.
>
> This means that before your patchset gets merged we have to wait a bit more.
> I think it would be better to do this:
> - for a while you don't care about rebasing on top  of master
> - when you have a some code ready to be reviewed you can put in on a remote git
>   repo that we can check (e.g. github?)
> - we/I review the code so that we make it ready to be sent as PATCH
> - when these two (and a bit) patchsets are merged you can do the final rebase
>   and send them to the ml for merging.
>
> What do you think?
> In this way we same some painful rebase cycles, but we can continue preparing
> the code.
>

I understand, but it should be done similar? Like multiple patches?
The idea is that I might add some patches and then find a bug that was
in an old patch.
That means to find the patch with the bug, resolve it, and re-patch
everything after it.

It would be easier to do the changes directly on the existing code
than restart everything from scratch.
I'm not sure if this is what you meant by using github.
  

On Tue, Sep 10, 2013 at 03:45:44PM +0300, Mihail Costea wrote:
> On 10 September 2013 08:38, Antonio Quartulli <antonio@meshcoding.com> wrote:
> > On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
> >> On 9 September 2013 17:53, Antonio Quartulli <antonio@meshcoding.com> wrote:
> >> > On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
> >> >> Hi Antonio,
> >> >>
> >> >> Is it possible to send the new model for the generalization as a patch
> >> >> first (the part without IPv6), or maybe everything as a patch as once?
> >> >> Having 5-6 patches to rewrite every time something changes makes the
> >> >> development harder.
> >> >
> >> > Which patches do you want to merge?
> >> > If they are ready it is better to send them as PATCH to the ml and then base
> >> > your work on top of them assuming they will be merged at some point.
> >> >
> >>
> >> I took a small rest last week and now I'm redoing everything.
> >> I was thinking about sending the first part for merging (the one with
> >> generalization the DAT).
> >> That is the one that needs most rewriting every time because it
> >> affects the most existing code.
> >> The rest I think I can send them together.
> >
> > I understood. Well, the problem is also that this period is a sort of
> > "transition" because batman-adv is getting changed in some of its most important
> > part
> > and we would like all the "new features" that are not essential to come after
> > these changes.
> > We still need to merge two (or two and a bit) patchsets before we can start
> > merging other things.
> >
> > This means that before your patchset gets merged we have to wait a bit more.
> > I think it would be better to do this:
> > - for a while you don't care about rebasing on top  of master
> > - when you have a some code ready to be reviewed you can put in on a remote git
> >   repo that we can check (e.g. github?)
> > - we/I review the code so that we make it ready to be sent as PATCH
> > - when these two (and a bit) patchsets are merged you can do the final rebase
> >   and send them to the ml for merging.
> >
> > What do you think?
> > In this way we same some painful rebase cycles, but we can continue preparing
> > the code.
> >
> 
> I understand, but it should be done similar? Like multiple patches?

multiple patches is always the way to go when we have more than one change, we
cannot mix them all.

> The idea is that I might add some patches and then find a bug that was
> in an old patch.
> That means to find the patch with the bug, resolve it, and re-patch
> everything after it.

this is normal when you have multiple patches: if a fix in the very first patch
of a series creates conflicts with all the following ones, you have to adjust
them all (this is what the "git rebase" helps you with).

> 
> It would be easier to do the changes directly on the existing code
> than restart everything from scratch.

restart everything from scratch? I did not get this.


> I'm not sure if this is what you meant by using github.
> 

for using github (or whetever else remote repository) I meant that instead of
rebasing on top of master every time you have to send the patches to the ml for
review, you could upload your code on a remote repo and have us reviewing the
code on there directly.
In this way you save the pain of respinning all your patches on top of master
every week..

I hope I clarified your doubts.

Cheers,
  

On 11 September 2013 00:01, Antonio Quartulli <antonio@meshcoding.com> wrote:
> On Tue, Sep 10, 2013 at 03:45:44PM +0300, Mihail Costea wrote:
>> On 10 September 2013 08:38, Antonio Quartulli <antonio@meshcoding.com> wrote:
>> > On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
>> >> On 9 September 2013 17:53, Antonio Quartulli <antonio@meshcoding.com> wrote:
>> >> > On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
>> >> >> Hi Antonio,
>> >> >>
>> >> >> Is it possible to send the new model for the generalization as a patch
>> >> >> first (the part without IPv6), or maybe everything as a patch as once?
>> >> >> Having 5-6 patches to rewrite every time something changes makes the
>> >> >> development harder.
>> >> >
>> >> > Which patches do you want to merge?
>> >> > If they are ready it is better to send them as PATCH to the ml and then base
>> >> > your work on top of them assuming they will be merged at some point.
>> >> >
>> >>
>> >> I took a small rest last week and now I'm redoing everything.
>> >> I was thinking about sending the first part for merging (the one with
>> >> generalization the DAT).
>> >> That is the one that needs most rewriting every time because it
>> >> affects the most existing code.
>> >> The rest I think I can send them together.
>> >
>> > I understood. Well, the problem is also that this period is a sort of
>> > "transition" because batman-adv is getting changed in some of its most important
>> > part
>> > and we would like all the "new features" that are not essential to come after
>> > these changes.
>> > We still need to merge two (or two and a bit) patchsets before we can start
>> > merging other things.
>> >
>> > This means that before your patchset gets merged we have to wait a bit more.
>> > I think it would be better to do this:
>> > - for a while you don't care about rebasing on top  of master
>> > - when you have a some code ready to be reviewed you can put in on a remote git
>> >   repo that we can check (e.g. github?)
>> > - we/I review the code so that we make it ready to be sent as PATCH
>> > - when these two (and a bit) patchsets are merged you can do the final rebase
>> >   and send them to the ml for merging.
>> >
>> > What do you think?
>> > In this way we same some painful rebase cycles, but we can continue preparing
>> > the code.
>> >
>>
>> I understand, but it should be done similar? Like multiple patches?
>
> multiple patches is always the way to go when we have more than one change, we
> cannot mix them all.
>
>> The idea is that I might add some patches and then find a bug that was
>> in an old patch.
>> That means to find the patch with the bug, resolve it, and re-patch
>> everything after it.
>
> this is normal when you have multiple patches: if a fix in the very first patch
> of a series creates conflicts with all the following ones, you have to adjust
> them all (this is what the "git rebase" helps you with).
>

I haven't used it before but I will try it now.

>>
>> It would be easier to do the changes directly on the existing code
>> than restart everything from scratch.
>
> restart everything from scratch? I did not get this.
>

The changes I'm doing now are quite big (as they change the first patch).
That will make big changes to the code base.
I will send next days the first patch for review first because it
changed how the
generalization works (more exactly I have remove mac_addr to introduce a new
void * member).

I'd like the base to be written correctly as everything depends on the
structures
introduces there.

>
>> I'm not sure if this is what you meant by using github.
>>
>
> for using github (or whetever else remote repository) I meant that instead of
> rebasing on top of master every time you have to send the patches to the ml for
> review, you could upload your code on a remote repo and have us reviewing the
> code on there directly.
> In this way you save the pain of respinning all your patches on top of master
> every week..
>
> I hope I clarified your doubts.

Thanks,
Mihail
  

On Wed, Sep 11, 2013 at 07:33:41AM +0300, Mihail Costea wrote:
> >
> > this is normal when you have multiple patches: if a fix in the very first patch
> > of a series creates conflicts with all the following ones, you have to adjust
> > them all (this is what the "git rebase" helps you with).
> >
> 
> I haven't used it before but I will try it now.

git rebase is essential to make changes at a patch series. It helps you making a
change in an initial patch and propagate the change to the rest.
This may also be why you are experiencing much difficulty in doing this
operation.

Cheers,
  

On Saturday 10 August 2013 12:01:02 Mihail Costea wrote:
> Hi Antonio,
> 
> Thanks for the update.
> I will do the changes as soon as possible. I haven't sync in a while
> with the code base, so I think the whole changes might take a while to
> implement (+ I'm also working full time and trying to finish the paper
> for my Diploma Project).
> I hope that in 2 weeks I will finish everything because I can mostly
> work on my weekends. I don't have to finish everything by the time I
> present the Diploma Project, but once I have presented it, I can
> dedicate all my time here (at that time I would have finished my
> internship too).
> 
> But maybe that is done by then :).

What is the state here [1, 2, 3, 4, 5, 6]?

Kind regards,
	Sven

[1] https://patchwork.open-mesh.org/patch/3177/
[2] https://patchwork.open-mesh.org/patch/3195/
[3] https://patchwork.open-mesh.org/patch/3194/
[4] https://patchwork.open-mesh.org/patch/3166/
[5] https://patchwork.open-mesh.org/patch/3156/
[6] https://patchwork.open-mesh.org/patch/3205/
  

On Thu, Mar 10, 2016 at 08:11:29PM +0100, Sven Eckelmann wrote:
> On Saturday 10 August 2013 12:01:02 Mihail Costea wrote:
> > Hi Antonio,
> > 
> > Thanks for the update.
> > I will do the changes as soon as possible. I haven't sync in a while
> > with the code base, so I think the whole changes might take a while to
> > implement (+ I'm also working full time and trying to finish the paper
> > for my Diploma Project).
> > I hope that in 2 weeks I will finish everything because I can mostly
> > work on my weekends. I don't have to finish everything by the time I
> > present the Diploma Project, but once I have presented it, I can
> > dedicate all my time here (at that time I would have finished my
> > internship too).
> > 
> > But maybe that is done by then :).
> 
> What is the state here [1, 2, 3, 4, 5, 6]?

There was not much interest to work on this feature and these patches are not
yet complete, therefore they should not be considered for merging.

Cheers,