Re: [B.A.T.M.A.N.] [PATCH] Added generic transformer to string function for DAT data

Hi Mihail,

You should first send the patch introducing the dat_entry->type field, otherwise this one cannot apply.. Then please add a few lines commit message describing what this patch is bringing and why it would be nice to have this new function (e.g. talk about future uses with many dat_entry types..).

However I put some comments inline

On Mon, Apr 22, 2013 at 02:12:18PM +0300, YourName wrote:

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 | 71 +++++++++++++++++++++++++++++++++++++++++------ types.h | 8 ++++++ 2 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/distributed-arp-table.c b/distributed-arp-table.c index 3a3e1d8..5df8f19 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -34,6 +34,36 @@ 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
• • Returns the string representation of data. This should be freed with kfree.
• */

+static char *batadv_dat_data_to_str(void *data, uint8_t type) +{

• size_t buf_size;
• char *buf, *format_type, ipv4[] = "%pI4";

char ipv4[] should instead be const char *ipv4. However, I think a more general approach should be nice. Look at sysfs.c:46 for an example. In that case we have an array of strings where for a given item with value 0, you find its string representation exactly at index 0. For DAT it may be the same:

if you do something like

enum batadv_dat_types { BATADV_DAT_IPV4 = 0, BATADV_DAT_CHACHA = 1, };

then you can have an array of strings where you can put the format to use with Ipv4 exactly at poistion 0 and the format to use for CHACHA at position 1:

static char *batadv_dat_types_str_fmt[] { "%pI4", "%chacha", }

This makes the code easier to be read by other people and also easy to be improved in the future.

• switch (type) {
• case BATADV_DAT_IPV4:

• /* maximum length for an IPv4 string representation + NULL */
  

• buf_size = 16;
  

• format_type = ipv4;
  

• break;
  

• default:

• return NULL;
  

• }

If you use the same approach for the buf_size (another static array, say batadv_dat_type_str_len) then you can get rid of this switch (which otherwise would grow continuously as soon as we add new types) and use a couple of lines only.

• buf = kmalloc(buf_size, GFP_ATOMIC);
• if (!buf)

• return NULL;
  

• snprintf(buf, buf_size, format_type, data);
• return buf;

+}

for what concern the buffer, what do you think about using an approach similar to inet_ntop() ? Instead of allocating a buffer inside (which may fail as you correctly handled) the function could receive a buffer and its len directly from outside and use it (and return it). In this way you could easily put this function directly into a batadv_dbg().

E.g.:

*batadv_dat_data_to_str(data, type, buf, buf_len) { /* play with buf and ensure we don't exceed buf_len */ return buf; }

... char buf[20]; ... batadv_dbg(BATADV_DBG_DAT, bat_priv, "this and that %s\n", batadv_dat_data_to_str(data, type, buf, sizeof(buf))); ...

I think the code using batadv_dat_data_to_str() will become smaller and easier, no?

+/**

• batadv_dat_start_timer - initialise the DAT periodic worker
• @bat_priv: the bat priv with all the soft interface information

*/ @@ -272,6 +302,7 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip, { struct batadv_dat_entry *dat_entry; int hash_added;

• char *dbg_data;

  dat_entry = batadv_dat_entry_hash_find(bat_priv, ip); /* if this entry is already known, just update it */

@@ -279,9 +310,15 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip, if (!batadv_compare_eth(dat_entry->mac_addr, mac_addr)) memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN); dat_entry->last_update = jiffies;

• batadv_dbg(BATADV_DBG_DAT, bat_priv,
  

• 	   "Entry updated: %pI4 %pM\n", &dat_entry->ip,
  

• 	   dat_entry->mac_addr);
  

• dbg_data = batadv_dat_data_to_str(&dat_entry->ip,
  

• 				  BATADV_DAT_IPV4);
  

• if (dbg_data) {
  

• 	batadv_dbg(BATADV_DBG_DAT, bat_priv,
  

• 		   "Entry updated: %s %pM\n", dbg_data,
  

• 		   dat_entry->mac_addr);
  

• 	kfree(dbg_data);
  

• }
  

  goto out; }

@@ -304,8 +341,13 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip, goto out; }

• batadv_dbg(BATADV_DBG_DAT, bat_priv, "New entry added: %pI4 %pM\n",

•    &dat_entry->ip, dat_entry->mac_addr);
  

• dbg_data = batadv_dat_data_to_str(&dat_entry->ip, BATADV_DAT_IPV4);
• if (dbg_data) {

• batadv_dbg(BATADV_DBG_DAT, bat_priv,
  

• 	   "New entry added: %s %pM\n", dbg_data,
  

• 	   dat_entry->mac_addr);
  

• kfree(dbg_data);
  

• }

out: if (dat_entry) @@ -527,6 +569,7 @@ batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst) int select; batadv_dat_addr_t last_max = BATADV_DAT_ADDR_MAX, ip_key; struct batadv_dat_candidate *res;

• char *dbg_data;

  if (!bat_priv->orig_hash) return NULL;

@@ -538,9 +581,13 @@ batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst) ip_key = (batadv_dat_addr_t)batadv_hash_dat(&ip_dst, BATADV_DAT_ADDR_MAX);

• batadv_dbg(BATADV_DBG_DAT, bat_priv,

•    "dat_select_candidates(): IP=%pI4 hash(IP)=%u\n", &ip_dst,
  

•    ip_key);
  

• dbg_data = batadv_dat_data_to_str(&ip_dst, BATADV_DAT_IPV4);

• if (dbg_data) {

• batadv_dbg(BATADV_DBG_DAT, bat_priv,
  

• 	   "dat_select_candidates(): IP=%s hash(IP)=%u\n",
  

• 	   dbg_data, ip_key);
  

• kfree(dbg_data);
  

• }

  for (select = 0; select < BATADV_DAT_CANDIDATES_NUM; select++) batadv_choose_next_candidate(bat_priv, res, select, ip_key,

@@ -572,12 +619,18 @@ static bool batadv_dat_send_data(struct batadv_priv *bat_priv, struct batadv_neigh_node *neigh_node = NULL; struct sk_buff *tmp_skb; struct batadv_dat_candidate *cand;

• char *dbg_data;

  cand = batadv_dat_select_candidates(bat_priv, ip); if (!cand) goto out;

• batadv_dbg(BATADV_DBG_DAT, bat_priv, "DHT_SEND for %pI4\n", &ip);

• dbg_data = batadv_dat_data_to_str(&ip, BATADV_DAT_IPV4);

• if (dbg_data) {

• batadv_dbg(BATADV_DBG_DAT, bat_priv,
  

• 	   "DHT_SEND for %s\n", dbg_data);
  

• kfree(dbg_data);
  

• }

  for (i = 0; i < BATADV_DAT_CANDIDATES_NUM; i++) { if (cand[i].type == BATADV_DAT_CANDIDATE_NOT_FOUND)

diff --git a/types.h b/types.h index b2c94e1..3488528 100644 --- a/types.h +++ b/types.h @@ -980,6 +980,14 @@ struct batadv_dat_entry { };

/**

• • batadv_dat_types - types used in batadv_dat_entry for IP
• • @BATADV_DAT_IPv4: IPv4 address type
• */

+enum batadv_dat_types {

• BATADV_DAT_IPV4,

+};

if you use the approach I proposed above, remember to assign values to the enum constants (at least tot he first) so to ensure they have the values you want.

+/**

• struct batadv_dat_candidate - candidate destination for DAT operations
• @type: the type of the selected candidate. It can one of the following:

•  - BATADV_DAT_CANDIDATE_NOT_FOUND
  

-- 1.7.10.4

Thanks a lot so far! If you have any question, comment or whatever feel free to interact with us on the batman-adv IRC channel (#batman @ freenode)!

Cheers,