[Qemu-devel] [PATCH 1/3] qemu-nbd: Fix coding style

[Qemu-devel] [PATCH 1/3] qemu-nbd: Fix coding style

[Ryota Ozaki]

Follow "Every indented statement is braced; even if the block
contains just one statement." described in CODING_STYLE.

Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
---
 qemu-nbd.c |   60 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6d854d3..00b8896 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -113,8 +113,9 @@ static int find_partition(BlockDriverState *bs, int partition,
     int i;
     int ext_partnum = 4;
 
-    if (bdrv_read(bs, 0, data, 1))
+    if (bdrv_read(bs, 0, data, 1)) {
         errx(EXIT_FAILURE, "error while reading");
+    }
 
     if (data[510] != 0x55 || data[511] != 0xaa) {
         errno = -EINVAL;
@@ -124,21 +125,24 @@ static int find_partition(BlockDriverState *bs, int partition,
     for (i = 0; i < 4; i++) {
         read_partition(&data[446 + 16 * i], &mbr[i]);
 
-        if (!mbr[i].nb_sectors_abs)
+        if (!mbr[i].nb_sectors_abs) {
             continue;
+        }
 
         if (mbr[i].system == 0xF || mbr[i].system == 0x5) {
             struct partition_record ext[4];
             uint8_t data1[512];
             int j;
 
-            if (bdrv_read(bs, mbr[i].start_sector_abs, data1, 1))
+            if (bdrv_read(bs, mbr[i].start_sector_abs, data1, 1)) {
                 errx(EXIT_FAILURE, "error while reading");
+            }
 
             for (j = 0; j < 4; j++) {
                 read_partition(&data1[446 + 16 * j], &ext[j]);
-                if (!ext[j].nb_sectors_abs)
+                if (!ext[j].nb_sectors_abs) {
                     continue;
+                }
 
                 if ((ext_partnum + j + 1) == partition) {
                     *offset = (uint64_t)ext[j].start_sector_abs << 9;
@@ -169,8 +173,9 @@ static void show_parts(const char *device)
          *     modprobe nbd max_part=63
          */
         nbd = open(device, O_RDWR);
-        if (nbd != -1)
+        if (nbd != -1) {
               close(nbd);
+        }
         exit(0);
     }
 }
@@ -262,15 +267,18 @@ int main(int argc, char **argv)
             break;
         case 'P':
             partition = strtol(optarg, &end, 0);
-            if (*end)
+            if (*end) {
                 errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
-            if (partition < 1 || partition > 8)
+            }
+            if (partition < 1 || partition > 8) {
                 errx(EXIT_FAILURE, "Invalid partition %d", partition);
+            }
             break;
         case 'k':
             socket = optarg;
-            if (socket[0] != '/')
+            if (socket[0] != '/') {
                 errx(EXIT_FAILURE, "socket path must be absolute\n");
+            }
             break;
         case 'd':
             disconnect = true;
@@ -315,8 +323,9 @@ int main(int argc, char **argv)
 
     if (disconnect) {
         fd = open(argv[optind], O_RDWR);
-        if (fd == -1)
+        if (fd == -1) {
             errx(EXIT_FAILURE, "Cannot open %s", argv[optind]);
+        }
 
         nbd_disconnect(fd);
 
@@ -330,17 +339,20 @@ int main(int argc, char **argv)
     bdrv_init();
 
     bs = bdrv_new("hda");
-    if (bs == NULL)
+    if (bs == NULL) {
         return 1;
+    }
 
-    if (bdrv_open(bs, argv[optind], flags) < 0)
+    if (bdrv_open(bs, argv[optind], flags) < 0) {
         return 1;
+    }
 
     fd_size = bs->total_sectors * 512;
 
     if (partition != -1 &&
-        find_partition(bs, partition, &dev_offset, &fd_size))
+        find_partition(bs, partition, &dev_offset, &fd_size)) {
         errx(EXIT_FAILURE, "Could not find partition %d", partition);
+    }
 
     if (device) {
         pid_t pid;
@@ -360,8 +372,9 @@ int main(int argc, char **argv)
         }
 
         pid = fork();
-        if (pid < 0)
+        if (pid < 0) {
             return 1;
+        }
         if (pid != 0) {
             off_t size;
             size_t blocksize;
@@ -372,8 +385,9 @@ int main(int argc, char **argv)
             do {
                 sock = unix_socket_outgoing(socket);
                 if (sock == -1) {
-                    if (errno != ENOENT && errno != ECONNREFUSED)
+                    if (errno != ENOENT && errno != ECONNREFUSED) {
                         goto out;
+                    }
                     sleep(1);	/* wait children */
                 }
             } while (sock == -1);
@@ -422,14 +436,16 @@ int main(int argc, char **argv)
         sharing_fds[0] = tcp_socket_incoming(bindto, port);
     }
 
-    if (sharing_fds[0] == -1)
+    if (sharing_fds[0] == -1) {
         return 1;
+    }
     max_fd = sharing_fds[0];
     nb_fds++;
 
     data = qemu_memalign(512, NBD_BUFFER_SIZE);
-    if (data == NULL)
+    if (data == NULL) {
         errx(EXIT_FAILURE, "Cannot allocate data buffer");
+    }
 
     do {
 
@@ -438,11 +454,13 @@ int main(int argc, char **argv)
             FD_SET(sharing_fds[i], &fds);
 
         ret = select(max_fd + 1, &fds, NULL, NULL, NULL);
-        if (ret == -1)
+        if (ret == -1) {
             break;
+        }
 
-        if (FD_ISSET(sharing_fds[0], &fds))
+        if (FD_ISSET(sharing_fds[0], &fds)) {
             ret--;
+        }
         for (i = 1; i < nb_fds && ret; i++) {
             if (FD_ISSET(sharing_fds[i], &fds)) {
                 if (nbd_trip(bs, sharing_fds[i], fd_size, dev_offset,
@@ -463,8 +481,9 @@ int main(int argc, char **argv)
                                              &addr_len);
                 if (sharing_fds[nb_fds] != -1 &&
                     nbd_negotiate(sharing_fds[nb_fds], fd_size) != -1) {
-                        if (sharing_fds[nb_fds] > max_fd)
+                        if (sharing_fds[nb_fds] > max_fd) {
                             max_fd = sharing_fds[nb_fds];
+                        }
                         nb_fds++;
                 }
             }
@@ -475,8 +494,9 @@ int main(int argc, char **argv)
     close(sharing_fds[0]);
     bdrv_close(bs);
     qemu_free(sharing_fds);
-    if (socket)
+    if (socket) {
         unlink(socket);
+    }
 
     return 0;
 }
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/3] qemu-nbd: Extend read-only option to nbd device file

[Ryota Ozaki]

This patch allows to operate on nbd device file
without write permission for the file if read-only
option is specified.

Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
---
 qemu-nbd.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 00b8896..7ef409f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -162,7 +162,7 @@ static int find_partition(BlockDriverState *bs, int partition,
     return -1;
 }
 
-static void show_parts(const char *device)
+static void show_parts(const char *device, bool readonly)
 {
     if (fork() == 0) {
         int nbd;
@@ -172,7 +172,7 @@ static void show_parts(const char *device)
          * but remember to load the module with max_part != 0 :
          *     modprobe nbd max_part=63
          */
-        nbd = open(device, O_RDWR);
+        nbd = open(device, readonly ? O_RDONLY : O_RDWR);
         if (nbd != -1) {
               close(nbd);
         }
@@ -322,7 +322,7 @@ int main(int argc, char **argv)
     }
 
     if (disconnect) {
-        fd = open(argv[optind], O_RDWR);
+        fd = open(argv[optind], readonly ? O_RDONLY : O_RDWR);
         if (fd == -1) {
             errx(EXIT_FAILURE, "Cannot open %s", argv[optind]);
         }
@@ -392,7 +392,7 @@ int main(int argc, char **argv)
                 }
             } while (sock == -1);
 
-            fd = open(device, O_RDWR);
+            fd = open(device, readonly ? O_RDONLY : O_RDWR);
             if (fd == -1) {
                 ret = 1;
                 goto out;
@@ -415,7 +415,7 @@ int main(int argc, char **argv)
 
 	    /* update partition table */
 
-            show_parts(device);
+            show_parts(device, readonly);
 
             nbd_client(fd, sock);
             close(fd);
-- 
1.6.5.2

[Qemu-devel] [PATCH 3/3] 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
- 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(-)

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 */
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH 1/3] qemu-nbd: Fix coding style

[Kevin Wolf]

Am 28.03.2010 19:07, schrieb Ryota Ozaki:
> Follow "Every indented statement is braced; even if the block
> contains just one statement." described in CODING_STYLE.
> 
> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>

Usually we fix the coding style in places that need to be changed
anyway, but avoid touching things only for style cleanup (it makes git
blame less meaningful, for example).

Kevin

Re: [Qemu-devel] [PATCH 2/3] qemu-nbd: Extend read-only option to nbd device file

[Kevin Wolf]

Am 28.03.2010 19:07, schrieb Ryota Ozaki:
> This patch allows to operate on nbd device file
> without write permission for the file if read-only
> option is specified.
> 
> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>

The help for -r should be changed, too. Currently it says:

-r, --read-only      export read-only

> ---
>  qemu-nbd.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 00b8896..7ef409f 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -162,7 +162,7 @@ static int find_partition(BlockDriverState *bs, int partition,
>      return -1;
>  }
>  
> -static void show_parts(const char *device)
> +static void show_parts(const char *device, bool readonly)
>  {
>      if (fork() == 0) {
>          int nbd;
> @@ -172,7 +172,7 @@ static void show_parts(const char *device)
>           * but remember to load the module with max_part != 0 :
>           *     modprobe nbd max_part=63
>           */
> -        nbd = open(device, O_RDWR);
> +        nbd = open(device, readonly ? O_RDONLY : O_RDWR);
>          if (nbd != -1) {
>                close(nbd);
>          }

Can't we always use O_RDONLY here? Assuming that this is enough to
trigger a partition table update, I haven't tested it. But if it's not
enough, wouldn't be enough for readonly either.

Kevin

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

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 1/3] qemu-nbd: Fix coding style

[Ryota Ozaki]

Hi Kevin

On Mon, Mar 29, 2010 at 7:44 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 28.03.2010 19:07, schrieb Ryota Ozaki:
>> Follow "Every indented statement is braced; even if the block
>> contains just one statement." described in CODING_STYLE.
>>
>> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
>
> Usually we fix the coding style in places that need to be changed
> anyway, but avoid touching things only for style cleanup (it makes git
> blame less meaningful, for example).

Thank you for reviewing and I'm sorry I've stuck for long time...

I follow the policy and will drop the patch.

Thanks,
  ozaki-r


>
> Kevin
>

Re: [Qemu-devel] [PATCH 2/3] qemu-nbd: Extend read-only option to nbd device file

[Ryota Ozaki]

On Mon, Mar 29, 2010 at 7:54 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 28.03.2010 19:07, schrieb Ryota Ozaki:
>> This patch allows to operate on nbd device file
>> without write permission for the file if read-only
>> option is specified.
>>
>> Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
>
> The help for -r should be changed, too. Currently it says:
>
> -r, --read-only      export read-only

Indeed.

>
>> ---
>>  qemu-nbd.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 00b8896..7ef409f 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -162,7 +162,7 @@ static int find_partition(BlockDriverState *bs, int partition,
>>      return -1;
>>  }
>>
>> -static void show_parts(const char *device)
>> +static void show_parts(const char *device, bool readonly)
>>  {
>>      if (fork() == 0) {
>>          int nbd;
>> @@ -172,7 +172,7 @@ static void show_parts(const char *device)
>>           * but remember to load the module with max_part != 0 :
>>           *     modprobe nbd max_part=63
>>           */
>> -        nbd = open(device, O_RDWR);
>> +        nbd = open(device, readonly ? O_RDONLY : O_RDWR);
>>          if (nbd != -1) {
>>                close(nbd);
>>          }
>
> Can't we always use O_RDONLY here? Assuming that this is enough to
> trigger a partition table update, I haven't tested it. But if it's not
> enough, wouldn't be enough for readonly either.

You're right. It works always with O_RDONLY. So we can remove the
condition expression.

However, I will drop this patch because I found I misunderstood about
nbd. The patch intended to be for non-root users, but it does not make
sense because nbd requires CAP_SYS_ADMIN.

So I hold it until I understand well and focus on the last patch at first.
I'm sorry for messing up.

Thanks,
  ozaki-r

>
> Kevin
>

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

[Ryota Ozaki]

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
>

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

[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?

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
>
>
>
>    

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

[Kevin Wolf]

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