[libvirt] [PATCH] qemu: use guest-fsfreeze-freeze-list command if mountpoints to freeze specified

[Eric Blake]

On 08/11/2014 03:25 PM, Tomoki Sekiyama wrote:
> On 8/11/14 16:36 , "Eric Blake" <eblake at redhat.com> wrote:
> 
>> On 08/08/2014 02:03 PM, Tomoki Sekiyama wrote:
>>> A command to freeze a part of mounted file systems is implemented in
>>> upstream QEMU-guest-agent with a name of 'guest-fsfreeze-freeze-list'.
>>> This fixes the name of the command used to partial fsfreeze in qemu
>>> driver
>>> when 'mountpoints' option is specified to virDomainFSFreeze API.
>>>
>>> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama at hds.com>
>>> ---

>>> @@ -1336,7 +1336,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const
>>> char **mountpoints,
>>>          if (!arg)
>>>              return -1;
>>>  
>>> -        cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze",
>>> +        cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list",
>>
>> Worse, this patch causes a regression for guests using qemu-guest-agent
>> 2.1 (which lacks 'guest-fsfreeze-freeze-list', but has
>> 'guest-fsfreeze-freeze'), for the case where the user provided NULL for
>> the set of mountpoints.  The only proper way to fix this is to do
>> conditional calls: if the user supplies NULL for mountpoints,
>> unconditionally use the old API; if the user supplies non-NULL
>> mountpoints, then use the new command.  Then, depending on whether the
> 
> This is what my patch does -- if mountpoints == NULL, it always uses
> 'guest-fsfreeze-freeze' command, so old qga (<=2.1) still works with
> patched libvirt.

Oh, I missed the bigger context:

    if (mountpoints && nmountpoints) {
        arg = qemuAgentMakeStringsArray(mountpoints, nmountpoints);
        if (!arg)
            return -1;

        cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list",
                                   "a:mountpoints", arg, NULL);
    } else {
        cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL);
    }

so indeed, the command is already conditional, and the difference is now
that a client of qga 2.1 use to report an error that 'mountpoints' is an
unrecognized parameter to 'guest-fsfreeze-freeze', and now reports an
error that 'guest-fsfreeze-freeze-list' is an unknown command.

You've convinced me there is no regression of a command that used to
pass now failing, so that leaves only the error message sanity in question.

> 
>> error message that gets generated when qga 2.1 doesn't support the
>> command is gross looking (basically, if the error message includes
>> "internal error"), then you also need libvirt capability probing so that
>> libvirt can detect up front whether qga is new enough, and give a nicer
>> error message up front instead of relying on the guest agent to complain.
> 
> OK, for nicer error messages, I will post a follow up patch soon.

Thanks.

> 
> That would query 'guest-info', and if qga doesn't support
> 'guest-fsfreeze-freeze-list', gives users an error message that says
> something like "this version of qemu guest agent doesn't support
> specifying mountpoints to freeze". Does it sound correct?

Yes, that's the idiom we've used in several other places for unsupported
commands present only in newer versions (although I'm not sure if we've
done it for the agent in particular, or just for the QMP monitor).

> 
>> I'd like to see a followup patch before libvirt 1.2.8 goes into freeze;
>> if we don't get one in time, then I plan on reverting this patch so that
>> we don't cause regressions to users of older guest agents.

And given that the command has already been conditional, and already
produces not-so-nice error messages, I won't revert this patch after all
(anything you do further will be an improvement, and not a regression
fix; reverts should be reserved only for regressions).

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140811/86fea916/attachment-0001.sig>