diff mbox

[v3,3/4] qmp: refactor duplicate code

Message ID 1493735386-39622-4-git-send-email-pradeep.jagadeesh@huawei.com
State New
Headers show

Commit Message

Pradeep Jagadeesh May 2, 2017, 2:29 p.m. UTC 
This patchset factor out the duplicate qmp throttle interface code
that was present in both block and fsdev device files.

Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
 blockdev.c                      | 53 +++----------------------------------
 fsdev/qemu-fsdev-throttle.c     | 51 +-----------------------------------
 fsdev/qemu-fsdev.c              |  1 -
 hmp.c                           | 34 ++++++++++++------------
 include/qemu/throttle-options.h |  5 ++++
 util/Makefile.objs              |  1 +
 util/throttle-options.c         | 58 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 85 insertions(+), 118 deletions(-)
 create mode 100644 util/throttle-options.c

Comments

Eric Blake May 2, 2017, 10:15 p.m. UTC | #1 
On 05/02/2017 09:29 AM, Pradeep Jagadeesh wrote:
> This patchset factor out the duplicate qmp throttle interface code
> that was present in both block and fsdev device files.

Instead of adding the duplicate code in patch 2 then cleaning it out
here, you should refactor this patch to be first (fix existing code in
blockdev.c to make use of the new interfaces), then add the new fsdev
code that uses this code right away.

> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---
>  blockdev.c                      | 53 +++----------------------------------
>  fsdev/qemu-fsdev-throttle.c     | 51 +-----------------------------------
>  fsdev/qemu-fsdev.c              |  1 -
>  hmp.c                           | 34 ++++++++++++------------
>  include/qemu/throttle-options.h |  5 ++++
>  util/Makefile.objs              |  1 +
>  util/throttle-options.c         | 58 +++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 85 insertions(+), 118 deletions(-)
>  create mode 100644 util/throttle-options.c
>
Greg Kurz May 3, 2017, 5:21 p.m. UTC | #2 
On Tue, 2 May 2017 17:15:02 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 05/02/2017 09:29 AM, Pradeep Jagadeesh wrote:
> > This patchset factor out the duplicate qmp throttle interface code
> > that was present in both block and fsdev device files.  
> 
> Instead of adding the duplicate code in patch 2 then cleaning it out
> here, you should refactor this patch to be first (fix existing code in
> blockdev.c to make use of the new interfaces), then add the new fsdev
> code that uses this code right away.
> 

Yeah I second that.

Pradeep,

When you worked on the command line version for 2.9, I didn't really know
what we would be able to share between fsdev and blockdev... I had hence
asked to code it separately and then factor out. The situation is different
here: the core throttling code is in place for both fsdev and blockdev. We
really want to share as much code as possible. So you probably should move
this patch and maybe the next one in front and then have fsdev to use the
new code as Eric is suggesting.

Cheers,

--
Greg

> > 
> > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > ---
> >  blockdev.c                      | 53 +++----------------------------------
> >  fsdev/qemu-fsdev-throttle.c     | 51 +-----------------------------------
> >  fsdev/qemu-fsdev.c              |  1 -
> >  hmp.c                           | 34 ++++++++++++------------
> >  include/qemu/throttle-options.h |  5 ++++
> >  util/Makefile.objs              |  1 +
> >  util/throttle-options.c         | 58 +++++++++++++++++++++++++++++++++++++++++
> >  7 files changed, 85 insertions(+), 118 deletions(-)
> >  create mode 100644 util/throttle-options.c
> >   
>
Pradeep Jagadeesh May 4, 2017, 9:44 a.m. UTC | #3 
On 5/3/2017 7:21 PM, Greg Kurz wrote:
> On Tue, 2 May 2017 17:15:02 -0500
> Eric Blake <eblake@redhat.com> wrote:
>
>> On 05/02/2017 09:29 AM, Pradeep Jagadeesh wrote:
>>> This patchset factor out the duplicate qmp throttle interface code
>>> that was present in both block and fsdev device files.
>>
>> Instead of adding the duplicate code in patch 2 then cleaning it out
>> here, you should refactor this patch to be first (fix existing code in
>> blockdev.c to make use of the new interfaces), then add the new fsdev
>> code that uses this code right away.
>>
>
> Yeah I second that.
>
> Pradeep,
>
> When you worked on the command line version for 2.9, I didn't really know
> what we would be able to share between fsdev and blockdev... I had hence
> asked to code it separately and then factor out. The situation is different
> here: the core throttling code is in place for both fsdev and blockdev. We
> really want to share as much code as possible. So you probably should move
> this patch and maybe the next one in front and then have fsdev to use the
> new code as Eric is suggesting.

OK, Sure. I will do.
>
> Cheers,
>
> --
> Greg
>
>>>
>>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>>> ---
>>>  blockdev.c                      | 53 +++----------------------------------
>>>  fsdev/qemu-fsdev-throttle.c     | 51 +-----------------------------------
>>>  fsdev/qemu-fsdev.c              |  1 -
>>>  hmp.c                           | 34 ++++++++++++------------
>>>  include/qemu/throttle-options.h |  5 ++++
>>>  util/Makefile.objs              |  1 +
>>>  util/throttle-options.c         | 58 +++++++++++++++++++++++++++++++++++++++++
>>>  7 files changed, 85 insertions(+), 118 deletions(-)
>>>  create mode 100644 util/throttle-options.c
>>>
>>
>
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 6428206..b056788 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2635,6 +2635,7 @@  void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     BlockDriverState *bs;
     BlockBackend *blk;
     AioContext *aio_context;
+    IOThrottle *iothrottle;
 
     blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
                       arg->has_id ? arg->id : NULL,
@@ -2652,56 +2653,8 @@  void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
         goto out;
     }
 
-    throttle_config_init(&cfg);
-    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
-    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
-    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
-
-    cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
-    cfg.buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
-    cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
-
-    if (arg->has_bps_max) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
-    }
-    if (arg->has_bps_rd_max) {
-        cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
-    }
-    if (arg->has_bps_wr_max) {
-        cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
-    }
-    if (arg->has_iops_max) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
-    }
-    if (arg->has_iops_rd_max) {
-        cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
-    }
-    if (arg->has_iops_wr_max) {
-        cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
-    }
-
-    if (arg->has_bps_max_length) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
-    }
-    if (arg->has_bps_rd_max_length) {
-        cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
-    }
-    if (arg->has_bps_wr_max_length) {
-        cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
-    }
-    if (arg->has_iops_max_length) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
-    }
-    if (arg->has_iops_rd_max_length) {
-        cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
-    }
-    if (arg->has_iops_wr_max_length) {
-        cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
-    }
-
-    if (arg->has_iops_size) {
-        cfg.op_size = arg->iops_size;
-    }
+    iothrottle = qapi_BlockIOThrottle_base(arg);
+    qmp_set_io_throttle(&cfg, iothrottle);
 
     if (!throttle_is_valid(&cfg, errp)) {
         goto out;
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 43dbcd8..cd653af 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -33,56 +33,7 @@  void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp)
 {
     ThrottleConfig cfg;
 
-    throttle_config_init(&cfg);
-    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
-    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
-    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
-
-    cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
-    cfg.buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
-    cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
-
-    if (arg->has_bps_max) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
-    }
-    if (arg->has_bps_rd_max) {
-        cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
-    }
-    if (arg->has_bps_wr_max) {
-        cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
-    }
-    if (arg->has_iops_max) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
-    }
-    if (arg->has_iops_rd_max) {
-        cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
-    }
-    if (arg->has_iops_wr_max) {
-        cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
-    }
-
-    if (arg->has_bps_max_length) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
-    }
-    if (arg->has_bps_rd_max_length) {
-        cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
-    }
-    if (arg->has_bps_wr_max_length) {
-        cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
-    }
-    if (arg->has_iops_max_length) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
-    }
-    if (arg->has_iops_rd_max_length) {
-        cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
-    }
-    if (arg->has_iops_wr_max_length) {
-        cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
-    }
-
-    if (arg->has_iops_size) {
-        cfg.op_size = arg->iops_size;
-    }
+    qmp_set_io_throttle(&cfg, arg);
 
     if (throttle_is_valid(&cfg, errp)) {
         fst->cfg = cfg;
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index a99e299..8c078b1 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -102,7 +102,6 @@  FsDriverEntry *get_fsdev_fsentry(char *id)
 
 void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp)
 {
-
     FsDriverEntry *fse;
 
     fse = get_fsdev_fsentry(arg->has_id ? arg->id : NULL);
diff --git a/hmp.c b/hmp.c
index 9c02e55..05c28b5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1553,20 +1553,29 @@  void hmp_change(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
+{
+    iot->has_id = true;
+    iot->id = (char *) qdict_get_str(qdict, "id");
+    iot->bps = qdict_get_int(qdict, "bps");
+    iot->bps_rd = qdict_get_int(qdict, "bps_rd");
+    iot->bps_wr = qdict_get_int(qdict, "bps_wr");
+    iot->iops = qdict_get_int(qdict, "iops");
+    iot->iops_rd = qdict_get_int(qdict, "iops_rd");
+    iot->iops_wr = qdict_get_int(qdict, "iops_wr");
+}
+
 void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
+    IOThrottle *iothrottle;
     BlockIOThrottle throttle = {
         .has_device = true,
         .device = (char *) qdict_get_str(qdict, "device"),
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
     };
 
+    iothrottle = qapi_BlockIOThrottle_base(&throttle);
+    hmp_initialize_io_throttle(iothrottle, qdict);
     qmp_block_set_io_throttle(&throttle, &err);
     hmp_handle_error(mon, &err);
 }
@@ -1576,17 +1585,8 @@  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
 void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    IOThrottle throttle = {
-        .has_id = true,
-        .id = (char *) qdict_get_str(qdict, "id"),
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
-    };
-
+    IOThrottle throttle;
+    hmp_initialize_io_throttle(&throttle, qdict);
     qmp_fsdev_set_io_throttle(&throttle, &err);
     hmp_handle_error(mon, &err);
 }
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 3133d1c..ca0fd7d 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -10,6 +10,9 @@ 
 #ifndef THROTTLE_OPTIONS_H
 #define THROTTLE_OPTIONS_H
 
+#include "qemu/throttle.h"
+#include "qmp-commands.h"
+
 #define THROTTLE_OPTS \
           { \
             .name = "throttling.iops-total",\
@@ -89,4 +92,6 @@ 
             .help = "when limiting by iops max size of an I/O in bytes",\
         }
 
+void qmp_set_io_throttle(ThrottleConfig *, IOThrottle *);
+
 #endif
diff --git a/util/Makefile.objs b/util/Makefile.objs
index c6205eb..7119ce0 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -43,3 +43,4 @@  util-obj-y += qdist.o
 util-obj-y += qht.o
 util-obj-y += range.o
 util-obj-y += systemd.o
+util-obj-y += throttle-options.o
diff --git a/util/throttle-options.c b/util/throttle-options.c
new file mode 100644
index 0000000..47cffed
--- /dev/null
+++ b/util/throttle-options.c
@@ -0,0 +1,58 @@ 
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/throttle-options.h"
+#include "qemu/iov.h"
+
+void qmp_set_io_throttle(ThrottleConfig *cfg, IOThrottle *arg)
+{
+    throttle_config_init(cfg);
+    cfg->buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
+    cfg->buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
+    cfg->buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
+
+    cfg->buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
+    cfg->buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
+    cfg->buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
+
+    if (arg->has_bps_max) {
+        cfg->buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
+    }
+    if (arg->has_bps_rd_max) {
+        cfg->buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
+    }
+    if (arg->has_bps_wr_max) {
+        cfg->buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
+    }
+    if (arg->has_iops_max) {
+        cfg->buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
+    }
+    if (arg->has_iops_rd_max) {
+        cfg->buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
+    }
+    if (arg->has_iops_wr_max) {
+        cfg->buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
+    }
+
+    if (arg->has_bps_max_length) {
+        cfg->buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
+    }
+    if (arg->has_bps_rd_max_length) {
+        cfg->buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
+    }
+    if (arg->has_bps_wr_max_length) {
+        cfg->buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
+    }
+    if (arg->has_iops_max_length) {
+        cfg->buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
+    }
+    if (arg->has_iops_rd_max_length) {
+        cfg->buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
+    }
+    if (arg->has_iops_wr_max_length) {
+        cfg->buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
+    }
+
+    if (arg->has_iops_size) {
+        cfg->op_size = arg->iops_size;
+    }
+}