[Qemu-devel] [PATCH 1/2] qemu-nbd: Fix return value handling of bdrv_open

[Qemu-devel] [PATCH 1/2] qemu-nbd: Fix return value handling of bdrv_open

[Ryota Ozaki]

bdrv_open may return -errno so we have to check
if the return value is '< 0', not '== -1'.
---
 qemu-nbd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index a393583..b89c361 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -333,7 +333,7 @@ int main(int argc, char **argv)
     if (bs == NULL)
         return 1;
 
-    if (bdrv_open(bs, argv[optind], flags) == -1)
+    if (bdrv_open(bs, argv[optind], flags) < 0)
         return 1;
 
     fd_size = bs->total_sectors * 512;
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/2] qemu-nbd: Improve error reporting

[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
---
 qemu-nbd.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b89c361..5f10ff0 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(errno, "Cannot open %s", argv[optind]);
+            err(errno, "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(errno, "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(errno, "Could not find partition %d", partition);
+        err(errno, "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(errno, "Could not access '%s'", device);
+
         if (!verbose) {
             /* detach client and server */
             if (daemon(0, 0) == -1) {
-                errx(errno, "Failed to daemonize");
+                err(errno, "Failed to daemonize");
             }
         }
 
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH 2/2] qemu-nbd: Improve error reporting

[malc]

On Sat, 20 Mar 2010, 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

I'm under impression that you and the original author of this
code is misunderstading how err[x] works, first argument is
what will be used to exit the process and has little to do
with POSIX/C error codes.

> ---
>  qemu-nbd.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b89c361..5f10ff0 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(errno, "Cannot open %s", argv[optind]);
> +            err(errno, "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(errno, "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(errno, "Could not find partition %d", partition);
> +        err(errno, "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(errno, "Could not access '%s'", device);
> +
>          if (!verbose) {
>              /* detach client and server */
>              if (daemon(0, 0) == -1) {
> -                errx(errno, "Failed to daemonize");
> +                err(errno, "Failed to daemonize");
>              }
>          }
>  
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] qemu-nbd: Improve error reporting

[Ryota Ozaki]

On Sat, Mar 20, 2010 at 2:26 PM, malc <av1474@comtv.ru> wrote:
> On Sat, 20 Mar 2010, 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
>
> I'm under impression that you and the original author of this
> code is misunderstading how err[x] works, first argument is
> what will be used to exit the process and has little to do
> with POSIX/C error codes.

Oh, you're right. I'll revise and resend the patch.
(and I'll add missing Signed-off-by fields)

Thanks,
  ozaki-r

>
>> ---
>>  qemu-nbd.c |   16 +++++++++++-----
>>  1 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index b89c361..5f10ff0 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(errno, "Cannot open %s", argv[optind]);
>> +            err(errno, "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(errno, "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(errno, "Could not find partition %d", partition);
>> +        err(errno, "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(errno, "Could not access '%s'", device);
>> +
>>          if (!verbose) {
>>              /* detach client and server */
>>              if (daemon(0, 0) == -1) {
>> -                errx(errno, "Failed to daemonize");
>> +                err(errno, "Failed to daemonize");
>>              }
>>          }
>>
>>
>
> --
> mailto:av1474@comtv.ru
>