What's the most elegant way of doing this?

[Alex Buell]

I have the following snippet (only works on Linux), which works
perfectly when I need to output only one hardware address at a time,
but when I have occasions to print two diffferent ones (i.e source and
destination hardware addresses), it will print two copies of the same
hardware address, which isn't what I wanted, and yes I do know why that
happens but my workarounds for this looks quite kludgy involving
separate character arrays for each address output.

Can anyone come up with a much better and a lot more elegant solution
than this?

Thanks, much appreciated!

/* cut here */
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <net/ethernet.h>

char *hardware_address_string(unsigned char *hardware_address)
{
static char buffer[ETH_ALEN * 5 + 2];
char *p = (char *)buffer;
unsigned char *q = (unsigned char *)hardware_address;

if (hardware_address)
{
for (; q < (hardware_address + ETH_ALEN); q++)
{
sprintf(p, "%02X", (unsigned char)*q);
p += 2;

if (q < hardware_address + ETH_ALEN - 1)
sprintf(p++, ":");
}
}

return buffer;
}

int main()
{
unsigned char addr1[] = { 0x0, 0x50, 0x56, 0xc0, 0x0, 0x01 };
unsigned char addr2[] = { 0x0, 0x0d, 0x56, 0x38, 0x97, 0x33 };
char buffer[80];

sprintf(buffer, "Addr1 = %s, addr2 = %s",
hardware_address_string(addr1),
hardware_address_string(addr2));

printf("%s\n", buffer);
return 0;
}
/* end of snippet */

 

[user923005]

I have the following snippet (only works on Linux), which works
perfectly when I need to output only one hardware address at a time,
but when I have occasions to print two diffferent ones (i.e source and
destination hardware addresses), it will print two copies of the same
hardware address, which isn't what I wanted, and yes I do know why that
happens but my workarounds for this looks quite kludgy involving
separate character arrays for each address output.

Can anyone come up with a much better and a lot more elegant solution
than this?

Thanks, much appreciated!

/* cut here */
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <net/ethernet.h>

char *hardware_address_string(unsigned char *hardware_address)
{
        static char buffer[ETH_ALEN * 5 + 2];
        char *p = (char *)buffer;
        unsigned char *q = (unsigned char *)hardware_address;

        if (hardware_address)
        {
                for (; q < (hardware_address + ETH_ALEN); q++)
                {
                        sprintf(p, "%02X", (unsigned char)*q);
                        p += 2;

                        if (q < hardware_address + ETH_ALEN - 1)
                                sprintf(p++, ":");
                }
        }

        return buffer;

}

int main()
{
        unsigned char addr1[] = { 0x0, 0x50, 0x56, 0xc0, 0x0, 0x01 };
        unsigned char addr2[] = { 0x0, 0x0d, 0x56, 0x38, 0x97, 0x33 };
        char buffer[80];

        sprintf(buffer, "Addr1 = %s, addr2 = %s",
                hardware_address_string(addr1),
hardware_address_string(addr2));

        printf("%s\n", buffer);
        return 0;}

From the C-FAQ:
=======================================================================
7.5b: So what's the right way to return a string or other aggregate?

A: The returned pointer should be to a statically-allocated buffer
(as in the answer to question 7.5a), or to a buffer passed in by
the caller, or to memory obtained with malloc(), but *not* to a
local (automatic) array.

See also question 20.1.
=======================================================================
Now, in your case you want to have two simultaneous, distinct values.
That eliminates the use of a static variable.
Hence, you can either pass in a string to hold the value and tell the
function also how big the string is or you could allocate the string
with malloc().

 

[Kaz Kylheku]

I have the following snippet (only works on Linux), which works
perfectly when I need to output only one hardware address at a time,
but when I have occasions to print two diffferent ones (i.e source and
destination hardware addresses), it will print two copies of the same
hardware address, which isn't what I wanted, and yes I do know why that
happens but my workarounds for this looks quite kludgy involving
separate character arrays for each address output.

Can anyone come up with a much better and a lot more elegant solution
than this?

Thanks, much appreciated!

/* cut here */
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <net/ethernet.h>

Avoid including Linux kernel headers in user space programs, unless
you have an excellent reason for doing so, like needing to access things
in the kernel, with no other interface being provided.

char *hardware_address_string(unsigned char *hardware_address)
{

Since ethernet addresses are not specific to Linux, it does not make sense to
depend on a Linux-specific header for this. For instance instead of relying on
the constant ETH_ALEN, you could just define your own. How many bytes long an
ethernet address is isn't Linux specific.

The task of converting a bunch of bytes in an array to a printed notation can
be done in completely portable C.

static char buffer[ETH_ALEN * 5 + 2];
char *p = (char *)buffer;
unsigned char *q = (unsigned char *)hardware_address;

if (hardware_address)
{
for (; q < (hardware_address + ETH_ALEN); q++)
{
sprintf(p, "%02X", (unsigned char)*q);
p += 2;

if (q < hardware_address + ETH_ALEN - 1)
sprintf(p++, ":");
}
}

Note that the above code is longer than this:

sprintf(buffer, "%02x:%02x:%02x:%02x:%02x:%02x",
a[0], a[1], a[2], a[3], a[4], a[5]);

You can generalize this into a pair of macros that can be used in any printf
situation:

#define ETHER_FMT "%02x:%02x:%02x:%02x:%02x:%02x"
#define ETHER_FMT_ARG(P) (P)[0], (P)[1], (P)[2], (P)[3], (P)[4], (P)[5]

This depends on P being, or decaying, to a byte pointer. If it's something
like void *, you have to cast.

So now you can do:

unsigned char vmware_addr[] = { 0x0, 0x50, 0x56, 0xc0, 0x0, 0x01 };

printf("addr is " ETHER_FMT "\n", ETHER_FMT_ARG(vmware_addr));

See? No sprintf to a buffer, no buffer overflow risks. No Linux
kernel headers in user space compiling.

While we are on the topic of the Linux kernel, take a look at the
NIPQUAD and NIPQUAD_FMT macros in include/linux/kernel.h. They are
similar to the above idea, for printing IPv4 addresses.

 

[Alex Buell]

Avoid including Linux kernel headers in user space programs, unless
you have an excellent reason for doing so, like needing to access
things in the kernel, with no other interface being provided.

In this case, yes I do have a very good reason for including linux
headers as the daemon I'm developing will eventually become part of
the kernel in the future.

Since ethernet addresses are not specific to Linux, it does not make
sense to depend on a Linux-specific header for this. For instance
instead of relying on the constant ETH_ALEN, you could just define
your own. How many bytes long an ethernet address is isn't Linux
specific.

The task of converting a bunch of bytes in an array to a printed
notation can be done in completely portable C.

If this was a purely userspace program, I'd agree and thank you for
the advice.

Note that the above code is longer than this:

sprintf(buffer, "%02x:%02x:%02x:%02x:%02x:%02x",
a[0], a[1], a[2], a[3], a[4], a[5]);

You can generalize this into a pair of macros that can be used in any
printf situation:

#define ETHER_FMT "%02x:%02x:%02x:%02x:%02x:%02x"
#define ETHER_FMT_ARG(P) (P)[0], (P)[1], (P)[2], (P)[3], (P)[4],
(P)[5]

Thank you, that solution works far better than anything I've tried.
None of the other solutions posed were as good as this.

While we are on the topic of the Linux kernel, take a look at the
NIPQUAD and NIPQUAD_FMT macros in include/linux/kernel.h. They are
similar to the above idea, for printing IPv4 addresses.

That's an bonus, thank you very much.