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

login
register
mail settings
Submitter Ryota OZAKI
Date March 28, 2010, 5:07 p.m.
Message ID <1269796032-9166-3-git-send-email-ozaki.ryota@gmail.com>
Download mbox | patch
Permalink /patch/48783/
State New
Headers show

Comments

Ryota OZAKI - March 28, 2010, 5:07 p.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
- add missing 'ret = 1' when unix_socket_outgoing failed

Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
---
 qemu-nbd.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)
Kevin Wolf - March 29, 2010, 11:03 a.m.
Am 28.03.2010 19:07, schrieb Ryota Ozaki:
> - 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
> - add missing 'ret = 1' when unix_socket_outgoing failed
> 
> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>

> @@ -343,25 +343,31 @@ int main(int argc, char **argv)
>          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]);
>      }

If you do it like this, you could do the change for even more errors,
like the ones returned by bdrv_read. But that doesn't make this patch
less correct, of course.

Kevin
Ryota OZAKI - May 2, 2010, 9:52 p.m.
On Mon, Mar 29, 2010 at 8:03 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 28.03.2010 19:07, schrieb Ryota Ozaki:
>> - 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
>> - add missing 'ret = 1' when unix_socket_outgoing failed
>>
>> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
>
> Acked-by: Kevin Wolf <kwolf@redhat.com>
>
>> @@ -343,25 +343,31 @@ int main(int argc, char **argv)
>>          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]);
>>      }
>
> If you do it like this, you could do the change for even more errors,
> like the ones returned by bdrv_read. But that doesn't make this patch
> less correct, of course.

Indeed. So I will include the changes for bdrv_read as well
in the next round.

Thanks,
  ozaki-r

>
> Kevin
>
Anthony Liguori - May 3, 2010, 5:01 p.m.
On 03/29/2010 06:03 AM, Kevin Wolf wrote:
> Am 28.03.2010 19:07, schrieb Ryota Ozaki:
>    
>> - 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
>> - add missing 'ret = 1' when unix_socket_outgoing failed
>>
>> Signed-off-by: Ryota Ozaki<ozaki.ryota@gmail.com>
>>      
> Acked-by: Kevin Wolf<kwolf@redhat.com>
>    

Are you going to pull this into the block branch?

Regards,

Anthony Liguori

>> @@ -343,25 +343,31 @@ int main(int argc, char **argv)
>>           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]);
>>       }
>>      
> If you do it like this, you could do the change for even more errors,
> like the ones returned by bdrv_read. But that doesn't make this patch
> less correct, of course.
>
> Kevin
>
>
>
>
Kevin Wolf - May 4, 2010, 7:29 a.m.
Am 03.05.2010 19:01, schrieb Anthony Liguori:
> On 03/29/2010 06:03 AM, Kevin Wolf wrote:
>> Am 28.03.2010 19:07, schrieb Ryota Ozaki:
>>    
>>> - 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
>>> - add missing 'ret = 1' when unix_socket_outgoing failed
>>>
>>> Signed-off-by: Ryota Ozaki<ozaki.ryota@gmail.com>
>>>      
>> Acked-by: Kevin Wolf<kwolf@redhat.com>
>>    
> 
> Are you going to pull this into the block branch?

There was a v2 of this patch yesterday, which I did apply to the block
branch.

Kevin

Patch

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7ef409f..d3e1814 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -324,7 +324,7 @@  int main(int argc, char **argv)
     if (disconnect) {
         fd = open(argv[optind], readonly ? O_RDONLY : O_RDWR);
         if (fd == -1) {
-            errx(EXIT_FAILURE, "Cannot open %s", argv[optind]);
+            err(EXIT_FAILURE, "Cannot open %s", argv[optind]);
         }
 
         nbd_disconnect(fd);
@@ -343,25 +343,31 @@  int main(int argc, char **argv)
         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, readonly ? R_OK : 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");
             }
         }
 
@@ -386,6 +392,7 @@  int main(int argc, char **argv)
                 sock = unix_socket_outgoing(socket);
                 if (sock == -1) {
                     if (errno != ENOENT && errno != ECONNREFUSED) {
+                        ret = 1;
                         goto out;
                     }
                     sleep(1);	/* wait children */