Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 07/12/2017 01:00 PM, Max Reitz wrote: > On 2017-07-12 16:56, Kevin Wolf wrote: >> Am 12.07.2017 um 16:42 hat Eric Blake geschrieben: >>> On 05/17/2017 07:38 AM, Max Reitz wrote: >>> >>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786 >>> >>> Or, rather, force the open of a backing image if one was specified >>> for creation. Using a similar -unsafe option as rebase, allow >>> qemu-img >>> to ignore the backing file validation if possible. >>> > >>> >> I suspect this is because the backing file is opened somewhere and >> trying to open it breaks now with the locking series applied. > > I think we can legitimately set force-shared=on for opening the backing > file when testing whether the file exists. For testing whether the file exists, probably. I wouldn't call it legitimately because I don't like making that the default at all, but it should work. But then we have to differentiate between testing whether the file exists and opening it to read its size; I'm even more wary regarding the latter. All in all, I've stated my opinion on this matter on IRC: I don't know how much we need this patch. It's nice to have, it's convenient to know you have mistyped the backing filename before you try to use the image in qemu, but it's not critical. I don't consider the current behavior a bug per-se, it's just counterintuitive behavior (that will not cut your head off if you don't know it, though!). >>> >>> The fact that this topic came up on the mailing list again means we >>> should probably consider something along these lines for 2.10. >>> (Also, we have a lot of counterintuitive behavior. See this: $ qemu-img create -f qcow2 sub/backing.qcow2 64M Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such file or directory Yeah, sure, you'll know after you've done the mistake once. But the same applies here, too, doesn't it...?) So for me, it's a question of convenience: How much does the current behavior annoy users? How much annoyance will changing the behavior bring? I'm not (or rather, no longer) convinced the former outweighs the latter, especially since it means having to change management tools (which create external snapshots with qemu-img create while the backing image is in use by qemu). If you think otherwise, well, you're the main maintainer. :-) Max PS: Can there be a middle ground? Like just printing a warning if the backing file cannot be opened and we don't absolutely have to? >>> >>> Middle ground may be harder, but may be nicer (especially since we are >>> talking about tightening behavior, making things that used to work now >>> fail is not nice if there is not a transition period where it first just >>> warns). >> >> We can do the nowadays usual thing: Add a -u option, print a deprecation >> warning if we don't have -u and can't open the backing file, and put >> it into the list of things to remove in 3.0. > > Works for me. > > Max > Sold, thanks for the idea. (Feel free to quabble on "unsafe" in the upcoming patch, I'm not sold on that either, but I wanted it to match the -u flag for rebase.) ((Maybe I'll keep it as -u but pretend it stands for --unchecked or some such nonsense.))
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On Wed, Jul 12, 2017 at 8:00 PM Max Reitz wrote:
> On 2017-07-12 16:56, Kevin Wolf wrote:
> > Am 12.07.2017 um 16:42 hat Eric Blake geschrieben:
> >> On 05/17/2017 07:38 AM, Max Reitz wrote:
> >>
> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
> >>
> >> Or, rather, force the open of a backing image if one was
> specified
> >> for creation. Using a similar -unsafe option as rebase, allow
> qemu-img
> >> to ignore the backing file validation if possible.
> >>
>
> >>
> > I suspect this is because the backing file is opened somewhere and
> > trying to open it breaks now with the locking series applied.
>
> I think we can legitimately set force-shared=on for opening the
> backing
> file when testing whether the file exists.
> >>>
> >>> For testing whether the file exists, probably. I wouldn't call it
> >>> legitimately because I don't like making that the default at all, but
> it
> >>> should work.
> >>>
> >>> But then we have to differentiate between testing whether the file
> >>> exists and opening it to read its size; I'm even more wary regarding
> the
> >>> latter.
> >>>
> >>> All in all, I've stated my opinion on this matter on IRC: I don't know
> >>> how much we need this patch. It's nice to have, it's convenient to know
> >>> you have mistyped the backing filename before you try to use the image
> >>> in qemu, but it's not critical. I don't consider the current behavior a
> >>> bug per-se, it's just counterintuitive behavior (that will not cut your
> >>> head off if you don't know it, though!).
> >>>
> >>
> >> The fact that this topic came up on the mailing list again means we
> >> should probably consider something along these lines for 2.10.
> >>
> >>> (Also, we have a lot of counterintuitive behavior. See this:
> >>>
> >>> $ qemu-img create -f qcow2 sub/backing.qcow2 64M
> >>> Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off
> >>> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> >>> $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2
> >>> qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No
> such
> >>> file or directory
> >>>
> >>> Yeah, sure, you'll know after you've done the mistake once. But the
> same
> >>> applies here, too, doesn't it...?)
> >>>
> >>> So for me, it's a question of convenience: How much does the current
> >>> behavior annoy users? How much annoyance will changing the behavior
> >>> bring? I'm not (or rather, no longer) convinced the former outweighs
> the
> >>> latter, especially since it means having to change management tools
> >>> (which create external snapshots with qemu-img create while the backing
> >>> image is in use by qemu).
> >>>
> >>> If you think otherwise, well, you're the main maintainer. :-)
> >>>
> >>> Max
> >>>
> >>>
> >>> PS: Can there be a middle ground? Like just printing a warning if the
> >>> backing file cannot be opened and we don't absolutely have to?
> >>
> >> Middle ground may be harder, but may be nicer (especially since we are
> >> talking about tightening behavior, making things that used to work now
> >> fail is not nice if there is not a transition period where it first just
> >> warns).
> >
> > We can do the nowadays usual thing: Add a -u option, print a deprecation
> > warning if we don't have -u and can't open the backing file, and put
> > it into the list of things to remove in 3.0.
>
> Works for me.
>
Should work for oVirt.
When we consume the -u/--unsafe option, should we use qemu-img version,
or maybe we need an official api for detecting capabilities?
qemu-img caps --format json
{
"unsafe-create": True,
...
Nir
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 2017-07-12 16:56, Kevin Wolf wrote: > Am 12.07.2017 um 16:42 hat Eric Blake geschrieben: >> On 05/17/2017 07:38 AM, Max Reitz wrote: >> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786 >> >> Or, rather, force the open of a backing image if one was specified >> for creation. Using a similar -unsafe option as rebase, allow >> qemu-img >> to ignore the backing file validation if possible. >> >> > I suspect this is because the backing file is opened somewhere and > trying to open it breaks now with the locking series applied. I think we can legitimately set force-shared=on for opening the backing file when testing whether the file exists. >>> >>> For testing whether the file exists, probably. I wouldn't call it >>> legitimately because I don't like making that the default at all, but it >>> should work. >>> >>> But then we have to differentiate between testing whether the file >>> exists and opening it to read its size; I'm even more wary regarding the >>> latter. >>> >>> All in all, I've stated my opinion on this matter on IRC: I don't know >>> how much we need this patch. It's nice to have, it's convenient to know >>> you have mistyped the backing filename before you try to use the image >>> in qemu, but it's not critical. I don't consider the current behavior a >>> bug per-se, it's just counterintuitive behavior (that will not cut your >>> head off if you don't know it, though!). >>> >> >> The fact that this topic came up on the mailing list again means we >> should probably consider something along these lines for 2.10. >> >>> (Also, we have a lot of counterintuitive behavior. See this: >>> >>> $ qemu-img create -f qcow2 sub/backing.qcow2 64M >>> Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off >>> cluster_size=65536 lazy_refcounts=off refcount_bits=16 >>> $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 >>> qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such >>> file or directory >>> >>> Yeah, sure, you'll know after you've done the mistake once. But the same >>> applies here, too, doesn't it...?) >>> >>> So for me, it's a question of convenience: How much does the current >>> behavior annoy users? How much annoyance will changing the behavior >>> bring? I'm not (or rather, no longer) convinced the former outweighs the >>> latter, especially since it means having to change management tools >>> (which create external snapshots with qemu-img create while the backing >>> image is in use by qemu). >>> >>> If you think otherwise, well, you're the main maintainer. :-) >>> >>> Max >>> >>> >>> PS: Can there be a middle ground? Like just printing a warning if the >>> backing file cannot be opened and we don't absolutely have to? >> >> Middle ground may be harder, but may be nicer (especially since we are >> talking about tightening behavior, making things that used to work now >> fail is not nice if there is not a transition period where it first just >> warns). > > We can do the nowadays usual thing: Add a -u option, print a deprecation > warning if we don't have -u and can't open the backing file, and put > it into the list of things to remove in 3.0. Works for me. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
Am 12.07.2017 um 16:42 hat Eric Blake geschrieben: > On 05/17/2017 07:38 AM, Max Reitz wrote: > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786 > > Or, rather, force the open of a backing image if one was specified > for creation. Using a similar -unsafe option as rebase, allow > qemu-img > to ignore the backing file validation if possible. > > >> > > >>> I suspect this is because the backing file is opened somewhere and > >>> trying to open it breaks now with the locking series applied. > >> > >> I think we can legitimately set force-shared=on for opening the backing > >> file when testing whether the file exists. > > > > For testing whether the file exists, probably. I wouldn't call it > > legitimately because I don't like making that the default at all, but it > > should work. > > > > But then we have to differentiate between testing whether the file > > exists and opening it to read its size; I'm even more wary regarding the > > latter. > > > > All in all, I've stated my opinion on this matter on IRC: I don't know > > how much we need this patch. It's nice to have, it's convenient to know > > you have mistyped the backing filename before you try to use the image > > in qemu, but it's not critical. I don't consider the current behavior a > > bug per-se, it's just counterintuitive behavior (that will not cut your > > head off if you don't know it, though!). > > > > The fact that this topic came up on the mailing list again means we > should probably consider something along these lines for 2.10. > > > (Also, we have a lot of counterintuitive behavior. See this: > > > > $ qemu-img create -f qcow2 sub/backing.qcow2 64M > > Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off > > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > > $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 > > qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such > > file or directory > > > > Yeah, sure, you'll know after you've done the mistake once. But the same > > applies here, too, doesn't it...?) > > > > So for me, it's a question of convenience: How much does the current > > behavior annoy users? How much annoyance will changing the behavior > > bring? I'm not (or rather, no longer) convinced the former outweighs the > > latter, especially since it means having to change management tools > > (which create external snapshots with qemu-img create while the backing > > image is in use by qemu). > > > > If you think otherwise, well, you're the main maintainer. :-) > > > > Max > > > > > > PS: Can there be a middle ground? Like just printing a warning if the > > backing file cannot be opened and we don't absolutely have to? > > Middle ground may be harder, but may be nicer (especially since we are > talking about tightening behavior, making things that used to work now > fail is not nice if there is not a transition period where it first just > warns). We can do the nowadays usual thing: Add a -u option, print a deprecation warning if we don't have -u and can't open the backing file, and put it into the list of things to remove in 3.0. Kevin pgpgeiYshcQdV.pgp Description: PGP signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 05/17/2017 07:38 AM, Max Reitz wrote: Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786 Or, rather, force the open of a backing image if one was specified for creation. Using a similar -unsafe option as rebase, allow qemu-img to ignore the backing file validation if possible. >> >>> I suspect this is because the backing file is opened somewhere and >>> trying to open it breaks now with the locking series applied. >> >> I think we can legitimately set force-shared=on for opening the backing >> file when testing whether the file exists. > > For testing whether the file exists, probably. I wouldn't call it > legitimately because I don't like making that the default at all, but it > should work. > > But then we have to differentiate between testing whether the file > exists and opening it to read its size; I'm even more wary regarding the > latter. > > All in all, I've stated my opinion on this matter on IRC: I don't know > how much we need this patch. It's nice to have, it's convenient to know > you have mistyped the backing filename before you try to use the image > in qemu, but it's not critical. I don't consider the current behavior a > bug per-se, it's just counterintuitive behavior (that will not cut your > head off if you don't know it, though!). > The fact that this topic came up on the mailing list again means we should probably consider something along these lines for 2.10. > (Also, we have a lot of counterintuitive behavior. See this: > > $ qemu-img create -f qcow2 sub/backing.qcow2 64M > Formatting 'sub/backing.qcow2', fmt=qcow2 size=67108864 encryption=off > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > $ qemu-img create -f qcow2 -b sub/backing.qcow2 sub/top.qcow2 > qemu-img: sub/top.qcow2: Could not open 'sub/sub/backing.qcow2': No such > file or directory > > Yeah, sure, you'll know after you've done the mistake once. But the same > applies here, too, doesn't it...?) > > So for me, it's a question of convenience: How much does the current > behavior annoy users? How much annoyance will changing the behavior > bring? I'm not (or rather, no longer) convinced the former outweighs the > latter, especially since it means having to change management tools > (which create external snapshots with qemu-img create while the backing > image is in use by qemu). > > If you think otherwise, well, you're the main maintainer. :-) > > Max > > > PS: Can there be a middle ground? Like just printing a warning if the > backing file cannot be opened and we don't absolutely have to? Middle ground may be harder, but may be nicer (especially since we are talking about tightening behavior, making things that used to work now fail is not nice if there is not a transition period where it first just warns). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 2017-05-16 10:17, Kevin Wolf wrote:
> Am 15.05.2017 um 21:17 hat Max Reitz geschrieben:
>> On 2017-05-15 20:41, Max Reitz wrote:
>>> On 2017-05-12 21:47, John Snow wrote:
On 05/12/2017 03:46 PM, Eric Blake wrote:
> On 05/12/2017 01:07 PM, Max Reitz wrote:
>> On 2017-05-11 20:27, John Snow wrote:
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>>>
>>> Or, rather, force the open of a backing image if one was specified
>>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>>> to ignore the backing file validation if possible.
>>>
>
>>> +++ b/block.c
>>> @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename,
>>> const char *fmt,
>>> // The size for the image must always be specified, with one
>>> exception:
>>> // If we are using a backing file, we can obtain the size from
>>> there
>>> size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
>>> -if (size == -1) {
>>
>> "Hang on, why should this be -1 when the defval is 0? Where does the -1
>> come from?"
>> "..."
>> "Oh, the option exists and is set to -1? Why is that?"
>> "..."
>> "Oh, because this function always sets it itself, and because @img_size
>> is set to (uint64_t)-1."
>
> I had pretty much the same conversation on my v1 review.
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>
>>
>> First, I won't start with how signed integer overflow is
>> implementation-defined in C because I hope you have thrashed that out
>> with Eric (I hope that "to thrash out" is a good translation for
>> "auskaspern" (lit. "to buffoon out").).
>
> Sounds like a reasonable choice of words, even if I don't speak the
> counterpart language to validate your translation.
>
> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
> wrinkles at you.
>>>
>>> We're not really fine because that conversion happens when the result of
>>> qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
>>>
> I seem to recall that qemu has chosen to use compiler flags and/or
> assumptions that we are using 2s-complement arithmetic with sane
> behavior (that is, tighter behavior than the bare minimum that C
> requires), because it was easier than auditing our code for strict C
> compliance on border cases of conversions from unsigned to signed that
> trigger undefined behavior. But again, I don't think it affects this
> patch (where our conversion is only from signed to unsigned, and that is
> well-defined behavior).
>>>
>>> Right. Which is why I said I won't even start on it, but of course I
>>> did. O:-)
>>>
>> Second, well, at least we should put -1 as the default value here, then.
>
> Indeed, now that two reviewers have tripped on it,
> qemu_opt_get_size(,,-1) would be nicer.
>
>>
>> Not strictly your fault or something that you need to fix, but it is
>> just a single line in the vicinity...
>>
>> Let me know if you want to address this, for now I'll leave a
>>
>> Reviewed-by: Max Reitz
>>
>> here if you don't want to.
>
> I'm okay whether you want to squash that fix into this patch, or whether
> you do it as a separate followup patch.
>
I had considered the issue separate; but you're welcome to either write
a patch or squish it into this one, I'm not going to be picky.
>>>
>>> Yep, it is a separate issue, just related. :-)
>>>
>>> But since you and Eric agree, I've squashed it in and thus I'm more than
>>> glad to thank you very much and announce this patch as applied to my
>>> block branch:
>>>
>>> https://github.com/XanClic/qemu/commits/block
>>
>> ...well, so much for that. I'll have to unstage it again because it
>> breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
>> failing to acquire image locks. :-/
>>
>> I suspect this is because the backing file is opened somewhere and
>> trying to open it breaks now with the locking series applied.
>
> I think we can legitimately set force-shared=on for opening the backing
> file when testing whether the file exists.
For testing whether the file exists, probably. I wouldn't call it
legitimately because I don't like making that the default at all, but it
should work.
But then we have to differentiate between testing whether the file
exists and opening it to read its size; I'm even more wary regarding the
latter.
All in all, I've stated my opinion on this matter on IRC: I don't know
how much we need this patch. It's nice to have, it's convenient to know
you have mistyped the backing filename before you try to use the image
in qemu, but it's not critical. I don't consider the current behavior a
bug per-se, it's just counterintuitive behavior (
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
Am 15.05.2017 um 21:17 hat Max Reitz geschrieben:
> On 2017-05-15 20:41, Max Reitz wrote:
> > On 2017-05-12 21:47, John Snow wrote:
> >>
> >>
> >> On 05/12/2017 03:46 PM, Eric Blake wrote:
> >>> On 05/12/2017 01:07 PM, Max Reitz wrote:
> On 2017-05-11 20:27, John Snow wrote:
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
> >
> > Or, rather, force the open of a backing image if one was specified
> > for creation. Using a similar -unsafe option as rebase, allow qemu-img
> > to ignore the backing file validation if possible.
> >
> >>>
> > +++ b/block.c
> > @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename,
> > const char *fmt,
> > // The size for the image must always be specified, with one
> > exception:
> > // If we are using a backing file, we can obtain the size from
> > there
> > size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> > -if (size == -1) {
>
> "Hang on, why should this be -1 when the defval is 0? Where does the -1
> come from?"
> "..."
> "Oh, the option exists and is set to -1? Why is that?"
> "..."
> "Oh, because this function always sets it itself, and because @img_size
> is set to (uint64_t)-1."
> >>>
> >>> I had pretty much the same conversation on my v1 review.
> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
> >>>
>
> First, I won't start with how signed integer overflow is
> implementation-defined in C because I hope you have thrashed that out
> with Eric (I hope that "to thrash out" is a good translation for
> "auskaspern" (lit. "to buffoon out").).
> >>>
> >>> Sounds like a reasonable choice of words, even if I don't speak the
> >>> counterpart language to validate your translation.
> >>>
> >>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
> >>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
> >>> wrinkles at you.
> >
> > We're not really fine because that conversion happens when the result of
> > qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
> >
> >>> I seem to recall that qemu has chosen to use compiler flags and/or
> >>> assumptions that we are using 2s-complement arithmetic with sane
> >>> behavior (that is, tighter behavior than the bare minimum that C
> >>> requires), because it was easier than auditing our code for strict C
> >>> compliance on border cases of conversions from unsigned to signed that
> >>> trigger undefined behavior. But again, I don't think it affects this
> >>> patch (where our conversion is only from signed to unsigned, and that is
> >>> well-defined behavior).
> >
> > Right. Which is why I said I won't even start on it, but of course I
> > did. O:-)
> >
> Second, well, at least we should put -1 as the default value here, then.
> >>>
> >>> Indeed, now that two reviewers have tripped on it,
> >>> qemu_opt_get_size(,,-1) would be nicer.
> >>>
>
> Not strictly your fault or something that you need to fix, but it is
> just a single line in the vicinity...
>
> Let me know if you want to address this, for now I'll leave a
>
> Reviewed-by: Max Reitz
>
> here if you don't want to.
> >>>
> >>> I'm okay whether you want to squash that fix into this patch, or whether
> >>> you do it as a separate followup patch.
> >>>
> >>
> >> I had considered the issue separate; but you're welcome to either write
> >> a patch or squish it into this one, I'm not going to be picky.
> >
> > Yep, it is a separate issue, just related. :-)
> >
> > But since you and Eric agree, I've squashed it in and thus I'm more than
> > glad to thank you very much and announce this patch as applied to my
> > block branch:
> >
> > https://github.com/XanClic/qemu/commits/block
>
> ...well, so much for that. I'll have to unstage it again because it
> breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
> failing to acquire image locks. :-/
>
> I suspect this is because the backing file is opened somewhere and
> trying to open it breaks now with the locking series applied.
I think we can legitimately set force-shared=on for opening the backing
file when testing whether the file exists.
Kevin
pgp5wL9H1o0v9.pgp
Description: PGP signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 2017-05-15 20:41, Max Reitz wrote:
> On 2017-05-12 21:47, John Snow wrote:
>>
>>
>> On 05/12/2017 03:46 PM, Eric Blake wrote:
>>> On 05/12/2017 01:07 PM, Max Reitz wrote:
On 2017-05-11 20:27, John Snow wrote:
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>
> Or, rather, force the open of a backing image if one was specified
> for creation. Using a similar -unsafe option as rebase, allow qemu-img
> to ignore the backing file validation if possible.
>
>>>
> +++ b/block.c
> @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const
> char *fmt,
> // The size for the image must always be specified, with one
> exception:
> // If we are using a backing file, we can obtain the size from there
> size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> -if (size == -1) {
"Hang on, why should this be -1 when the defval is 0? Where does the -1
come from?"
"..."
"Oh, the option exists and is set to -1? Why is that?"
"..."
"Oh, because this function always sets it itself, and because @img_size
is set to (uint64_t)-1."
>>>
>>> I had pretty much the same conversation on my v1 review.
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>>
First, I won't start with how signed integer overflow is
implementation-defined in C because I hope you have thrashed that out
with Eric (I hope that "to thrash out" is a good translation for
"auskaspern" (lit. "to buffoon out").).
>>>
>>> Sounds like a reasonable choice of words, even if I don't speak the
>>> counterpart language to validate your translation.
>>>
>>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>>> wrinkles at you.
>
> We're not really fine because that conversion happens when the result of
> qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
>
>>> I seem to recall that qemu has chosen to use compiler flags and/or
>>> assumptions that we are using 2s-complement arithmetic with sane
>>> behavior (that is, tighter behavior than the bare minimum that C
>>> requires), because it was easier than auditing our code for strict C
>>> compliance on border cases of conversions from unsigned to signed that
>>> trigger undefined behavior. But again, I don't think it affects this
>>> patch (where our conversion is only from signed to unsigned, and that is
>>> well-defined behavior).
>
> Right. Which is why I said I won't even start on it, but of course I
> did. O:-)
>
Second, well, at least we should put -1 as the default value here, then.
>>>
>>> Indeed, now that two reviewers have tripped on it,
>>> qemu_opt_get_size(,,-1) would be nicer.
>>>
Not strictly your fault or something that you need to fix, but it is
just a single line in the vicinity...
Let me know if you want to address this, for now I'll leave a
Reviewed-by: Max Reitz
here if you don't want to.
>>>
>>> I'm okay whether you want to squash that fix into this patch, or whether
>>> you do it as a separate followup patch.
>>>
>>
>> I had considered the issue separate; but you're welcome to either write
>> a patch or squish it into this one, I'm not going to be picky.
>
> Yep, it is a separate issue, just related. :-)
>
> But since you and Eric agree, I've squashed it in and thus I'm more than
> glad to thank you very much and announce this patch as applied to my
> block branch:
>
> https://github.com/XanClic/qemu/commits/block
...well, so much for that. I'll have to unstage it again because it
breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
failing to acquire image locks. :-/
I suspect this is because the backing file is opened somewhere and
trying to open it breaks now with the locking series applied.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 2017-05-12 21:47, John Snow wrote:
>
>
> On 05/12/2017 03:46 PM, Eric Blake wrote:
>> On 05/12/2017 01:07 PM, Max Reitz wrote:
>>> On 2017-05-11 20:27, John Snow wrote:
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
Or, rather, force the open of a backing image if one was specified
for creation. Using a similar -unsafe option as rebase, allow qemu-img
to ignore the backing file validation if possible.
>>
+++ b/block.c
@@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const
char *fmt,
// The size for the image must always be specified, with one
exception:
// If we are using a backing file, we can obtain the size from there
size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
-if (size == -1) {
>>>
>>> "Hang on, why should this be -1 when the defval is 0? Where does the -1
>>> come from?"
>>> "..."
>>> "Oh, the option exists and is set to -1? Why is that?"
>>> "..."
>>> "Oh, because this function always sets it itself, and because @img_size
>>> is set to (uint64_t)-1."
>>
>> I had pretty much the same conversation on my v1 review.
>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>
>>>
>>> First, I won't start with how signed integer overflow is
>>> implementation-defined in C because I hope you have thrashed that out
>>> with Eric (I hope that "to thrash out" is a good translation for
>>> "auskaspern" (lit. "to buffoon out").).
>>
>> Sounds like a reasonable choice of words, even if I don't speak the
>> counterpart language to validate your translation.
>>
>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>> wrinkles at you.
We're not really fine because that conversion happens when the result of
qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
>> I seem to recall that qemu has chosen to use compiler flags and/or
>> assumptions that we are using 2s-complement arithmetic with sane
>> behavior (that is, tighter behavior than the bare minimum that C
>> requires), because it was easier than auditing our code for strict C
>> compliance on border cases of conversions from unsigned to signed that
>> trigger undefined behavior. But again, I don't think it affects this
>> patch (where our conversion is only from signed to unsigned, and that is
>> well-defined behavior).
Right. Which is why I said I won't even start on it, but of course I
did. O:-)
>>> Second, well, at least we should put -1 as the default value here, then.
>>
>> Indeed, now that two reviewers have tripped on it,
>> qemu_opt_get_size(,,-1) would be nicer.
>>
>>>
>>> Not strictly your fault or something that you need to fix, but it is
>>> just a single line in the vicinity...
>>>
>>> Let me know if you want to address this, for now I'll leave a
>>>
>>> Reviewed-by: Max Reitz
>>>
>>> here if you don't want to.
>>
>> I'm okay whether you want to squash that fix into this patch, or whether
>> you do it as a separate followup patch.
>>
>
> I had considered the issue separate; but you're welcome to either write
> a patch or squish it into this one, I'm not going to be picky.
Yep, it is a separate issue, just related. :-)
But since you and Eric agree, I've squashed it in and thus I'm more than
glad to thank you very much and announce this patch as applied to my
block branch:
https://github.com/XanClic/qemu/commits/block
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 05/12/2017 03:46 PM, Eric Blake wrote:
> On 05/12/2017 01:07 PM, Max Reitz wrote:
>> On 2017-05-11 20:27, John Snow wrote:
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>>>
>>> Or, rather, force the open of a backing image if one was specified
>>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>>> to ignore the backing file validation if possible.
>>>
>
>>> +++ b/block.c
>>> @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const
>>> char *fmt,
>>> // The size for the image must always be specified, with one exception:
>>> // If we are using a backing file, we can obtain the size from there
>>> size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
>>> -if (size == -1) {
>>
>> "Hang on, why should this be -1 when the defval is 0? Where does the -1
>> come from?"
>> "..."
>> "Oh, the option exists and is set to -1? Why is that?"
>> "..."
>> "Oh, because this function always sets it itself, and because @img_size
>> is set to (uint64_t)-1."
>
> I had pretty much the same conversation on my v1 review.
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>
>>
>> First, I won't start with how signed integer overflow is
>> implementation-defined in C because I hope you have thrashed that out
>> with Eric (I hope that "to thrash out" is a good translation for
>> "auskaspern" (lit. "to buffoon out").).
>
> Sounds like a reasonable choice of words, even if I don't speak the
> counterpart language to validate your translation.
>
> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
> wrinkles at you.
>
> I seem to recall that qemu has chosen to use compiler flags and/or
> assumptions that we are using 2s-complement arithmetic with sane
> behavior (that is, tighter behavior than the bare minimum that C
> requires), because it was easier than auditing our code for strict C
> compliance on border cases of conversions from unsigned to signed that
> trigger undefined behavior. But again, I don't think it affects this
> patch (where our conversion is only from signed to unsigned, and that is
> well-defined behavior).
>
>
>>
>> Second, well, at least we should put -1 as the default value here, then.
>
> Indeed, now that two reviewers have tripped on it,
> qemu_opt_get_size(,,-1) would be nicer.
>
>>
>> Not strictly your fault or something that you need to fix, but it is
>> just a single line in the vicinity...
>>
>> Let me know if you want to address this, for now I'll leave a
>>
>> Reviewed-by: Max Reitz
>>
>> here if you don't want to.
>
> I'm okay whether you want to squash that fix into this patch, or whether
> you do it as a separate followup patch.
>
I had considered the issue separate; but you're welcome to either write
a patch or squish it into this one, I'm not going to be picky.
--js
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 05/12/2017 01:07 PM, Max Reitz wrote:
> On 2017-05-11 20:27, John Snow wrote:
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>>
>> Or, rather, force the open of a backing image if one was specified
>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>> to ignore the backing file validation if possible.
>>
>> +++ b/block.c
>> @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const
>> char *fmt,
>> // The size for the image must always be specified, with one exception:
>> // If we are using a backing file, we can obtain the size from there
>> size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
>> -if (size == -1) {
>
> "Hang on, why should this be -1 when the defval is 0? Where does the -1
> come from?"
> "..."
> "Oh, the option exists and is set to -1? Why is that?"
> "..."
> "Oh, because this function always sets it itself, and because @img_size
> is set to (uint64_t)-1."
I had pretty much the same conversation on my v1 review.
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>
> First, I won't start with how signed integer overflow is
> implementation-defined in C because I hope you have thrashed that out
> with Eric (I hope that "to thrash out" is a good translation for
> "auskaspern" (lit. "to buffoon out").).
Sounds like a reasonable choice of words, even if I don't speak the
counterpart language to validate your translation.
(uint64_t)-1 is well-defined in C (so I think we're just fine here). But
(int64_t)UINT64_MAX is where signed integer overflow does indeed throw
wrinkles at you.
I seem to recall that qemu has chosen to use compiler flags and/or
assumptions that we are using 2s-complement arithmetic with sane
behavior (that is, tighter behavior than the bare minimum that C
requires), because it was easier than auditing our code for strict C
compliance on border cases of conversions from unsigned to signed that
trigger undefined behavior. But again, I don't think it affects this
patch (where our conversion is only from signed to unsigned, and that is
well-defined behavior).
>
> Second, well, at least we should put -1 as the default value here, then.
Indeed, now that two reviewers have tripped on it,
qemu_opt_get_size(,,-1) would be nicer.
>
> Not strictly your fault or something that you need to fix, but it is
> just a single line in the vicinity...
>
> Let me know if you want to address this, for now I'll leave a
>
> Reviewed-by: Max Reitz
>
> here if you don't want to.
I'm okay whether you want to squash that fix into this patch, or whether
you do it as a separate followup patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
On 2017-05-11 20:27, John Snow wrote:
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>
> Or, rather, force the open of a backing image if one was specified
> for creation. Using a similar -unsafe option as rebase, allow qemu-img
> to ignore the backing file validation if possible.
>
> It may not always be possible, as in the existing case when a filesize
> for the new image was not specified.
>
> This is accomplished by shifting around the conditionals in
> bdrv_img_create, such that a backing file is always opened unless we
> provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
> when -u is provided to create.
>
> Sorry for the heinous looking diffstat, but it's mostly whitespace.
>
> Reported-by: Yi Sun
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
> ---
>
> v4: Actually do the things Eric told me to.
> v3: Rebased
> v2: Rebased for 2.10
> Corrected some of my less cromulent grammar
>
>
> block.c| 73
> +++---
> qemu-img-cmds.hx | 4 +--
> qemu-img.c | 16 ++
> tests/qemu-iotests/082 | 4 +--
> tests/qemu-iotests/082.out | 4 +--
> 5 files changed, 54 insertions(+), 47 deletions(-)
>
> diff --git a/block.c b/block.c
> index a45b9b5..3c3df54 100644
> --- a/block.c
> +++ b/block.c
> @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const char
> *fmt,
> // The size for the image must always be specified, with one exception:
> // If we are using a backing file, we can obtain the size from there
> size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> -if (size == -1) {
"Hang on, why should this be -1 when the defval is 0? Where does the -1
come from?"
"..."
"Oh, the option exists and is set to -1? Why is that?"
"..."
"Oh, because this function always sets it itself, and because @img_size
is set to (uint64_t)-1."
First, I won't start with how signed integer overflow is
implementation-defined in C because I hope you have thrashed that out
with Eric (I hope that "to thrash out" is a good translation for
"auskaspern" (lit. "to buffoon out").).
Second, well, at least we should put -1 as the default value here, then.
Not strictly your fault or something that you need to fix, but it is
just a single line in the vicinity...
Let me know if you want to address this, for now I'll leave a
Reviewed-by: Max Reitz
here if you don't want to.
Max
> -if (backing_file) {
> -BlockDriverState *bs;
> -char *full_backing = g_new0(char, PATH_MAX);
> -int64_t size;
> -int back_flags;
> -QDict *backing_options = NULL;
> -
> -bdrv_get_full_backing_filename_from_filename(filename,
> backing_file,
> - full_backing,
> PATH_MAX,
> - &local_err);
> -if (local_err) {
> -g_free(full_backing);
> -goto out;
> -}
> -
> -/* backing files always opened read-only */
> -back_flags = flags;
> -back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> BDRV_O_NO_BACKING);
> -
> -if (backing_fmt) {
> -backing_options = qdict_new();
> -qdict_put_str(backing_options, "driver", backing_fmt);
> -}
> -
> -bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
> - &local_err);
> +if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
> +BlockDriverState *bs;
> +char *full_backing = g_new0(char, PATH_MAX);
> +int back_flags;
> +QDict *backing_options = NULL;
> +
> +bdrv_get_full_backing_filename_from_filename(filename, backing_file,
> + full_backing, PATH_MAX,
> + &local_err);
> +if (local_err) {
> g_free(full_backing);
> -if (!bs) {
> -goto out;
> -}
> +goto out;
> +}
> +
> +/* backing files always opened read-only */
> +back_flags = flags;
> +back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +
> +if (backing_fmt) {
> +backing_options = qdict_new();
> +qdict_put_str(backing_options, "driver", backing_fmt);
> +}
> +
> +bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
> + &local_err);
> +g_free(full_backing);
> +if (!bs) {
> +goto out;
> +}
> +
> +if (size == -1) {
> size = bdrv_getlength(bs);
> if (size < 0) {
> error_setg_errno(errp, -size, "Could not get size of '%s'",
> @@ -4313,14 +4313,15 @@ void bdrv_img_create(const char *filename, const char
>
[Qemu-block] [PATCH v4] qemu-img: Check for backing image if specified during create
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
Or, rather, force the open of a backing image if one was specified
for creation. Using a similar -unsafe option as rebase, allow qemu-img
to ignore the backing file validation if possible.
It may not always be possible, as in the existing case when a filesize
for the new image was not specified.
This is accomplished by shifting around the conditionals in
bdrv_img_create, such that a backing file is always opened unless we
provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
when -u is provided to create.
Sorry for the heinous looking diffstat, but it's mostly whitespace.
Reported-by: Yi Sun
Signed-off-by: John Snow
Reviewed-by: Eric Blake
---
v4: Actually do the things Eric told me to.
v3: Rebased
v2: Rebased for 2.10
Corrected some of my less cromulent grammar
block.c| 73 +++---
qemu-img-cmds.hx | 4 +--
qemu-img.c | 16 ++
tests/qemu-iotests/082 | 4 +--
tests/qemu-iotests/082.out | 4 +--
5 files changed, 54 insertions(+), 47 deletions(-)
diff --git a/block.c b/block.c
index a45b9b5..3c3df54 100644
--- a/block.c
+++ b/block.c
@@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const char
*fmt,
// The size for the image must always be specified, with one exception:
// If we are using a backing file, we can obtain the size from there
size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
-if (size == -1) {
-if (backing_file) {
-BlockDriverState *bs;
-char *full_backing = g_new0(char, PATH_MAX);
-int64_t size;
-int back_flags;
-QDict *backing_options = NULL;
-
-bdrv_get_full_backing_filename_from_filename(filename,
backing_file,
- full_backing,
PATH_MAX,
- &local_err);
-if (local_err) {
-g_free(full_backing);
-goto out;
-}
-
-/* backing files always opened read-only */
-back_flags = flags;
-back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
-
-if (backing_fmt) {
-backing_options = qdict_new();
-qdict_put_str(backing_options, "driver", backing_fmt);
-}
-
-bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
- &local_err);
+if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
+BlockDriverState *bs;
+char *full_backing = g_new0(char, PATH_MAX);
+int back_flags;
+QDict *backing_options = NULL;
+
+bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+ full_backing, PATH_MAX,
+ &local_err);
+if (local_err) {
g_free(full_backing);
-if (!bs) {
-goto out;
-}
+goto out;
+}
+
+/* backing files always opened read-only */
+back_flags = flags;
+back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+
+if (backing_fmt) {
+backing_options = qdict_new();
+qdict_put_str(backing_options, "driver", backing_fmt);
+}
+
+bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
+ &local_err);
+g_free(full_backing);
+if (!bs) {
+goto out;
+}
+
+if (size == -1) {
size = bdrv_getlength(bs);
if (size < 0) {
error_setg_errno(errp, -size, "Could not get size of '%s'",
@@ -4313,14 +4313,15 @@ void bdrv_img_create(const char *filename, const char
*fmt,
bdrv_unref(bs);
goto out;
}
-
qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
-
-bdrv_unref(bs);
-} else {
-error_setg(errp, "Image creation needs a size parameter");
-goto out;
}
+
+bdrv_unref(bs);
+}
+
+if (size == -1) {
+error_setg(errp, "Image creation needs a size parameter");
+goto out;
}
if (!quiet) {
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index bf4ce59..1d230c6 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,9 +22,9 @@ STEXI
ETEXI
DEF("create", img_create,
-"create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F
backing_fmt] [-o options] filename [size]")
+"create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F
backing_fmt] [-u] [-o options] filename [size]")
STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b
@var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{file
