diff mbox

[4/5] Avoid unportable %m format

Message ID d7c2ed47a6eda87b2d97c819e3cf21cf60dc95e1.1341748181.git.blauwirbel@gmail.com
State New
Headers show

Commit Message

Blue Swirl July 8, 2012, 11:51 a.m. UTC 
From: Blue Swirl <blauwirbel@gmail.com>

Replace %m format with explicit call to standard strerror().

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 kvm-all.c       |    3 ++-
 main-loop.c     |    2 +-
 net/tap-linux.c |    8 +++++---
 qemu-nbd.c      |    3 +--
 4 files changed, 9 insertions(+), 7 deletions(-)

Comments

Anthony Liguori July 10, 2012, 8:53 p.m. UTC | #1 
On 07/08/2012 06:51 AM, blauwirbel@gmail.com wrote:
> From: Blue Swirl<blauwirbel@gmail.com>
>
> Replace %m format with explicit call to standard strerror().
>
> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>

I would expect '%m' to be thread safe whereas strerror() isn't.  I don't think 
this change is actually good.

You'd need to do something more clever with strerror_r() to be equivalent.

Regards,

Anthony Liguori

> ---
>   kvm-all.c       |    3 ++-
>   main-loop.c     |    2 +-
>   net/tap-linux.c |    8 +++++---
>   qemu-nbd.c      |    3 +--
>   4 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index f8e4328..d6f4819 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1219,7 +1219,8 @@ int kvm_init(void)
>       s->vmfd = -1;
>       s->fd = qemu_open("/dev/kvm", O_RDWR);
>       if (s->fd == -1) {
> -        fprintf(stderr, "Could not access KVM kernel module: %m\n");
> +        fprintf(stderr, "Could not access KVM kernel module: %s\n",
> +                strerror(errno));
>           ret = -errno;
>           goto err;
>       }
> diff --git a/main-loop.c b/main-loop.c
> index eb3b6e6..472c55e 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -116,7 +116,7 @@ static void sigfd_handler(void *opaque)
>           }
>
>           if (len != sizeof(info)) {
> -            printf("read from sigfd returned %zd: %m\n", len);
> +            printf("read from sigfd returned %zd: %s\n", len, strerror(errno));
>               return;
>           }
>
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 41d581b..94e5b1e 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -42,7 +42,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
>
>       TFR(fd = open(PATH_NET_TUN, O_RDWR));
>       if (fd<  0) {
> -        error_report("could not open %s: %m", PATH_NET_TUN);
> +        error_report("could not open %s: %s", PATH_NET_TUN, strerror(errno));
>           return -1;
>       }
>       memset(&ifr, 0, sizeof(ifr));
> @@ -74,9 +74,11 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
>       ret = ioctl(fd, TUNSETIFF, (void *)&ifr);
>       if (ret != 0) {
>           if (ifname[0] != '\0') {
> -            error_report("could not configure %s (%s): %m", PATH_NET_TUN, ifr.ifr_name);
> +            error_report("could not configure %s (%s): %s", PATH_NET_TUN,
> +                         ifr.ifr_name, strerror(errno));
>           } else {
> -            error_report("could not configure %s: %m", PATH_NET_TUN);
> +            error_report("could not configure %s: %s", PATH_NET_TUN,
> +                         strerror(errno));
>           }
>           close(fd);
>           return -1;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 5a0300e..099b5ed 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -213,8 +213,7 @@ static void *nbd_client_thread(void *arg)
>
>       fd = open(device, O_RDWR);
>       if (fd<  0) {
> -        /* Linux-only, we can use %m in printf.  */
> -        fprintf(stderr, "Failed to open %s: %m", device);
> +        fprintf(stderr, "Failed to open %s: %s", device, strerror(errno));
>           goto out;
>       }
>
Stefan Weil July 10, 2012, 9:12 p.m. UTC | #2 
Am 10.07.2012 22:53, schrieb Anthony Liguori:
> On 07/08/2012 06:51 AM, blauwirbel@gmail.com wrote:
>> From: Blue Swirl<blauwirbel@gmail.com>
>>
>> Replace %m format with explicit call to standard strerror().
>>
>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>
> I would expect '%m' to be thread safe whereas strerror() isn't.  I 
> don't think this change is actually good.
>
> You'd need to do something more clever with strerror_r() to be 
> equivalent.
>
> Regards,
>
> Anthony Liguori
>

Extract from the Linux manpage PRINTF(3):

        m      (Glibc extension.)  Print output of strerror(errno).  No 
argument is required.

The patch should result in identical behaviour with Glibc.
It improves the situation for all platforms which don't use Glibc.

In theory, strerror_r() might be more correct, but in the typical practical
scenarios (error output before abort) it is not really needed.

Regards,

Stefan Weil
Anthony Liguori July 10, 2012, 9:23 p.m. UTC | #3 
On 07/10/2012 04:12 PM, Stefan Weil wrote:
> Am 10.07.2012 22:53, schrieb Anthony Liguori:
>> On 07/08/2012 06:51 AM, blauwirbel@gmail.com wrote:
>>> From: Blue Swirl<blauwirbel@gmail.com>
>>>
>>> Replace %m format with explicit call to standard strerror().
>>>
>>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>>
>> I would expect '%m' to be thread safe whereas strerror() isn't. I don't think
>> this change is actually good.
>>
>> You'd need to do something more clever with strerror_r() to be equivalent.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>
> Extract from the Linux manpage PRINTF(3):
>
> m (Glibc extension.) Print output of strerror(errno). No argument is required.

You're reading the docs too literally:

http://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/vfprintf.c;h=d5690342536bc8cf948c786f663bb63f73f91f3a;hb=HEAD#l966

It uses strerror_r() and is thread-safe.  Converting to strerror() removes that 
safety.

Regards,

Anthony Liguori
Blue Swirl July 12, 2012, 7:43 p.m. UTC | #4 
On Tue, Jul 10, 2012 at 8:53 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 07/08/2012 06:51 AM, blauwirbel@gmail.com wrote:
>>
>> From: Blue Swirl<blauwirbel@gmail.com>
>>
>> Replace %m format with explicit call to standard strerror().
>>
>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>
>
> I would expect '%m' to be thread safe whereas strerror() isn't.  I don't
> think this change is actually good.
>
> You'd need to do something more clever with strerror_r() to be equivalent.

There's also g_strerror() which has a simpler interface.

>
> Regards,
>
> Anthony Liguori
>
>
>> ---
>>   kvm-all.c       |    3 ++-
>>   main-loop.c     |    2 +-
>>   net/tap-linux.c |    8 +++++---
>>   qemu-nbd.c      |    3 +--
>>   4 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index f8e4328..d6f4819 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1219,7 +1219,8 @@ int kvm_init(void)
>>       s->vmfd = -1;
>>       s->fd = qemu_open("/dev/kvm", O_RDWR);
>>       if (s->fd == -1) {
>> -        fprintf(stderr, "Could not access KVM kernel module: %m\n");
>> +        fprintf(stderr, "Could not access KVM kernel module: %s\n",
>> +                strerror(errno));
>>           ret = -errno;
>>           goto err;
>>       }
>> diff --git a/main-loop.c b/main-loop.c
>> index eb3b6e6..472c55e 100644
>> --- a/main-loop.c
>> +++ b/main-loop.c
>> @@ -116,7 +116,7 @@ static void sigfd_handler(void *opaque)
>>           }
>>
>>           if (len != sizeof(info)) {
>> -            printf("read from sigfd returned %zd: %m\n", len);
>> +            printf("read from sigfd returned %zd: %s\n", len,
>> strerror(errno));
>>               return;
>>           }
>>
>> diff --git a/net/tap-linux.c b/net/tap-linux.c
>> index 41d581b..94e5b1e 100644
>> --- a/net/tap-linux.c
>> +++ b/net/tap-linux.c
>> @@ -42,7 +42,7 @@ int tap_open(char *ifname, int ifname_size, int
>> *vnet_hdr, int vnet_hdr_required
>>
>>       TFR(fd = open(PATH_NET_TUN, O_RDWR));
>>       if (fd<  0) {
>> -        error_report("could not open %s: %m", PATH_NET_TUN);
>> +        error_report("could not open %s: %s", PATH_NET_TUN,
>> strerror(errno));
>>           return -1;
>>       }
>>       memset(&ifr, 0, sizeof(ifr));
>> @@ -74,9 +74,11 @@ int tap_open(char *ifname, int ifname_size, int
>> *vnet_hdr, int vnet_hdr_required
>>       ret = ioctl(fd, TUNSETIFF, (void *)&ifr);
>>       if (ret != 0) {
>>           if (ifname[0] != '\0') {
>> -            error_report("could not configure %s (%s): %m", PATH_NET_TUN,
>> ifr.ifr_name);
>> +            error_report("could not configure %s (%s): %s", PATH_NET_TUN,
>> +                         ifr.ifr_name, strerror(errno));
>>           } else {
>> -            error_report("could not configure %s: %m", PATH_NET_TUN);
>> +            error_report("could not configure %s: %s", PATH_NET_TUN,
>> +                         strerror(errno));
>>           }
>>           close(fd);
>>           return -1;
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 5a0300e..099b5ed 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -213,8 +213,7 @@ static void *nbd_client_thread(void *arg)
>>
>>       fd = open(device, O_RDWR);
>>       if (fd<  0) {
>> -        /* Linux-only, we can use %m in printf.  */
>> -        fprintf(stderr, "Failed to open %s: %m", device);
>> +        fprintf(stderr, "Failed to open %s: %s", device,
>> strerror(errno));
>>           goto out;
>>       }
>>
>
diff mbox

Patch

diff --git a/kvm-all.c b/kvm-all.c
index f8e4328..d6f4819 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1219,7 +1219,8 @@  int kvm_init(void)
     s->vmfd = -1;
     s->fd = qemu_open("/dev/kvm", O_RDWR);
     if (s->fd == -1) {
-        fprintf(stderr, "Could not access KVM kernel module: %m\n");
+        fprintf(stderr, "Could not access KVM kernel module: %s\n",
+                strerror(errno));
         ret = -errno;
         goto err;
     }
diff --git a/main-loop.c b/main-loop.c
index eb3b6e6..472c55e 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -116,7 +116,7 @@  static void sigfd_handler(void *opaque)
         }
 
         if (len != sizeof(info)) {
-            printf("read from sigfd returned %zd: %m\n", len);
+            printf("read from sigfd returned %zd: %s\n", len, strerror(errno));
             return;
         }
 
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 41d581b..94e5b1e 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -42,7 +42,7 @@  int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
 
     TFR(fd = open(PATH_NET_TUN, O_RDWR));
     if (fd < 0) {
-        error_report("could not open %s: %m", PATH_NET_TUN);
+        error_report("could not open %s: %s", PATH_NET_TUN, strerror(errno));
         return -1;
     }
     memset(&ifr, 0, sizeof(ifr));
@@ -74,9 +74,11 @@  int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
     ret = ioctl(fd, TUNSETIFF, (void *) &ifr);
     if (ret != 0) {
         if (ifname[0] != '\0') {
-            error_report("could not configure %s (%s): %m", PATH_NET_TUN, ifr.ifr_name);
+            error_report("could not configure %s (%s): %s", PATH_NET_TUN,
+                         ifr.ifr_name, strerror(errno));
         } else {
-            error_report("could not configure %s: %m", PATH_NET_TUN);
+            error_report("could not configure %s: %s", PATH_NET_TUN,
+                         strerror(errno));
         }
         close(fd);
         return -1;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 5a0300e..099b5ed 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -213,8 +213,7 @@  static void *nbd_client_thread(void *arg)
 
     fd = open(device, O_RDWR);
     if (fd < 0) {
-        /* Linux-only, we can use %m in printf.  */
-        fprintf(stderr, "Failed to open %s: %m", device);
+        fprintf(stderr, "Failed to open %s: %s", device, strerror(errno));
         goto out;
     }