Patchwork [3/3] qemu-nbd: Improve error reporting

login
register
mail settings
Submitter Ryota OZAKI
Date March 20, 2010, 6:23 a.m.
Message ID <1269066204-4376-3-git-send-email-ozaki.ryota@gmail.com>
Download mbox | patch
Permalink /patch/48202/
State New
Headers show

Comments

Ryota OZAKI - March 20, 2010, 6:23 a.m.
- use err(3) instead of errx(3) if errno is available
  to report why failed
- let fail prior to daemon(3) if opening a nbd file
  is likely to fail after daemonizing to avoid silent
  failure exit

Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
---
 qemu-nbd.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)
Aurelien Jarno - March 27, 2010, 1:02 p.m.
On Sat, Mar 20, 2010 at 03:23:24PM +0900, Ryota Ozaki wrote:
> - use err(3) instead of errx(3) if errno is available
>   to report why failed
> - let fail prior to daemon(3) if opening a nbd file
>   is likely to fail after daemonizing to avoid silent
>   failure exit
> 
> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
> ---
>  qemu-nbd.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 6d854d3..8fb8cc3 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -316,7 +316,7 @@ int main(int argc, char **argv)
>      if (disconnect) {
>          fd = open(argv[optind], O_RDWR);
>          if (fd == -1)
> -            errx(EXIT_FAILURE, "Cannot open %s", argv[optind]);
> +            err(EXIT_FAILURE, "Cannot open %s", argv[optind]);
>  
>          nbd_disconnect(fd);
>  
> @@ -333,23 +333,29 @@ int main(int argc, char **argv)
>      if (bs == NULL)
>          return 1;
>  
> -    if (bdrv_open(bs, argv[optind], flags) < 0)
> -        return 1;
> +    if ((ret = bdrv_open(bs, argv[optind], flags)) < 0) {
> +        errno = -ret;
> +        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
> +    } 
>  
>      fd_size = bs->total_sectors * 512;
>  
>      if (partition != -1 &&
>          find_partition(bs, partition, &dev_offset, &fd_size))
> -        errx(EXIT_FAILURE, "Could not find partition %d", partition);
> +        err(EXIT_FAILURE, "Could not find partition %d", partition);
>  
>      if (device) {
>          pid_t pid;
>          int sock;
>  
> +        /* want to fail before daemonizing */
> +        if (access(device, R_OK|W_OK) == -1)
> +            err(EXIT_FAILURE, "Could not access '%s'", device);
> +

First of all you need to put this line between curly braces. Secondly,
qemu-nbd as a read-only option to export a block device as read-only.
The test has to be improved to also take care of that.

>          if (!verbose) {
>              /* detach client and server */
>              if (daemon(0, 0) == -1) {
> -                errx(EXIT_FAILURE, "Failed to daemonize");
> +                err(EXIT_FAILURE, "Failed to daemonize");
>              }
>          }
>  
> -- 
> 1.6.5.2
> 
> 
> 
>
Ryota OZAKI - March 28, 2010, 12:26 p.m.
On Sat, Mar 27, 2010 at 10:02 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Sat, Mar 20, 2010 at 03:23:24PM +0900, Ryota Ozaki wrote:
>> - use err(3) instead of errx(3) if errno is available
>>   to report why failed
>> - let fail prior to daemon(3) if opening a nbd file
>>   is likely to fail after daemonizing to avoid silent
>>   failure exit
>>
>> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
>> ---
>>  qemu-nbd.c |   16 +++++++++++-----
>>  1 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 6d854d3..8fb8cc3 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -316,7 +316,7 @@ int main(int argc, char **argv)
>>      if (disconnect) {
>>          fd = open(argv[optind], O_RDWR);
>>          if (fd == -1)
>> -            errx(EXIT_FAILURE, "Cannot open %s", argv[optind]);
>> +            err(EXIT_FAILURE, "Cannot open %s", argv[optind]);
>>
>>          nbd_disconnect(fd);
>>
>> @@ -333,23 +333,29 @@ int main(int argc, char **argv)
>>      if (bs == NULL)
>>          return 1;
>>
>> -    if (bdrv_open(bs, argv[optind], flags) < 0)
>> -        return 1;
>> +    if ((ret = bdrv_open(bs, argv[optind], flags)) < 0) {
>> +        errno = -ret;
>> +        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
>> +    }
>>
>>      fd_size = bs->total_sectors * 512;
>>
>>      if (partition != -1 &&
>>          find_partition(bs, partition, &dev_offset, &fd_size))
>> -        errx(EXIT_FAILURE, "Could not find partition %d", partition);
>> +        err(EXIT_FAILURE, "Could not find partition %d", partition);
>>
>>      if (device) {
>>          pid_t pid;
>>          int sock;
>>
>> +        /* want to fail before daemonizing */
>> +        if (access(device, R_OK|W_OK) == -1)
>> +            err(EXIT_FAILURE, "Could not access '%s'", device);
>> +
>
> First of all you need to put this line between curly braces. Secondly,

Oh, sorry.

> qemu-nbd as a read-only option to export a block device as read-only.
> The test has to be improved to also take care of that.

I guess the option is intended to be only for disk image, because
open(2) for nbd device file doesn't care about the option.

Nonetheless, extending the option to operations to nbd device file
makes sense, because the option allows to open nbd device
file without write permission.

So I'll correct it as well as fixing the problems you pointed out.

Thanks,
  ozaki-r

>
>>          if (!verbose) {
>>              /* detach client and server */
>>              if (daemon(0, 0) == -1) {
>> -                errx(EXIT_FAILURE, "Failed to daemonize");
>> +                err(EXIT_FAILURE, "Failed to daemonize");
>>              }
>>          }
>>
>> --
>> 1.6.5.2
>>
>>
>>
>>
>
> --
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
>

Patch

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6d854d3..8fb8cc3 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -316,7 +316,7 @@  int main(int argc, char **argv)
     if (disconnect) {
         fd = open(argv[optind], O_RDWR);
         if (fd == -1)
-            errx(EXIT_FAILURE, "Cannot open %s", argv[optind]);
+            err(EXIT_FAILURE, "Cannot open %s", argv[optind]);
 
         nbd_disconnect(fd);
 
@@ -333,23 +333,29 @@  int main(int argc, char **argv)
     if (bs == NULL)
         return 1;
 
-    if (bdrv_open(bs, argv[optind], flags) < 0)
-        return 1;
+    if ((ret = bdrv_open(bs, argv[optind], flags)) < 0) {
+        errno = -ret;
+        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
+    } 
 
     fd_size = bs->total_sectors * 512;
 
     if (partition != -1 &&
         find_partition(bs, partition, &dev_offset, &fd_size))
-        errx(EXIT_FAILURE, "Could not find partition %d", partition);
+        err(EXIT_FAILURE, "Could not find partition %d", partition);
 
     if (device) {
         pid_t pid;
         int sock;
 
+        /* want to fail before daemonizing */
+        if (access(device, R_OK|W_OK) == -1)
+            err(EXIT_FAILURE, "Could not access '%s'", device);
+
         if (!verbose) {
             /* detach client and server */
             if (daemon(0, 0) == -1) {
-                errx(EXIT_FAILURE, "Failed to daemonize");
+                err(EXIT_FAILURE, "Failed to daemonize");
             }
         }