Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6087 bytes --]

On 02.02.2017 08:32, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 01.02.2017 13:28, Daniel P. Berrange wrote:
>>> On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote:
>>>> On 01.02.2017 13:16, Daniel P. Berrange wrote:
>>>>> On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote:
>>>>>> On 30.01.2017 19:37, Eric Blake wrote:
>>>>>>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote:
>>>>>>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote:
>>>>>>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote:
>>>>>>>>>> The -n arg to the convert command allows use of a pre-existing image,
>>>>>>>>>> rather than creating a new image. This adds a -n arg to the dd command
>>>>>>>>>> to get feature parity.
>>>>>>>>>
>>>>>>>>> I remember there was a discussion about changing qemu-img dd's default to a
>>>>>>>>> "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part
>>>>>>>>> hasn't made it into the tree, and I'm not sure which direction we should take.
>>>>>>>>> (Personally I think default to nocreat is a good idea).
>>>>>>>>
>>>>>>>> Use nocreat by default would be semantically different from real "dd"
>>>>>>>> binary which feels undesirable if the goal is to make "qemu-img dd"
>>>>>>>> be as consistent with "dd" as possible.
>>>>>>>>
>>>>>>>> It would be trivial to rewrite this patch to add support for the "conv"
>>>>>>>> option, allowing the user to explicitly give 'qemu-img dd conv=nocreat'
>>>>>>>> instead of my 'qemu-img dd -n' syntax, without changing default semantics.
>>>>>>>
>>>>>>> Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
>>>>>>
>>>>>> The original idea was to make conv=nocreat a mandatory option, I think.
>>>>>> qemu-img was supposed error out if the user did not specify it.
>>>>>
>>>>> I'm not really seeing a benefit in doing that - it would just break
>>>>> existing usage of qemu-img dd for no obvious benefit.
>>>>
>>>> Well... Is there existing usage?
>>>
>>> It shipped in 2.8.0 though, so imho that means we have to assume there
>>> are users, and thus additions must to be backwards compatible from now
>>> on.
>>
>> Depends. I don't think there are too many users, so we could still
>> justify a change if there's a very good reason for it.
>>
>> I do agree that it's probably not a very good reason, though.
>>
>>>> The benefit would be that one could (should?) expect qemu-img dd to
>>>> behave on disk images as if they were block devices; and dd to a block
>>>> device will not truncate or "recreate" it.
>>>>
>>>> If you don't give nocreat, it's thus a bit unclear whether you want to
>>>> delete and recreate the target or whether you want to write into it.
>>>> Some may expect qemu-img dd to behave as if the target is a normal file
>>>> (delete and recreate it), others may expect it's treated like a block
>>>> device (just write into it). If you force the user to specify nocreat,
>>>> it would make the behavior clear.
>>>>
>>>> (And you can always delete+recreate the target with qemu-img create.)
>>>>
>>>> It's all a bit complicated. :-/
>>>
>>> If the goal is to be compatible with /usr/bin/dd then IIUC, the behaviour
>>> needs to be
>>>
>>>  - If target is a block device, then silently assume nocreat|notrunc
>>>    is set, even if not specified by user
>>>
>>>  - If target is a file, then silently create & truncate the file
>>>    unless nocreat or notrunc are set
>>
>> Yes. But you could easily argue that every image file is a "block device".
> 
> No.  /bin/dd treats exactly block special files as block special files,
> so the qemu-img command that tries to behave like it should do, too.

*/usr*/bin/dd (O:-)) also treats qcow2 files like raw files.

> In case you say that's inconvenient: pretty much everything about dd's
> archaic user interface is inconvenient.  If you want convenient, roll
> your own.  If you want familiar, stick to the original.

I agree. But qemu-img dd already is not dd. It interprets disk image
files as virtual disks instead of as plain files. The question is
whether virtual disks are to be treated as block devices or as files.

I don't have a strong opinion on the matter. Either way will surprise
some people. The original issue was whether to make nocreat/notrunc a
mandatory option, so if we didn't have any backwards compatibility
issues, it would be the following two surprises:

(1) Don't make nocreat/notrunc mandatory (as it is now). Then people
    who expect qemu-img dd to treat image files as block devices will
    be surprised that all their data is gone. Bad.

(2) Make it mandatory. Then people who don't expect this (i.e.
    everyone) will be surprised about the error message "nocreat is
    mandatory, please use it." Then they will fix their command line
    and can proceed. Much less bad.

Of course, "much less bad" is offset by the fact that everyone is
affected. I think it's still less bad, though.

But we do have backwards compatibility to care about (at least a bit),
so I don't think it's worth changing the behavior now.


You may argue that actually nobody will be affected by (1). But I don't
think so. I personally usually use dd to copy something off or onto a
device file, often block devices. Disk images literally are block
devices in the guest. It is entirely conceivable that someone would (a)
associate dd with "copy things from/to block devices" and (b) assume
that *qemu*-img dd behaves as from the guest's perspective (where the
image is a block device).

That is because qemu-img dd does *not* view the image entirely from the
host's perspective. It does interpret the image file contents, i.e., it
does look at it from a guest's perspective in that regard.


I agree that qemu-img dd probably was a bad idea. I agree that we need
to make sure people know that it is basically a different interface of
qemu-img convert.

Yes, yes, I'll get to write the conversion and man page updates when I
find the time to do so.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]