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 arm...@redhat.com
 ---
  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 benoit.ca...@nodalink.com

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

2014-09-15 Thread Markus Armbruster 
Benoît Canet benoit.ca...@irqsave.net 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 arm...@redhat.com
 ---
  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 benoit.ca...@nodalink.com

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 arm...@redhat.com
---
  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 mre...@redhat.com

[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 arm...@redhat.com
---
 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