[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/3] block rebase: initial qemu implementation



On 02/01/2012 12:53 PM, Laine Stump wrote:
> On 02/01/2012 12:06 AM, Eric Blake wrote:
>> This is a trivial implementation, which works with the current
>> released qemu 1.0 with backports of preliminary block pull but
>> no partial rebase.  Future patches will update the monitor handling
>> to support an optional parameter for partial rebase; but as qemu
>> 1.1 is unreleased, it can be in later patches, designed to be
>> backported on top of the supported API.
>>
>> * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Add parameter,
>> @@ -11328,16 +11328,18 @@ qemuDomainBlockJobImpl(virDomainPtr dom,
>> const char *path,
>>           goto cleanup;
>>       }
>>
>> -    if (!virDomainObjIsActive(vm)) {
>> -        qemuReportError(VIR_ERR_OPERATION_INVALID,
>> -                        "%s", _("domain is not running"));
>> -        goto cleanup;
>> -    }
>> -
> 
> For all the existing virDomainBlockxxx() APIs I think this is a change
> in behavior, right? THey used to fial for inactive domains, and now they
> may succeed. Could this create any problems?

No.  This was a case of redundant code - we checked for isactive, then
obtained the job, then re-checked for isactive (since obtaining the job
involves dropping lock, so the domain could have quit in the meantime).
 The post-job check is mandatory (we've had bugs before where libvirtd
locks up if we forget the post-job check), and the pre-job check can be
safely be eliminated since the post-job check will always do the right
thing; the only difference in eliminating the pre-job check is that the
API might take a bit longer due to trying to obtain the job on an
inactive domain compared to where it used to error out without even
trying to get the job.

> 
> If not, then ACK. This is all pretty mechanical.

I think I answered your question, and thus I'm pushing the series.

> 
>>       device = qemuDiskPathToAlias(vm, path);
>>       if (!device) {
>>           goto cleanup;
>>       }
>> +    /* XXX - add a qemu capability check; if qemu 1.1 or newer, then
>> +     *  validate and convert non-NULL base into something that can
>> +     * be passed as optional base argument.  */
>> +    if (base)  {
>> +        qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> +                        _("partial block pull is not supported with
>> this QEMU binary"));
>> +        goto cleanup;
>> +    }
> 
> So the API is in, and the actual functionality missing until qemu that
> supports it is available.

And I'm in the process of writing/testing that as well, it's just that
it's a bit lower on my priority queue :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]