Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

2014-09-15 Thread Markus Armbruster 
Benoît Canet  writes:

> The Friday 12 Sep 2014 à 21:26:21 (+0200), Markus Armbruster wrote :
>> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
>> Finish the BlockDriverState job before starting to mess with
>> DriveInfo.  Easier on the eyes.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  blockdev.c | 43 +++
>>  1 file changed, 23 insertions(+), 20 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..5ec4635 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>  int ro = 0;
>>  int bdrv_flags = 0;
>>  int on_read_error, on_write_error;
>> +BlockDriverState *bs;
>>  DriveInfo *dinfo;
>>  ThrottleConfig cfg;
>>  int snapshot = 0;
>> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, 
>> QDict *bs_opts,
>>  }
>>  
>>  /* init */
>> +bs = bdrv_new(qemu_opts_id(opts), errp);
>> +if (!bs) {
>> +goto early_err;
>> +}
>> +bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> +bs->read_only = ro;
>> +bs->detect_zeroes = detect_zeroes;
>> +
>> +bdrv_set_on_error(bs, on_read_error, on_write_error);
>> +
>> +/* disk I/O throttling */
>> +if (throttle_enabled(&cfg)) {
>> +bdrv_io_limits_enable(bs);
>> +bdrv_set_io_limits(bs, &cfg);
>> +}
>> +
>>  dinfo = g_malloc0(sizeof(*dinfo));
>>  dinfo->id = g_strdup(qemu_opts_id(opts));
>> -dinfo->bdrv = bdrv_new(dinfo->id, &error);
>> -if (error) {
>> -error_propagate(errp, error);
>> -goto bdrv_new_err;
>> -}
>> -dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> -dinfo->bdrv->read_only = ro;
>> -dinfo->bdrv->detect_zeroes = detect_zeroes;
>> +dinfo->bdrv = bs;
>>  QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>>  
>> -bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
>> -
>> -/* disk I/O throttling */
>> -if (throttle_enabled(&cfg)) {
>> -bdrv_io_limits_enable(dinfo->bdrv);
>> -bdrv_set_io_limits(dinfo->bdrv, &cfg);
>> -}
>> -
>>  if (!file || !*file) {
>>  if (has_driver_specific_opts) {
>>  file = NULL;
>> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>  bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>  
>>  QINCREF(bs_opts);
>> -ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
>> &error);
>> +ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +assert(bs == dinfo->bdrv);
>>  
>>  if (ret < 0) {
>>  error_setg(errp, "could not open disk image %s: %s",
>> @@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>  goto err;
>>  }
>>  
>> -if (bdrv_key_required(dinfo->bdrv))
>> +if (bdrv_key_required(bs)) {
>>  autostart = 0;
>> +}
>>  
>>  QDECREF(bs_opts);
>>  qemu_opts_del(opts);
>> @@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>  return dinfo;
>>  
>>  err:
>> -bdrv_unref(dinfo->bdrv);
>
>> +bdrv_unref(bs);
> I would have moved this.
>
>>  QTAILQ_REMOVE(&drives, dinfo, next);
>> -bdrv_new_err:
>>  g_free(dinfo->id);
>>  g_free(dinfo);
>
> To Here.
>
> No functional difference but it would reflect it's goto chain role:
> being in the reverse order of the allocations.

You're right.

In the BlockBackend series, this becomes just

err:
blk_unref(blk);
early_err:

so the disorder is just temporary.  I'll change it anyway if I have to
respin for some other reason.

>>  early_err:
>> -- 
>> 1.9.3
>
> Reviewed-by: Benoît Canet 

Thanks!

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

2014-09-15 Thread Benoît Canet 
The Friday 12 Sep 2014 à 21:26:21 (+0200), Markus Armbruster wrote :
> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
> Finish the BlockDriverState job before starting to mess with
> DriveInfo.  Easier on the eyes.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  blockdev.c | 43 +++
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b361fbb..5ec4635 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>  int ro = 0;
>  int bdrv_flags = 0;
>  int on_read_error, on_write_error;
> +BlockDriverState *bs;
>  DriveInfo *dinfo;
>  ThrottleConfig cfg;
>  int snapshot = 0;
> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>  }
>  
>  /* init */
> +bs = bdrv_new(qemu_opts_id(opts), errp);
> +if (!bs) {
> +goto early_err;
> +}
> +bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> +bs->read_only = ro;
> +bs->detect_zeroes = detect_zeroes;
> +
> +bdrv_set_on_error(bs, on_read_error, on_write_error);
> +
> +/* disk I/O throttling */
> +if (throttle_enabled(&cfg)) {
> +bdrv_io_limits_enable(bs);
> +bdrv_set_io_limits(bs, &cfg);
> +}
> +
>  dinfo = g_malloc0(sizeof(*dinfo));
>  dinfo->id = g_strdup(qemu_opts_id(opts));
> -dinfo->bdrv = bdrv_new(dinfo->id, &error);
> -if (error) {
> -error_propagate(errp, error);
> -goto bdrv_new_err;
> -}
> -dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> -dinfo->bdrv->read_only = ro;
> -dinfo->bdrv->detect_zeroes = detect_zeroes;
> +dinfo->bdrv = bs;
>  QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>  
> -bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
> -
> -/* disk I/O throttling */
> -if (throttle_enabled(&cfg)) {
> -bdrv_io_limits_enable(dinfo->bdrv);
> -bdrv_set_io_limits(dinfo->bdrv, &cfg);
> -}
> -
>  if (!file || !*file) {
>  if (has_driver_specific_opts) {
>  file = NULL;
> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>  bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>  QINCREF(bs_opts);
> -ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
> &error);
> +ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
> +assert(bs == dinfo->bdrv);
>  
>  if (ret < 0) {
>  error_setg(errp, "could not open disk image %s: %s",
> @@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>  goto err;
>  }
>  
> -if (bdrv_key_required(dinfo->bdrv))
> +if (bdrv_key_required(bs)) {
>  autostart = 0;
> +}
>  
>  QDECREF(bs_opts);
>  qemu_opts_del(opts);
> @@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>  return dinfo;
>  
>  err:
> -bdrv_unref(dinfo->bdrv);

> +bdrv_unref(bs);
I would have moved this.

>  QTAILQ_REMOVE(&drives, dinfo, next);
> -bdrv_new_err:
>  g_free(dinfo->id);
>  g_free(dinfo);

To Here.

No functional difference but it would reflect it's goto chain role:
being in the reverse order of the allocations.

>  early_err:
> -- 
> 1.9.3

Reviewed-by: Benoît Canet 
>

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

2014-09-14 Thread Markus Armbruster 
Max Reitz  writes:

> On 12.09.2014 21:26, Markus Armbruster wrote:
>> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
>> Finish the BlockDriverState job before starting to mess with
>> DriveInfo.  Easier on the eyes.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>   blockdev.c | 43 +++
>>   1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..5ec4635 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>   int ro = 0;
>>   int bdrv_flags = 0;
>>   int on_read_error, on_write_error;
>> +BlockDriverState *bs;
>>   DriveInfo *dinfo;
>>   ThrottleConfig cfg;
>>   int snapshot = 0;
>> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, 
>> QDict *bs_opts,
>>   }
>> /* init */
>> +bs = bdrv_new(qemu_opts_id(opts), errp);
>> +if (!bs) {
>> +goto early_err;
>> +}
>> +bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> +bs->read_only = ro;
>> +bs->detect_zeroes = detect_zeroes;
>> +
>> +bdrv_set_on_error(bs, on_read_error, on_write_error);
>> +
>> +/* disk I/O throttling */
>> +if (throttle_enabled(&cfg)) {
>> +bdrv_io_limits_enable(bs);
>> +bdrv_set_io_limits(bs, &cfg);
>> +}
>> +
>>   dinfo = g_malloc0(sizeof(*dinfo));
>
> Could've changed this to g_new0 in the process, but you're the expert
> for that, so I'll leave it up to you. ;-)

When I made block use g_new() & friends, I only converted patterns like
p = g_malloc(sizeof(T)), not patterns like p = g_malloc(sizeof(*p)).

In the former case, p = g_new(T) is a clear improvement, because now the
compiler checks T matches typeof(*p).

In the latter case, we trade some visible obviousness for type safety.
Matter of taste.

If we agree to prefer type safety in block land, I'll gladly do the
conversion work.

>>   dinfo->id = g_strdup(qemu_opts_id(opts));
>> -dinfo->bdrv = bdrv_new(dinfo->id, &error);
>> -if (error) {
>> -error_propagate(errp, error);
>> -goto bdrv_new_err;
>> -}
>> -dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> -dinfo->bdrv->read_only = ro;
>> -dinfo->bdrv->detect_zeroes = detect_zeroes;
>> +dinfo->bdrv = bs;
>>   QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>>   -bdrv_set_on_error(dinfo->bdrv, on_read_error,
>> on_write_error);
>> -
>> -/* disk I/O throttling */
>> -if (throttle_enabled(&cfg)) {
>> -bdrv_io_limits_enable(dinfo->bdrv);
>> -bdrv_set_io_limits(dinfo->bdrv, &cfg);
>> -}
>> -
>>   if (!file || !*file) {
>>   if (has_driver_specific_opts) {
>>   file = NULL;
>> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>   bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>> QINCREF(bs_opts);
>> -ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
>> &error);
>> +ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +assert(bs == dinfo->bdrv);
>
> Well, this is guaranteed by bdrv_open(), but of course better having
> too many assertions than too few.

Indeed.  bdrv_open() is in another file, and its function comment
doesn't quite spell out this part of its contract.

Assertions do double-duty: they check assumptions are valid, and they
remind the reader of assumptions.  The second part can be useful even
when the first part is trivial.

> With or without g_new0:
>
> Reviewed-by: Max Reitz 

Thanks!

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

2014-09-13 Thread Max Reitz 

On 12.09.2014 21:26, Markus Armbruster wrote:

blockdev_init() mixes up BlockDriverState and DriveInfo initialization
Finish the BlockDriverState job before starting to mess with
DriveInfo.  Easier on the eyes.

Signed-off-by: Markus Armbruster 
---
  blockdev.c | 43 +++
  1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b361fbb..5ec4635 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
  int ro = 0;
  int bdrv_flags = 0;
  int on_read_error, on_write_error;
+BlockDriverState *bs;
  DriveInfo *dinfo;
  ThrottleConfig cfg;
  int snapshot = 0;
@@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
  }
  
  /* init */

+bs = bdrv_new(qemu_opts_id(opts), errp);
+if (!bs) {
+goto early_err;
+}
+bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
+bs->read_only = ro;
+bs->detect_zeroes = detect_zeroes;
+
+bdrv_set_on_error(bs, on_read_error, on_write_error);
+
+/* disk I/O throttling */
+if (throttle_enabled(&cfg)) {
+bdrv_io_limits_enable(bs);
+bdrv_set_io_limits(bs, &cfg);
+}
+
  dinfo = g_malloc0(sizeof(*dinfo));


Could've changed this to g_new0 in the process, but you're the expert 
for that, so I'll leave it up to you. ;-)



  dinfo->id = g_strdup(qemu_opts_id(opts));
-dinfo->bdrv = bdrv_new(dinfo->id, &error);
-if (error) {
-error_propagate(errp, error);
-goto bdrv_new_err;
-}
-dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
-dinfo->bdrv->read_only = ro;
-dinfo->bdrv->detect_zeroes = detect_zeroes;
+dinfo->bdrv = bs;
  QTAILQ_INSERT_TAIL(&drives, dinfo, next);
  
-bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);

-
-/* disk I/O throttling */
-if (throttle_enabled(&cfg)) {
-bdrv_io_limits_enable(dinfo->bdrv);
-bdrv_set_io_limits(dinfo->bdrv, &cfg);
-}
-
  if (!file || !*file) {
  if (has_driver_specific_opts) {
  file = NULL;
@@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
  bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
  
  QINCREF(bs_opts);

-ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
&error);
+ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
+assert(bs == dinfo->bdrv);


Well, this is guaranteed by bdrv_open(), but of course better having too 
many assertions than too few.


With or without g_new0:

Reviewed-by: Max Reitz

[Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

2014-09-12 Thread Markus Armbruster 
blockdev_init() mixes up BlockDriverState and DriveInfo initialization
Finish the BlockDriverState job before starting to mess with
DriveInfo.  Easier on the eyes.

Signed-off-by: Markus Armbruster 
---
 blockdev.c | 43 +++
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b361fbb..5ec4635 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
 int ro = 0;
 int bdrv_flags = 0;
 int on_read_error, on_write_error;
+BlockDriverState *bs;
 DriveInfo *dinfo;
 ThrottleConfig cfg;
 int snapshot = 0;
@@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
 }
 
 /* init */
+bs = bdrv_new(qemu_opts_id(opts), errp);
+if (!bs) {
+goto early_err;
+}
+bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
+bs->read_only = ro;
+bs->detect_zeroes = detect_zeroes;
+
+bdrv_set_on_error(bs, on_read_error, on_write_error);
+
+/* disk I/O throttling */
+if (throttle_enabled(&cfg)) {
+bdrv_io_limits_enable(bs);
+bdrv_set_io_limits(bs, &cfg);
+}
+
 dinfo = g_malloc0(sizeof(*dinfo));
 dinfo->id = g_strdup(qemu_opts_id(opts));
-dinfo->bdrv = bdrv_new(dinfo->id, &error);
-if (error) {
-error_propagate(errp, error);
-goto bdrv_new_err;
-}
-dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
-dinfo->bdrv->read_only = ro;
-dinfo->bdrv->detect_zeroes = detect_zeroes;
+dinfo->bdrv = bs;
 QTAILQ_INSERT_TAIL(&drives, dinfo, next);
 
-bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
-
-/* disk I/O throttling */
-if (throttle_enabled(&cfg)) {
-bdrv_io_limits_enable(dinfo->bdrv);
-bdrv_set_io_limits(dinfo->bdrv, &cfg);
-}
-
 if (!file || !*file) {
 if (has_driver_specific_opts) {
 file = NULL;
@@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
 bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
 QINCREF(bs_opts);
-ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
&error);
+ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
+assert(bs == dinfo->bdrv);
 
 if (ret < 0) {
 error_setg(errp, "could not open disk image %s: %s",
@@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
 goto err;
 }
 
-if (bdrv_key_required(dinfo->bdrv))
+if (bdrv_key_required(bs)) {
 autostart = 0;
+}
 
 QDECREF(bs_opts);
 qemu_opts_del(opts);
@@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
 return dinfo;
 
 err:
-bdrv_unref(dinfo->bdrv);
+bdrv_unref(bs);
 QTAILQ_REMOVE(&drives, dinfo, next);
-bdrv_new_err:
 g_free(dinfo->id);
 g_free(dinfo);
 early_err:
-- 
1.9.3