Re: [libvirt] [PATCH REBASE 1/5] qemu: erase synchronous block job cancel mentions in comments
pushed slightly reworded
On 03.05.2018 10:01, Nikolay Shirokovskiy wrote:
>
>
> On 01.05.2018 01:03, John Ferlan wrote:
>>
>>
>> On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote:
>>> Commit [1] dropped support for synchronous block job cancel.
>>> This patch erases remnants from comments.
>>>
>>> [1] commit 2350d101 "qemu: Remove support for legacy block jobs"
>>>
>>> Signed-off-by: Nikolay Shirokovskiy
>>> ---
>>> src/qemu/qemu_driver.c | 13 ++---
>>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 4e06c9c..0dd6032 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -16914,13 +16914,12 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
>>> if (save)
>>> ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir,
>>> vm, driver->caps));
>>>
>>> -/* With synchronous block cancel, we must synthesize an event, and
>>> - * we silently ignore the ABORT_ASYNC flag. With asynchronous
>>> - * block cancel, the event will come from qemu and will update the
>>> - * XML as appropriate, but without the ABORT_ASYNC flag, we must
>>> - * block to guarantee synchronous operation. We do the waiting
>>> - * while still holding the VM job, to prevent newly scheduled
>>> - * block jobs from confusing us. */
>>> +/*
>>> + * With asynchronous block cancel, the event will come from qemu and
>>> will
>>
>> /* With...
>>
>> IOW: don't have a separate line for the open comment...
>>
>>> + * update the XML as appropriate, but without the ABORT_ASYNC flag, we
>>> must
>>> + * block to guarantee synchronous operation. We do the waiting while
>>> still
>>> + * holding the VM job, to prevent newly scheduled block jobs from
>>> confusing
>>> + * us. */
>>
>> This works, but the lead-in doesn't mean as much without the "With
>> synchronous..." as the alternative... So perhaps even simpler:
>>
>> Wait for the QEMU event to update the the blockjob with the domain lock
>> to prevent newly scheduled block jobs from confusing us. The event will
>> update the XML as appropriate. Without the ABORT_ASYNC flag, we must
>> block to guarantee synchronous completion.
>>
>> With at least the comment entry fixup... feel free to use my wording as
>> well (as long as it makes sense to you)..
> I can not use your version unchanged too... It starts saying that we wait but
> we wait only if ABORT_ASYNC is not set. I'll reword the proposed version
> too to eliminate the mention that we use qemu's async block job cancel
> API.
>
>>
>> Reviewed-by: John Ferlan
>>
>> but please wait for 4.4.0 to open before pushing.
>>
>> John
>>
>>
>>> if (!async) {
>>> qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>> qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk, NULL);
>>>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 1/5] qemu: erase synchronous block job cancel mentions in comments
On 01.05.2018 01:03, John Ferlan wrote:
>
>
> On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote:
>> Commit [1] dropped support for synchronous block job cancel.
>> This patch erases remnants from comments.
>>
>> [1] commit 2350d101 "qemu: Remove support for legacy block jobs"
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/qemu/qemu_driver.c | 13 ++---
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 4e06c9c..0dd6032 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -16914,13 +16914,12 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
>> if (save)
>> ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
>> driver->caps));
>>
>> -/* With synchronous block cancel, we must synthesize an event, and
>> - * we silently ignore the ABORT_ASYNC flag. With asynchronous
>> - * block cancel, the event will come from qemu and will update the
>> - * XML as appropriate, but without the ABORT_ASYNC flag, we must
>> - * block to guarantee synchronous operation. We do the waiting
>> - * while still holding the VM job, to prevent newly scheduled
>> - * block jobs from confusing us. */
>> +/*
>> + * With asynchronous block cancel, the event will come from qemu and
>> will
>
> /* With...
>
> IOW: don't have a separate line for the open comment...
>
>> + * update the XML as appropriate, but without the ABORT_ASYNC flag, we
>> must
>> + * block to guarantee synchronous operation. We do the waiting while
>> still
>> + * holding the VM job, to prevent newly scheduled block jobs from
>> confusing
>> + * us. */
>
> This works, but the lead-in doesn't mean as much without the "With
> synchronous..." as the alternative... So perhaps even simpler:
>
> Wait for the QEMU event to update the the blockjob with the domain lock
> to prevent newly scheduled block jobs from confusing us. The event will
> update the XML as appropriate. Without the ABORT_ASYNC flag, we must
> block to guarantee synchronous completion.
>
> With at least the comment entry fixup... feel free to use my wording as
> well (as long as it makes sense to you)..
I can not use your version unchanged too... It starts saying that we wait but
we wait only if ABORT_ASYNC is not set. I'll reword the proposed version
too to eliminate the mention that we use qemu's async block job cancel
API.
>
> Reviewed-by: John Ferlan
>
> but please wait for 4.4.0 to open before pushing.
>
> John
>
>
>> if (!async) {
>> qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>> qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk, NULL);
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 1/5] qemu: erase synchronous block job cancel mentions in comments
On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote:
> Commit [1] dropped support for synchronous block job cancel.
> This patch erases remnants from comments.
>
> [1] commit 2350d101 "qemu: Remove support for legacy block jobs"
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_driver.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4e06c9c..0dd6032 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16914,13 +16914,12 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> if (save)
> ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
> driver->caps));
>
> -/* With synchronous block cancel, we must synthesize an event, and
> - * we silently ignore the ABORT_ASYNC flag. With asynchronous
> - * block cancel, the event will come from qemu and will update the
> - * XML as appropriate, but without the ABORT_ASYNC flag, we must
> - * block to guarantee synchronous operation. We do the waiting
> - * while still holding the VM job, to prevent newly scheduled
> - * block jobs from confusing us. */
> +/*
> + * With asynchronous block cancel, the event will come from qemu and will
/* With...
IOW: don't have a separate line for the open comment...
> + * update the XML as appropriate, but without the ABORT_ASYNC flag, we
> must
> + * block to guarantee synchronous operation. We do the waiting while
> still
> + * holding the VM job, to prevent newly scheduled block jobs from
> confusing
> + * us. */
This works, but the lead-in doesn't mean as much without the "With
synchronous..." as the alternative... So perhaps even simpler:
Wait for the QEMU event to update the the blockjob with the domain lock
to prevent newly scheduled block jobs from confusing us. The event will
update the XML as appropriate. Without the ABORT_ASYNC flag, we must
block to guarantee synchronous completion.
With at least the comment entry fixup... feel free to use my wording as
well (as long as it makes sense to you)..
Reviewed-by: John Ferlan
but please wait for 4.4.0 to open before pushing.
John
> if (!async) {
> qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk, NULL);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
