Merge remote-tracking branch 'qmp/queue/qmp' into staging

* qmp/queue/qmp:
  qapi: fix qmp_balloon() conversion
  qemu-iotests: add block-stream speed value test case
  block: add 'speed' optional parameter to block-stream
  block: change block-job-set-speed argument from 'value' to 'speed'
  block: use Error mechanism instead of -errno for block_job_set_speed()
  block: use Error mechanism instead of -errno for block_job_create()

balloon.c

@@ -108,7 +108,7 @@ void qmp_balloon(int64_t value, Error **errp)
     }
 
     if (value <= 0) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "target", "a size");
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
         return;
     }
     

block.c

@@ -4083,11 +4083,13 @@ out:
 }
 
 void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
-                       BlockDriverCompletionFunc *cb, void *opaque)
+                       int64_t speed, BlockDriverCompletionFunc *cb,
+                       void *opaque, Error **errp)
 {
     BlockJob *job;
 
     if (bs->job || bdrv_in_use(bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
     bdrv_set_in_use(bs, 1);
@@ -4098,6 +4100,20 @@ void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
     job->cb            = cb;
     job->opaque        = opaque;
     bs->job = job;
+
+    /* Only set speed when necessary to avoid NotSupported error */
+    if (speed != 0) {
+        Error *local_err = NULL;
+
+        block_job_set_speed(job, speed, &local_err);
+        if (error_is_set(&local_err)) {
+            bs->job = NULL;
+            g_free(job);
+            bdrv_set_in_use(bs, 0);
+            error_propagate(errp, local_err);
+            return NULL;
+        }
+    }
     return job;
 }
 
@@ -4112,18 +4128,21 @@ void block_job_complete(BlockJob *job, int ret)
     bdrv_set_in_use(bs, 0);
 }
 
-int block_job_set_speed(BlockJob *job, int64_t value)
+void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
-    int rc;
+    Error *local_err = NULL;
 
     if (!job->job_type->set_speed) {
-        return -ENOTSUP;
+        error_set(errp, QERR_NOT_SUPPORTED);
+        return;
     }
-    rc = job->job_type->set_speed(job, value);
+    job->job_type->set_speed(job, speed, &local_err);
-    if (rc == 0) {
+    if (error_is_set(&local_err)) {
-        job->speed = value;
+        error_propagate(errp, local_err);
+        return;
     }
-    return rc;
+
+    job->speed = speed;
 }
 
 void block_job_cancel(BlockJob *job)

block/stream.c

@@ -263,15 +263,15 @@ retry:
     block_job_complete(&s->common, ret);
 }
 
-static int stream_set_speed(BlockJob *job, int64_t value)
+static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common);
 
-    if (value < 0) {
+    if (speed < 0) {
-        return -EINVAL;
+        error_set(errp, QERR_INVALID_PARAMETER, "speed");
+        return;
     }
-    ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
+    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE);
-    return 0;
 }
 
 static BlockJobType stream_job_type = {
@@ -280,16 +280,17 @@ static BlockJobType stream_job_type = {
     .set_speed     = stream_set_speed,
 };
 
-int stream_start(BlockDriverState *bs, BlockDriverState *base,
+void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                 const char *base_id, BlockDriverCompletionFunc *cb,
+                  const char *base_id, int64_t speed,
-                 void *opaque)
+                  BlockDriverCompletionFunc *cb,
+                  void *opaque, Error **errp)
 {
     StreamBlockJob *s;
     Coroutine *co;
 
-    s = block_job_create(&stream_job_type, bs, cb, opaque);
+    s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp);
     if (!s) {
-        return -EBUSY; /* bs must already be in use */
+        return;
     }
 
     s->base = base;
@@ -300,5 +301,4 @@ int stream_start(BlockDriverState *bs, BlockDriverState *base,
     co = qemu_coroutine_create(stream_run);
     trace_stream_start(bs, base, s, co, opaque);
     qemu_coroutine_enter(co, s);
-    return 0;
 }

block_int.h

@@ -79,7 +79,7 @@ typedef struct BlockJobType {
     const char *job_type;
 
     /** Optional callback for job types that support setting a speed limit */
-    int (*set_speed)(BlockJob *job, int64_t value);
+    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
 } BlockJobType;
 
 /**
@@ -344,8 +344,10 @@ int is_windows_drive(const char *filename);
  * block_job_create:
  * @job_type: The class object for the newly-created job.
  * @bs: The block
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
+ * @errp: Error object.
  *
  * Create a new long-running block device job and return it.  The job
  * will call @cb asynchronously when the job completes.  Note that
@@ -357,7 +359,8 @@ int is_windows_drive(const char *filename);
  * called from a wrapper that is specific to the job type.
  */
 void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
-                       BlockDriverCompletionFunc *cb, void *opaque);
+                       int64_t speed, BlockDriverCompletionFunc *cb,
+                       void *opaque, Error **errp);
 
 /**
  * block_job_complete:
@@ -373,11 +376,12 @@ void block_job_complete(BlockJob *job, int ret);
  * block_job_set_speed:
  * @job: The job to set the speed for.
  * @speed: The new value
+ * @errp: Error object.
  *
  * Set a rate-limiting parameter for the job; the actual meaning may
  * vary depending on the job type.
  */
-int block_job_set_speed(BlockJob *job, int64_t value);
+void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
 
 /**
  * block_job_cancel:
@@ -415,8 +419,10 @@ void block_job_cancel_sync(BlockJob *job);
  * flatten the whole backing file chain onto @bs.
  * @base_id: The file name that will be written to @bs as the new
  * backing file if the job completes.  Ignored if @base is %NULL.
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
+ * @errp: Error object.
  *
  * Start a streaming operation on @bs.  Clusters that are unallocated
  * in @bs, but allocated in any image between @base and @bs (both
@@ -424,8 +430,9 @@ void block_job_cancel_sync(BlockJob *job);
  * streaming job, the backing file of @bs will be changed to
  * @base_id in the written image and to @base in the live BlockDriverState.
  */
-int stream_start(BlockDriverState *bs, BlockDriverState *base,
+void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                 const char *base_id, BlockDriverCompletionFunc *cb,
+                  const char *base_id, int64_t speed,
-                 void *opaque);
+                  BlockDriverCompletionFunc *cb,
+                  void *opaque, Error **errp);
 
 #endif /* BLOCK_INT_H */

blockdev.c

@@ -1091,11 +1091,12 @@ static void block_stream_cb(void *opaque, int ret)
 }
 
 void qmp_block_stream(const char *device, bool has_base,
-                      const char *base, Error **errp)
+                      const char *base, bool has_speed,
+                      int64_t speed, Error **errp)
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
-    int ret;
+    Error *local_err = NULL;
 
     bs = bdrv_find(device);
     if (!bs) {
@@ -1111,16 +1112,11 @@ void qmp_block_stream(const char *device, bool has_base,
         }
     }
 
-    ret = stream_start(bs, base_bs, base, block_stream_cb, bs);
+    stream_start(bs, base_bs, base, has_speed ? speed : 0,
-    if (ret < 0) {
+                 block_stream_cb, bs, &local_err);
-        switch (ret) {
+    if (error_is_set(&local_err)) {
-        case -EBUSY:
+        error_propagate(errp, local_err);
-            error_set(errp, QERR_DEVICE_IN_USE, device);
+        return;
-            return;
-        default:
-            error_set(errp, QERR_NOT_SUPPORTED);
-            return;
-        }
     }
 
     /* Grab a reference so hotplug does not delete the BlockDriverState from
@@ -1142,7 +1138,7 @@ static BlockJob *find_block_job(const char *device)
     return bs->job;
 }
 
-void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp)
+void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 {
     BlockJob *job = find_block_job(device);
 
@@ -1151,9 +1147,7 @@ void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp)
         return;
     }
 
-    if (block_job_set_speed(job, value) < 0) {
+    block_job_set_speed(job, speed, errp);
-        error_set(errp, QERR_NOT_SUPPORTED);
-    }
 }
 
 void qmp_block_job_cancel(const char *device, Error **errp)

hmp-commands.hx

@@ -71,8 +71,8 @@ ETEXI
 
     {
         .name       = "block_stream",
-        .args_type  = "device:B,base:s?",
+        .args_type  = "device:B,speed:o?,base:s?",
-        .params     = "device [base]",
+        .params     = "device [speed [base]]",
         .help       = "copy data from a backing file into a block device",
         .mhandler.cmd = hmp_block_stream,
     },
@@ -85,8 +85,8 @@ ETEXI
 
     {
         .name       = "block_job_set_speed",
-        .args_type  = "device:B,value:o",
+        .args_type  = "device:B,speed:o",
-        .params     = "device value",
+        .params     = "device speed",
         .help       = "set maximum speed for a background block operation",
         .mhandler.cmd = hmp_block_job_set_speed,
     },

hmp.c

@@ -835,8 +835,10 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     Error *error = NULL;
     const char *device = qdict_get_str(qdict, "device");
     const char *base = qdict_get_try_str(qdict, "base");
+    int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
-    qmp_block_stream(device, base != NULL, base, &error);
+    qmp_block_stream(device, base != NULL, base,
+                     qdict_haskey(qdict, "speed"), speed, &error);
 
     hmp_handle_error(mon, &error);
 }

qapi-schema.json

@@ -1571,15 +1571,19 @@
 #
 # @base:   #optional the common backing file name
 #
+# @speed:  #optional the maximum speed, in bytes per second
+#
 # Returns: Nothing on success
 #          If streaming is already active on this device, DeviceInUse
 #          If @device does not exist, DeviceNotFound
 #          If image streaming is not supported by this device, NotSupported
 #          If @base does not exist, BaseNotFound
+#          If @speed is invalid, InvalidParameter
 #
 # Since: 1.1
 ##
-{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str' } }
+{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str',
+                                       '*speed': 'int' } }
 
 ##
 # @block-job-set-speed:
@@ -1592,16 +1596,18 @@
 #
 # @device: the device name
 #
-# @value:  the maximum speed, in bytes per second
+# @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
+#          Defaults to 0.
 #
 # Returns: Nothing on success
 #          If the job type does not support throttling, NotSupported
+#          If the speed value is invalid, InvalidParameter
 #          If streaming is not active on this device, DeviceNotActive
 #
 # Since: 1.1
 ##
 { 'command': 'block-job-set-speed',
-  'data': { 'device': 'str', 'value': 'int' } }
+  'data': { 'device': 'str', 'speed': 'int' } }
 
 ##
 # @block-job-cancel:

qmp-commands.hx

@@ -688,13 +688,13 @@ EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?",
+        .args_type  = "device:B,base:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },
 
     {
         .name       = "block-job-set-speed",
-        .args_type  = "device:B,value:o",
+        .args_type  = "device:B,speed:o",
         .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
     },
 

tests/qemu-iotests/030

@@ -32,6 +32,21 @@ class ImageStreamingTestCase(iotests.QMPTestCase):
         result = self.vm.qmp('query-block-jobs')
         self.assert_qmp(result, 'return', [])
 
+    def cancel_and_wait(self, drive='drive0'):
+        '''Cancel a block job and wait for it to finish'''
+        result = self.vm.qmp('block-job-cancel', device=drive)
+        self.assert_qmp(result, 'return', {})
+
+        cancelled = False
+        while not cancelled:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_CANCELLED':
+                    self.assert_qmp(event, 'data/type', 'stream')
+                    self.assert_qmp(event, 'data/device', drive)
+                    cancelled = True
+
+        self.assert_no_active_streams()
+
 class TestSingleDrive(ImageStreamingTestCase):
     image_len = 1 * 1024 * 1024 # MB
 
@@ -97,21 +112,8 @@ class TestStreamStop(ImageStreamingTestCase):
         events = self.vm.get_qmp_events(wait=False)
         self.assertEqual(events, [], 'unexpected QMP event: %s' % events)
 
-        self.vm.qmp('block-job-cancel', device='drive0')
+        self.cancel_and_wait()
-        self.assert_qmp(result, 'return', {})
 
-        cancelled = False
-        while not cancelled:
-            for event in self.vm.get_qmp_events(wait=True):
-                if event['event'] == 'BLOCK_JOB_CANCELLED':
-                    self.assert_qmp(event, 'data/type', 'stream')
-                    self.assert_qmp(event, 'data/device', 'drive0')
-                    cancelled = True
-
-        self.assert_no_active_streams()
-
-# This is a short performance test which is not run by default.
-# Invoke "IMGFMT=qed ./030 TestSetSpeed.perf_test_set_speed"
 class TestSetSpeed(ImageStreamingTestCase):
     image_len = 80 * 1024 * 1024 # MB
 
@@ -126,13 +128,15 @@ class TestSetSpeed(ImageStreamingTestCase):
         os.remove(test_img)
         os.remove(backing_img)
 
-    def perf_test_set_speed(self):
+    # This is a short performance test which is not run by default.
+    # Invoke "IMGFMT=qed ./030 TestSetSpeed.perf_test_throughput"
+    def perf_test_throughput(self):
         self.assert_no_active_streams()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
 
-        result = self.vm.qmp('block-job-set-speed', device='drive0', value=8 * 1024 * 1024)
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
         self.assert_qmp(result, 'return', {})
 
         completed = False
@@ -147,5 +151,54 @@ class TestSetSpeed(ImageStreamingTestCase):
 
         self.assert_no_active_streams()
 
+    def test_set_speed(self):
+        self.assert_no_active_streams()
+
+        result = self.vm.qmp('block-stream', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        # Default speed is 0
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+        self.assert_qmp(result, 'return[0]/speed', 0)
+
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
+        self.assert_qmp(result, 'return', {})
+
+        # Ensure the speed we set was accepted
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+        self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024)
+
+        self.cancel_and_wait()
+
+        # Check setting speed in block-stream works
+        result = self.vm.qmp('block-stream', device='drive0', speed=4 * 1024 * 1024)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+        self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024)
+
+        self.cancel_and_wait()
+
+    def test_set_speed_invalid(self):
+        self.assert_no_active_streams()
+
+        result = self.vm.qmp('block-stream', device='drive0', speed=-1)
+        self.assert_qmp(result, 'error/class', 'InvalidParameter')
+        self.assert_qmp(result, 'error/data/name', 'speed')
+
+        self.assert_no_active_streams()
+
+        result = self.vm.qmp('block-stream', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
+        self.assert_qmp(result, 'error/class', 'InvalidParameter')
+        self.assert_qmp(result, 'error/data/name', 'speed')
+
+        self.cancel_and_wait()
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])

tests/qemu-iotests/030.out

@@ -1,5 +1,5 @@
-...
+.....
 ----------------------------------------------------------------------
-Ran 3 tests
+Ran 5 tests
 
 OK