Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-28 Thread John Snow


On 02/28/2018 01:25 PM, Kevin Wolf wrote:
> Am 24.02.2018 um 00:51 hat John Snow geschrieben:
>> Expose the "manual" property via QAPI for the backup-related jobs.
>> As of this commit, this allows the management API to request the
>> "concluded" and "dismiss" semantics for backup jobs.
>>
>> Signed-off-by: John Snow 
>> ---
>>  blockdev.c   | 19 ---
>>  qapi/block-core.json | 32 ++--
>>  2 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 05fd421cdc..2eddb0e726 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3260,7 +3260,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
>> BlockJobTxn *txn,
>>  AioContext *aio_context;
>>  QDict *options = NULL;
>>  Error *local_err = NULL;
>> -int flags;
>> +int flags, job_flags = BLOCK_JOB_DEFAULT;
>>  int64_t size;
>>  bool set_backing_hd = false;
>>  
>> @@ -3279,6 +3279,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
>> BlockJobTxn *txn,
>>  if (!backup->has_job_id) {
>>  backup->job_id = NULL;
>>  }
>> +if (!backup->has_manual) {
>> +backup->manual = false;
>> +}
> 
> I think this is unnecessary these days, NULL/0/false is the default
> value for QMP/QAPI.
> 

That's what I get for cargo cult. Eric, confirm/deny? Should I remove
the other auto-false defaults too?

Either we have them all or none of them for consistency.

--js

>>  if (!backup->has_compress) {
>>  backup->compress = false;
>>  }
>> @@ -3422,6 +3429,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
>> BlockJobTxn *txn,
>>  if (!backup->has_job_id) {
>>  backup->job_id = NULL;
>>  }
>> +if (!backup->has_manual) {
>> +backup->manual = false;
>> +}
> 
> Same here.
> 
>>  if (!backup->has_compress) {
>>  backup->compress = false;
>>  }
> 
> Kevin
> 



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-28 Thread John Snow


On 02/28/2018 01:23 PM, Kevin Wolf wrote:
> But given how often I already said that and people still don't consider
> it as an option, this doesn't appear to have been very convincing. So
> whatever... *shrug*
> 

Sorry, I didn't mean to give that impression.

I initially *did* use two (or more?) variables to describe different
points, and I was convinced to collapse it down into one boolean that
meant "new or old" syntax.

I was just hesitant to run out and change it back until further comments
on what the series looked like at the moment.

--js



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-28 Thread Kevin Wolf
Am 27.02.2018 um 22:57 hat John Snow geschrieben:
> 
> 
> On 02/27/2018 03:16 PM, Eric Blake wrote:
> > On 02/23/2018 05:51 PM, John Snow wrote:
> >> Expose the "manual" property via QAPI for the backup-related jobs.
> >> As of this commit, this allows the management API to request the
> >> "concluded" and "dismiss" semantics for backup jobs.
> >>
> >> Signed-off-by: John Snow 
> >> ---
> >>   blockdev.c   | 19 ---
> >>   qapi/block-core.json | 32 ++--
> >>   2 files changed, 42 insertions(+), 9 deletions(-)
> >>
> > 
> >> +++ b/qapi/block-core.json
> >> @@ -1177,6 +1177,16 @@
> >>   # @job-id: identifier for the newly-created block job. If
> >>   #  omitted, the device name will be used. (Since 2.7)
> >>   #
> >> +# @manual: True to use an expanded, more explicit job control flow.
> >> +#  Jobs may transition from a running state to a pending state,
> >> +#  where they must be instructed to complete manually via
> >> +#  block-job-finalize.
> >> +#  Jobs belonging to a transaction must either all or all not
> >> use this
> >> +#  setting. Once a transaction reaches a pending state,
> >> issuing the
> >> +#  finalize command to any one job in the transaction is
> >> sufficient
> >> +#  to finalize the entire transaction.
> > 
> > The previous commit message talked about mixed-manual transactions, but
> > this seems to imply it is not possible.  I'm fine if we don't support
> > mixed-manual transactions, but wonder if it means any changes to the
> > series.
> > 
> > Otherwise looks reasonable from the UI point of view.
> > 
> 
> More seriously, this documentation I wrote doesn't address the totality
> of the expanded flow. I omitted dismiss here by accident as well. This
> is at best a partial definition of the 'manual' property.
> 
> I'd like to use _this_ patch to ask the question: "What should the
> proper noun for the QEMU 2.12+ Expanded Block Job Management Flow
> Mechanism be?"
> 
> I'm not too sure, but "Manual mode" leaves a lot to be desired.

I still think that the best way to expose it is like this:

# both default to true
'*auto-finalize': 'bool',
'*auto-dismiss': 'bool',

These options would be very easy to describe because they just invoke
the functionality of a specific QMP command automatically as soon as it
becomes available for the job.

But given how often I already said that and people still don't consider
it as an option, this doesn't appear to have been very convincing. So
whatever... *shrug*

Kevin



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-28 Thread Kevin Wolf
Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> Expose the "manual" property via QAPI for the backup-related jobs.
> As of this commit, this allows the management API to request the
> "concluded" and "dismiss" semantics for backup jobs.
> 
> Signed-off-by: John Snow 
> ---
>  blockdev.c   | 19 ---
>  qapi/block-core.json | 32 ++--
>  2 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 05fd421cdc..2eddb0e726 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3260,7 +3260,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
> BlockJobTxn *txn,
>  AioContext *aio_context;
>  QDict *options = NULL;
>  Error *local_err = NULL;
> -int flags;
> +int flags, job_flags = BLOCK_JOB_DEFAULT;
>  int64_t size;
>  bool set_backing_hd = false;
>  
> @@ -3279,6 +3279,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
> BlockJobTxn *txn,
>  if (!backup->has_job_id) {
>  backup->job_id = NULL;
>  }
> +if (!backup->has_manual) {
> +backup->manual = false;
> +}

I think this is unnecessary these days, NULL/0/false is the default
value for QMP/QAPI.

>  if (!backup->has_compress) {
>  backup->compress = false;
>  }
> @@ -3422,6 +3429,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
> BlockJobTxn *txn,
>  if (!backup->has_job_id) {
>  backup->job_id = NULL;
>  }
> +if (!backup->has_manual) {
> +backup->manual = false;
> +}

Same here.

>  if (!backup->has_compress) {
>  backup->compress = false;
>  }

Kevin



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-27 Thread Eric Blake

On 02/27/2018 03:57 PM, John Snow wrote:



On 02/27/2018 03:16 PM, Eric Blake wrote:

On 02/23/2018 05:51 PM, John Snow wrote:

Expose the "manual" property via QAPI for the backup-related jobs.
As of this commit, this allows the management API to request the
"concluded" and "dismiss" semantics for backup jobs.



+# @manual: True to use an expanded, more explicit job control flow.
+#  Jobs may transition from a running state to a pending state,
+#  where they must be instructed to complete manually via
+#  block-job-finalize.
+#  Jobs belonging to a transaction must either all or all not
use this
+#  setting. Once a transaction reaches a pending state,
issuing the
+#  finalize command to any one job in the transaction is
sufficient
+#  to finalize the entire transaction.


The previous commit message talked about mixed-manual transactions, but
this seems to imply it is not possible.  I'm fine if we don't support
mixed-manual transactions, but wonder if it means any changes to the
series.

Otherwise looks reasonable from the UI point of view.



More seriously, this documentation I wrote doesn't address the totality
of the expanded flow. I omitted dismiss here by accident as well. This
is at best a partial definition of the 'manual' property.

I'd like to use _this_ patch to ask the question: "What should the
proper noun for the QEMU 2.12+ Expanded Block Job Management Flow
Mechanism be?"


"Manual" actually doesn't sound too bad; I could also see "Explicit job 
flow", as in, "within a transaction, all jobs should have the same 
setting for the choice of Explicit Job Flow" (but then the name 'manual' 
would have to be changed to match).  The idea of a central document, 
that gets referred to from multiple spots in the QAPI docs, rather than 
duplicating information throughout the QAPI docs, is reasonable.




I'm not too sure, but "Manual mode" leaves a lot to be desired.

I keep calling it something like "2.12+ Job Management" but that's not
really descriptive.


That, and if someone ever backports the enhanced state machine to a 2.11 
branch, it becomes a misnomer.



I conceptualize the feature as the addition of a
purposefully more "needy" and less automatic completion mechanism, hence
the "manual"

Anyway, I'd like to figure out a good "documentation name" for it so I
can point all instances of the creation property (for drive-backup,
drive-mirror, and everyone else) to a central location that explains the
STM and what exactly the differences between manual=on/off are. I'd then
like to expose this property via query and link the documentation there
to this description, too.


"Explicit" and "Manual" are the two best options coming to me as I type 
this email.




It'd be nice-- under the same arguments that prompted 'dismiss'-- to say
that if a client crashes it can reconnect and discover what kind of
attention certain jobs will need by asking for the manual property back.

--js



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-27 Thread John Snow


On 02/27/2018 03:16 PM, Eric Blake wrote:
> On 02/23/2018 05:51 PM, John Snow wrote:
>> Expose the "manual" property via QAPI for the backup-related jobs.
>> As of this commit, this allows the management API to request the
>> "concluded" and "dismiss" semantics for backup jobs.
>>
>> Signed-off-by: John Snow 
>> ---
>>   blockdev.c   | 19 ---
>>   qapi/block-core.json | 32 ++--
>>   2 files changed, 42 insertions(+), 9 deletions(-)
>>
> 
>> +++ b/qapi/block-core.json
>> @@ -1177,6 +1177,16 @@
>>   # @job-id: identifier for the newly-created block job. If
>>   #  omitted, the device name will be used. (Since 2.7)
>>   #
>> +# @manual: True to use an expanded, more explicit job control flow.
>> +#  Jobs may transition from a running state to a pending state,
>> +#  where they must be instructed to complete manually via
>> +#  block-job-finalize.
>> +#  Jobs belonging to a transaction must either all or all not
>> use this
>> +#  setting. Once a transaction reaches a pending state,
>> issuing the
>> +#  finalize command to any one job in the transaction is
>> sufficient
>> +#  to finalize the entire transaction.
> 
> The previous commit message talked about mixed-manual transactions, but
> this seems to imply it is not possible.  I'm fine if we don't support
> mixed-manual transactions, but wonder if it means any changes to the
> series.
> 
> Otherwise looks reasonable from the UI point of view.
> 

More seriously, this documentation I wrote doesn't address the totality
of the expanded flow. I omitted dismiss here by accident as well. This
is at best a partial definition of the 'manual' property.

I'd like to use _this_ patch to ask the question: "What should the
proper noun for the QEMU 2.12+ Expanded Block Job Management Flow
Mechanism be?"

I'm not too sure, but "Manual mode" leaves a lot to be desired.

I keep calling it something like "2.12+ Job Management" but that's not
really descriptive. I conceptualize the feature as the addition of a
purposefully more "needy" and less automatic completion mechanism, hence
the "manual"

Anyway, I'd like to figure out a good "documentation name" for it so I
can point all instances of the creation property (for drive-backup,
drive-mirror, and everyone else) to a central location that explains the
STM and what exactly the differences between manual=on/off are. I'd then
like to expose this property via query and link the documentation there
to this description, too.

It'd be nice-- under the same arguments that prompted 'dismiss'-- to say
that if a client crashes it can reconnect and discover what kind of
attention certain jobs will need by asking for the manual property back.

--js



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-27 Thread John Snow


On 02/27/2018 03:16 PM, Eric Blake wrote:
> On 02/23/2018 05:51 PM, John Snow wrote:
>> Expose the "manual" property via QAPI for the backup-related jobs.
>> As of this commit, this allows the management API to request the
>> "concluded" and "dismiss" semantics for backup jobs.
>>
>> Signed-off-by: John Snow 
>> ---
>>   blockdev.c   | 19 ---
>>   qapi/block-core.json | 32 ++--
>>   2 files changed, 42 insertions(+), 9 deletions(-)
>>
> 
>> +++ b/qapi/block-core.json
>> @@ -1177,6 +1177,16 @@
>>   # @job-id: identifier for the newly-created block job. If
>>   #  omitted, the device name will be used. (Since 2.7)
>>   #
>> +# @manual: True to use an expanded, more explicit job control flow.
>> +#  Jobs may transition from a running state to a pending state,
>> +#  where they must be instructed to complete manually via
>> +#  block-job-finalize.
>> +#  Jobs belonging to a transaction must either all or all not
>> use this
>> +#  setting. Once a transaction reaches a pending state,
>> issuing the
>> +#  finalize command to any one job in the transaction is
>> sufficient
>> +#  to finalize the entire transaction.
> 
> The previous commit message talked about mixed-manual transactions, but
> this seems to imply it is not possible.  I'm fine if we don't support
> mixed-manual transactions, but wonder if it means any changes to the
> series.
> 
> Otherwise looks reasonable from the UI point of view.
> 

Refactor hell.

The intent (and my belief) is that as of right now you CAN mix them. In
earlier drafts, it was not always the case.



Re: [Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-27 Thread Eric Blake

On 02/23/2018 05:51 PM, John Snow wrote:

Expose the "manual" property via QAPI for the backup-related jobs.
As of this commit, this allows the management API to request the
"concluded" and "dismiss" semantics for backup jobs.

Signed-off-by: John Snow 
---
  blockdev.c   | 19 ---
  qapi/block-core.json | 32 ++--
  2 files changed, 42 insertions(+), 9 deletions(-)




+++ b/qapi/block-core.json
@@ -1177,6 +1177,16 @@
  # @job-id: identifier for the newly-created block job. If
  #  omitted, the device name will be used. (Since 2.7)
  #
+# @manual: True to use an expanded, more explicit job control flow.
+#  Jobs may transition from a running state to a pending state,
+#  where they must be instructed to complete manually via
+#  block-job-finalize.
+#  Jobs belonging to a transaction must either all or all not use this
+#  setting. Once a transaction reaches a pending state, issuing the
+#  finalize command to any one job in the transaction is sufficient
+#  to finalize the entire transaction.


The previous commit message talked about mixed-manual transactions, but 
this seems to imply it is not possible.  I'm fine if we don't support 
mixed-manual transactions, but wonder if it means any changes to the series.


Otherwise looks reasonable from the UI point of view.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



[Qemu-devel] [RFC v4 19/21] blockjobs: Expose manual property

2018-02-23 Thread John Snow
Expose the "manual" property via QAPI for the backup-related jobs.
As of this commit, this allows the management API to request the
"concluded" and "dismiss" semantics for backup jobs.

Signed-off-by: John Snow 
---
 blockdev.c   | 19 ---
 qapi/block-core.json | 32 ++--
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 05fd421cdc..2eddb0e726 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3260,7 +3260,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 AioContext *aio_context;
 QDict *options = NULL;
 Error *local_err = NULL;
-int flags;
+int flags, job_flags = BLOCK_JOB_DEFAULT;
 int64_t size;
 bool set_backing_hd = false;
 
@@ -3279,6 +3279,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 if (!backup->has_job_id) {
 backup->job_id = NULL;
 }
+if (!backup->has_manual) {
+backup->manual = false;
+}
 if (!backup->has_compress) {
 backup->compress = false;
 }
@@ -3370,11 +3373,14 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
BlockJobTxn *txn,
 goto out;
 }
 }
+if (backup->manual) {
+job_flags |= BLOCK_JOB_MANUAL;
+}
 
 job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
 backup->sync, bmap, backup->compress,
 backup->on_source_error, backup->on_target_error,
-BLOCK_JOB_DEFAULT, NULL, NULL, txn, _err);
+job_flags, NULL, NULL, txn, _err);
 bdrv_unref(target_bs);
 if (local_err != NULL) {
 error_propagate(errp, local_err);
@@ -3409,6 +3415,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
BlockJobTxn *txn,
 Error *local_err = NULL;
 AioContext *aio_context;
 BlockJob *job = NULL;
+int job_flags = BLOCK_JOB_DEFAULT;
 
 if (!backup->has_speed) {
 backup->speed = 0;
@@ -3422,6 +3429,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
BlockJobTxn *txn,
 if (!backup->has_job_id) {
 backup->job_id = NULL;
 }
+if (!backup->has_manual) {
+backup->manual = false;
+}
 if (!backup->has_compress) {
 backup->compress = false;
 }
@@ -3450,10 +3460,13 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
BlockJobTxn *txn,
 goto out;
 }
 }
+if (backup->manual) {
+job_flags |= BLOCK_JOB_MANUAL;
+}
 job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
 backup->sync, NULL, backup->compress,
 backup->on_source_error, backup->on_target_error,
-BLOCK_JOB_DEFAULT, NULL, NULL, txn, _err);
+job_flags, NULL, NULL, txn, _err);
 if (local_err != NULL) {
 error_propagate(errp, local_err);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 549c6c02d8..7b3af93682 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1177,6 +1177,16 @@
 # @job-id: identifier for the newly-created block job. If
 #  omitted, the device name will be used. (Since 2.7)
 #
+# @manual: True to use an expanded, more explicit job control flow.
+#  Jobs may transition from a running state to a pending state,
+#  where they must be instructed to complete manually via
+#  block-job-finalize.
+#  Jobs belonging to a transaction must either all or all not use this
+#  setting. Once a transaction reaches a pending state, issuing the
+#  finalize command to any one job in the transaction is sufficient
+#  to finalize the entire transaction.
+#  (Since 2.12)
+#
 # @device: the device name or node-name of a root node which should be copied.
 #
 # @target: the target of the new image. If the file exists, or if it
@@ -1217,9 +1227,10 @@
 # Since: 1.6
 ##
 { 'struct': 'DriveBackup',
-  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
-'*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 
'NewImageMode',
-'*speed': 'int', '*bitmap': 'str', '*compress': 'bool',
+  'data': { '*job-id': 'str', '*manual': 'bool', 'device': 'str',
+'target': 'str', '*format': 'str', 'sync': 'MirrorSyncMode',
+'*mode': 'NewImageMode', '*speed': 'int',
+'*bitmap': 'str', '*compress': 'bool',
 '*on-source-error': 'BlockdevOnError',
 '*on-target-error': 'BlockdevOnError' } }
 
@@ -1229,6 +1240,16 @@
 # @job-id: identifier for the newly-created block job. If
 #  omitted, the device name will be used. (Since 2.7)
 #
+# @manual: True to use an expanded, more explicit job control flow.
+#  Jobs may transition from a running state to a pending state,
+#  where they must be