Re: [Qemu-devel] [PATCH v2 1/1] Add QMP bits for blockdev-snapshot-sync.

[Jes Sorensen]

On 04/28/11 16:21, Luiz Capitulino wrote:
> On Thu, 28 Apr 2011 15:21:41 +0200
> Jes Sorensen <address@hidden> wrote:
> 
>> On 04/27/11 17:05, Luiz Capitulino wrote:
>>> All arguments should be mandatory in QMP, IMO.
>>
>> Sorry, but there is absolutely no reason to make all arguments
>> mandatory. Sure it can be done, but the only result is a separate
>> handling function for it, so we got more almost identical, but still
>> different code to maintain.
> 
> We shouldn't compromise our external interface quality because of
> implementation details. What I'm really asking here is whether this is
> a good command for our management tools.

It has been discussed repeatedly for months, so yes I will argue it is.

> For example, I've just realized that the new root image is going to be
> automatically created after the first call to this command, and subsequent
> calls w/o the snapshot file name will re-use that file. Is that correct?

No of course not. Every call to snapshot-blockdev will create a new
snapshot. If you don't specify a filename, the new snapshot will be
internal, except you will get an error as we don't currently support that.

snapshots can be chained, so you can end up with a snapshot pointing to
the previous snapshot, which points to the previous snapshot, which
points to the original image .....

> Also note the optional format usage, the command (randomly) picks qcow2 if
> the format is not given. What happens if I pass a raw image and don't specify
> the format? Will it work as it works for qcow2?

The command doesn't pick randomly, it picks the default cow format for
qemu. You cannot pass a raw image as it is not cow compatible.

> I'm not exactly asking for mandatory arguments. For the format argument for
> example, we could try to auto-detect the format (is it possible)? And then
> we could fail with a meaningful error message.

The code is in there now, and there hasn't been requests for this in the
past. The introduction of the qmp wrapper is not the place to discuss this.

> And, I also would like to hear from Anthony, as he's picking up QMP 
> maintenance.

Anthony already stated to me that he was fairly happy with it. However
you are the QMP maintainer, so it needs to go in via the QMP tree.

>>> Finally, what's the expect behavior when -snapshot is used? I'm getting
>>> this:
>>>
>>>  (qemu) snapshot_blkdev ide0-hd0 snap-test
>>>  Could not open '/tmp/vl.6w8YXA'
>>>  (qemu)
>>
>> What type of file system is your /tmp?
> 
> ext4
> 
>> You need to provide full path to
>> the snapshot file if you don't want it created next to where your qemu
>> binary is being executed.
> 
> I'm not running in /tmp.

Well something is funny with your /tmp then, as the above isn't normal
behavior.

>>> At first, I don't see why we shouldn't generate the live snapshot, but 
>>> anyway,
>>> any special behavior like this should be noted in the section called Notes
>>> in the command's documentation.
>>>
>>
>> I don't follow this at all, please elaborate.
> 
> Any kind of limitation should be noted in the documentation.

We cannot document a users choice of /tmp, when /tmp isn't part of what
the command does.

Jes