2010-06-02 20:10:21

by Joe Perches

[permalink] [raw]
Subject: [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

Remove the last uses of MAC_FMT

Signed-off-by: Joe Perches <[email protected]>
---
drivers/staging/batman-adv/main.c | 3 +-
drivers/staging/batman-adv/translation-table.c | 25 ++++-------------------
2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/batman-adv/main.c b/drivers/staging/batman-adv/main.c
index 74c70d5..72851cd 100644
--- a/drivers/staging/batman-adv/main.c
+++ b/drivers/staging/batman-adv/main.c
@@ -226,8 +226,7 @@ void dec_module_count(void)

int addr_to_string(char *buff, uint8_t *addr)
{
- return sprintf(buff, MAC_FMT,
- addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
+ return sprintf(buff, "%pM", addr);
}

/* returns 1 if they are the same originator */
diff --git a/drivers/staging/batman-adv/translation-table.c b/drivers/staging/batman-adv/translation-table.c
index e01ff21..63d0967 100644
--- a/drivers/staging/batman-adv/translation-table.c
+++ b/drivers/staging/batman-adv/translation-table.c
@@ -202,13 +202,8 @@ int hna_local_fill_buffer_text(struct net_device *net_dev, char *buff,
hna_local_entry = hashit.bucket->data;

bytes_written += snprintf(buff + bytes_written, 22,
- " * " MAC_FMT "\n",
- hna_local_entry->addr[0],
- hna_local_entry->addr[1],
- hna_local_entry->addr[2],
- hna_local_entry->addr[3],
- hna_local_entry->addr[4],
- hna_local_entry->addr[5]);
+ " * %pM\n",
+ hna_local_entry->addr);
}

spin_unlock_irqrestore(&hna_local_hash_lock, flags);
@@ -420,19 +415,9 @@ int hna_global_fill_buffer_text(struct net_device *net_dev, char *buff,
hna_global_entry = hashit.bucket->data;

bytes_written += snprintf(buff + bytes_written, 44,
- " * " MAC_FMT " via " MAC_FMT "\n",
- hna_global_entry->addr[0],
- hna_global_entry->addr[1],
- hna_global_entry->addr[2],
- hna_global_entry->addr[3],
- hna_global_entry->addr[4],
- hna_global_entry->addr[5],
- hna_global_entry->orig_node->orig[0],
- hna_global_entry->orig_node->orig[1],
- hna_global_entry->orig_node->orig[2],
- hna_global_entry->orig_node->orig[3],
- hna_global_entry->orig_node->orig[4],
- hna_global_entry->orig_node->orig[5]);
+ " * %pM via %pM\n",
+ hna_global_entry->addr,
+ hna_global_entry->orig_node->orig);
}

spin_unlock_irqrestore(&hna_global_hash_lock, flags);



2010-06-02 23:23:33

by Sven Eckelmann

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

Joe Perches wrote:
> Remove the last uses of MAC_FMT
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/staging/batman-adv/main.c | 3 +-
> drivers/staging/batman-adv/translation-table.c | 25
> ++++------------------- 2 files changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/batman-adv/main.c
> b/drivers/staging/batman-adv/main.c index 74c70d5..72851cd 100644
> --- a/drivers/staging/batman-adv/main.c
> +++ b/drivers/staging/batman-adv/main.c
> @@ -226,8 +226,7 @@ void dec_module_count(void)
>
> int addr_to_string(char *buff, uint8_t *addr)
> {
> - return sprintf(buff, MAC_FMT,
> - addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
> + return sprintf(buff, "%pM", addr);
> }
[...]

Thanks for your patch.

We must currently support older kernels which doesn't support %pM. Thats why
we have an extra wrapper for printk in the out-of-kernel module. The same
would have to be done for sprintf as well. Most of the developers are
currently attending the Wireless Battle Mesh v3 - so the processing of that
patch is delayed a little bit.

The same problem arises with your patch "Use (pr|netdev)_<level> macro helper"
(which seems to be added in 2.6.34). But I think we could add a wrapper for
older kernels easily - but as mentioned before this is probably postponed
until next week or so.

You've also changed the output. So you may partly broke batctl too (have to
check that first).

Best regards,
Sven


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2010-06-02 23:32:08

by Joe Perches

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

On Thu, 2010-06-03 at 01:23 +0200, Sven Eckelmann wrote:
> You've also changed the output. So you may partly broke batctl too (have to
> check that first).

There was no change in output.
MAC_FMT output is the same as %pM

cheers, Joe

2010-06-02 23:33:39

by Sven Eckelmann

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

Joe Perches wrote:
> On Thu, 2010-06-03 at 01:23 +0200, Sven Eckelmann wrote:
> > You've also changed the output. So you may partly broke batctl too (have
> > to check that first).
>
> There was no change in output.
> MAC_FMT output is the same as %pM

I meant the other patch :)

Best regards,
Sven


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2010-06-02 23:47:33

by Joe Perches

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

On Thu, 2010-06-03 at 01:33 +0200, Sven Eckelmann wrote:
> Joe Perches wrote:
> > On Thu, 2010-06-03 at 01:23 +0200, Sven Eckelmann wrote:
> > > You've also changed the output. So you may partly broke batctl too (have
> > > to check that first).
> > There was no change in output.
> > MAC_FMT output is the same as %pM
> I meant the other patch :)

batctl is parsing dmesg or equivalent? ouch.

If so, may I suggest you consider using something other than
a message logging parser for batctl?

I glanced at the source and don't see any such use.
It looks like it's only /sys, /proc and debugfs,
but I only spent a few seconds at it.

http://www.open-mesh.net/changeset/1682/trunk?old_path=%2F&format=zip

cheers, Joe

2010-06-02 23:56:43

by Sven Eckelmann

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

Joe Perches wrote:
> On Thu, 2010-06-03 at 01:33 +0200, Sven Eckelmann wrote:
> > Joe Perches wrote:
> > > On Thu, 2010-06-03 at 01:23 +0200, Sven Eckelmann wrote:
> > > > You've also changed the output. So you may partly broke batctl too
> > > > (have to check that first).
> > >
> > > There was no change in output.
> > > MAC_FMT output is the same as %pM
> >
> > I meant the other patch :)
>
> batctl is parsing dmesg or equivalent? ouch.

It has functionality to parse logfiles which for example could come from
dmesg.

> If so, may I suggest you consider using something other than
> a message logging parser for batctl?
>
> I glanced at the source and don't see any such use.
> It looks like it's only /sys, /proc and debugfs,
> but I only spent a few seconds at it.
>
> http://www.open-mesh.net/changeset/1682/trunk?old_path=%2F&format=zip

see sys.c -> log_print
and read_file -> read_file

Best regards,
Sven


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2010-06-03 00:20:05

by Joe Perches

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

On Thu, 2010-06-03 at 01:56 +0200, Sven Eckelmann wrote:
> see sys.c -> log_print
> and read_file -> read_file

Yuck.

The patch changes the prefix from "batman-adv:" to "batman_adv: "
so yes, it would break as-is.

I think the concept is broken though, I believe dmesg output
specifically is not guaranteed to remain stable, and batman should
use some other, perhaps private, logger based on ethtool events.

cheers, Joe

2010-06-03 09:22:41

by Marek Lindner

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

On Thursday 03 June 2010 08:20:01 Joe Perches wrote:
> The patch changes the prefix from "batman-adv:" to "batman_adv: "
> so yes, it would break as-is.
>
> I think the concept is broken though, I believe dmesg output
> specifically is not guaranteed to remain stable, and batman should
> use some other, perhaps private, logger based on ethtool events.

I think changing a dash to an underscore is not such a big deal (I did not
check the rest of the patch yet). But I'm interested to hear more about your
"private logger" idea because the current solution is far from being perfect.
As we have to debug the routing protocol every now and then it would be very
helpful to get direct access to some internal logging facility. In fact, that
existed before (inside of /proc) but was removed to be more compliant with the
linux kernel and the existing log facilities.

Regards,
Marek

2010-06-03 14:51:18

by Sven Eckelmann

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

Joe Perches wrote:
> Remove the last uses of MAC_FMT
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/staging/batman-adv/main.c | 3 +-
> drivers/staging/batman-adv/translation-table.c | 25
> ++++------------------- 2 files changed, 6 insertions(+), 22 deletions(-)

Just noticed that this patch collides with another one which is waiting in
GregKH's queue. I will try to rewrite it on top of that. I hope that this is
ok for you.

Best regards,
Sven


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2010-06-03 15:01:33

by Greg KH

[permalink] [raw]
Subject: Re: [B.A.T.M.A.N.] [PATCH] drivers/staging/batman-adv: Convert MAC_FMT to %pM

On Thu, Jun 03, 2010 at 05:15:53PM +0800, Marek Lindner wrote:
> On Thursday 03 June 2010 08:20:01 Joe Perches wrote:
> > The patch changes the prefix from "batman-adv:" to "batman_adv: "
> > so yes, it would break as-is.
> >
> > I think the concept is broken though, I believe dmesg output
> > specifically is not guaranteed to remain stable, and batman should
> > use some other, perhaps private, logger based on ethtool events.
>
> I think changing a dash to an underscore is not such a big deal (I did not
> check the rest of the patch yet). But I'm interested to hear more about your
> "private logger" idea because the current solution is far from being perfect.
> As we have to debug the routing protocol every now and then it would be very
> helpful to get direct access to some internal logging facility. In fact, that
> existed before (inside of /proc) but was removed to be more compliant with the
> linux kernel and the existing log facilities.

You can always use debugfs if you like for something like this.

Or tie into the profile/perf subsystem, that might be even easier.

thanks,

greg k-h