On 11/07/2014 12:39 PM, Kevin Wolf wrote: > If the user neglects to specify the image format, QEMU probes the > image to guess it automatically, for convenience. >
[for those patches in 1-6 where I did not leave comments, I'm happy with
them, and saw that Max already gave R-b so I didn't spend thorough
review time on them]
>
> The other differences of this patch to the old one are that it doesn't
> silently write something different than the guest requested by zeroing
> out some bytes (it fails the request instead) and that it doesn't
> maintain a list of signatures in the raw driver (it calls the usual
> probe function instead).
>
> Note that this change doesn't introduce new breakage for false positive
> cases where the guest legitimately writes data into the first sector
> that matches the signatures of an image format (e.g. for nested virt):
> These cases were broken before, only the failure mode changes from
> corruption after the next restart (when the wrong format is probed) to
> failing the problematic write request.
I would feel better if this commit message explicitly mentioned that the
failed write can ONLY occur when probing occurs; therefore, a user can
ensure that guests can legitimately write anything to the first sector
by explicitly providing a format. But at least the error message does it.
I'm still not 100% convinced this is the patch we want, but am happy
enough that it won't break libvirt (which strives to always pass a
format), so I'm comfortable leaving a review.
>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> block.c | 5 +++--
> block/raw_bsd.c | 57
> ++++++++++++++++++++++++++++++++++++++++++++++-
> include/block/block_int.h | 3 +++
> 3 files changed, 62 insertions(+), 3 deletions(-)
> @@ -158,6 +202,17 @@ static int raw_open(BlockDriverState *bs, QDict
> *options, int flags,
> Error **errp)
> {
> bs->sg = bs->file->sg;
> +
> + if (bs->probed && !bdrv_is_read_only(bs)) {
> + fprintf(stderr,
> + "WARNING: Image format was not specified for '%s'.\n"
> + " Automatically detecting the format is dangerous
> for "
> + "raw images, write operations on block 0 will be
> restricted.\n"
> + " Specify the 'raw' format explicitly to remove the "
> + "restrictions.\n",
This error message works fairly well for me. Maybe the first line could
be a bit longer:
WARNING: Image format was not specified for '%s', and raw was assumed.\n
or maybe:
WARNING: Image format was not specified for '%s', and probing guessed raw.\n
but even with your original shorter wording,
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
