Kevin Wolf <kw...@redhat.com> writes: > Am 29.10.2014 um 14:54 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> > Instead, let me try once more to sell my old proposal [1] from the >> > thread you mentioned: >> > >> >> What if we let the raw driver know that it was probed and then it >> >> enables a check that returns -EIO for any write on the first 2k if that >> >> write would make the image look like a different format? >> > >> > Attacks the problem where it arises instead of trying to detect the >> > outcome of it, and works in whatever way it is nested in the BDS graph >> > and whatever way is used to address the image file. >> > >> > Kevin >> > >> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02548.html >> >> Arbitrarily breaking the virtual hardware that way is incompatible, too. >> It's a bigger no-no for me than changing user interface corner cases or >> deciding what to do with a file based on its file name extension. > > It is arbitrarily breaking theoretical cases, while your patch is less > arbitrarily breaking common cases. I strongly prefer the former.
I challenge "theoretical" as well as "common". For "theoretical", see below. As to "common": are unrecognizable image names really common? I doubt it. If I'm wrong, I expect users to complain about my warning, and then we can reconsider. > Nobody will ever want to write a qcow2 header into their boot sector for > just running a guest: > > 0: 51 push %cx > 1: 46 inc %si > 2: 49 dec %cx > 3: fb sti > > This code doesn't make any sense. It's similar for other formats. And if > they really have some odd reason to do that, the fix is easy: Either > don't use raw, or pass an explicit format=raw. A disk needn't start with a PC-style boot sector. Even on a PC. Not every disk needs to be bootable, or even partitioned. Databases exist that like to work on raw devices. Giving them /dev/sdb instead of a whole-disk partition /dev/sdb1 may not be the smartest thing to do, but it *works* with real hardware, so why not with virtual hardware? You either have to prevent *any* writing of the first 2048 bytes (the part that can be examined by a bdrv_probe() method, or your have to prevent writing anything a probe recognizes, or the user has to specify the format explicitly. If you do the former, you're way outside the realm of "theoretical". If you do the latter, the degree of "theoreticalness" depends on the union of the patterns you prevent. Issues: 1. Anthony's method of checking a few known signatures is fragile. The only obviously robust way to test "is probing going to find something other than 'raw'?" is running the probes. Feasible. 2. Whether the union of patterns qualifies as "theoretical" for all our targets is not obvious, and whether it'll remain "theoretical" for all future formats and target machines even less. 3. A write access that works just fine in one QEMU version can be rejected by another version that recognizes more formats. Or probes the same formats differently. 4. Rejecting a write fails *late*, and looks like hardware failure to the guest. With imperfect guest software, this risks data corruption. (4) is a deal breaker for me. (3) adds a cherry on top. I care about command line compatibility, but I care about guest ABI stability a whole lot more. I'm utterly unwilling to risk breaking a running guest's hardware just so that users can continue to call their qcow2 images "I want my pony.dammit". >> Anthony tried something similar (commit 79368c8), but couldn't get it >> right (commit 8b33d9e). > > The discussion back then: http://patchwork.ozlabs.org/patch/58980/ > > The problem with Anthony's code was that he didn't handle a qiov > correctly that had unaligned members. With today's block layer, this is > not a big deal to implement correctly. We're running coroutines instead > of AIO callbacks and we don't have to do all the manual qiov fixing > magic that Anthony had in his patch, util/iov.c provides all you need. > > I'll send out an RFC series that implements this. I'm strongly opposed to this idea.