Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Split batadv_priv in sub-structures for features

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 2306 bytes --]

Hello Sven,

some inline comments about the TT part.


On Sun, Jul 08, 2012 at 06:49:06 +0200, Sven Eckelmann wrote:
> The structure batadv_priv grows everytime a new feature is introduced. It gets
> hard to find the parts of the struct that belongs to a specific feature. This
> becomes even harder by the fact that not every feature uses a prefix in the
> member name.
> 
> The variables for bridge loop avoidence, gateway handling, translation table
> and visualization server are moved into separate structs that are included in
> the bat_priv main struct.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
  
> +struct batadv_priv_tt {
> +	atomic_t vn; /* translation table version number */
> +	atomic_t ogm_append_cnt;
> +	atomic_t local_changes; /* changes registered in a OGM interval */

please, substitute OGM with originator

> +	/* The tt_poss_change flag is used to detect an ongoing roaming phase.
> +	 * If true, then I received a Roaming_adv and I have to inspect every
> +	 * packet directed to me to check whether I am still the true
> +	 * destination or not. This flag will be reset to false as soon as I
> +	 * increase my TTVN
> +	 */
> +	bool poss_change;
> +	struct list_head changes_list; /* tracks changes in a OGM int */

please rephrase the comment as: /* tracks tt local changes within an originator interval */

> +	struct batadv_hashtable *local_hash;
> +	struct batadv_hashtable *global_hash;
> +	struct list_head req_list; /* list of pending tt_requests */
> +	struct list_head roam_list;
> +	spinlock_t changes_list_lock; /* protects changes */
> +	spinlock_t req_list_lock; /* protects req_list */
> +	spinlock_t roam_list_lock; /* protects roam_list */
> +	atomic_t num_local;

I'd prefer:

atomic_t local_entry_num;


> +	/* Checksum of the local table, recomputed before sending a new OGM */
> +	uint16_t crc;

uint16_t local_crc

> +	unsigned char *buff;

unsigned char* last_changeset;

> +	int16_t buff_len;

int16_t last_changeset_len;

> +	spinlock_t buff_lock; /* protects buff */

spinlock_t last_changeset_lock; /* protects buff_lock */

> +	struct delayed_work work;
> +};

Thank you,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]