We already have a lot of bdrv_getlength() fixes in -rc2; so I think
this is still okay for -rc3.

v1 was here (with a typo'd subject line):
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html

Since v1:
- patch 1: fix error message capitalization (Kevin, R-b kept)
- fix locking bug in original patch 2 (Kevin)
- split original patch 2 into two parts: signature update, and
added error checking (Kevin)
- check for unlikely integer overflow before bdrv_truncate (Jeff)

001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
002/5:[down] 'qcow: Change signature of get_cluster_offset()'
003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'

Eric Blake (5):
  vpc: Check failure of bdrv_getlength()
  qcow: Change signature of get_cluster_offset()
  qcow: Check failure of bdrv_getlength() and bdrv_truncate()
  qcow2: Drop debugging dump_refcounts()
  qcow2: Check failure of bdrv_getlength()

 block/qcow.c  | 165 ++++++++++++++++++++++++++++++++++------------------------
 block/qcow2.c |  26 ++-------
 block/vpc.c   |   9 +++-
 3 files changed, 110 insertions(+), 90 deletions(-)

-- 
2.13.4

Am 09.08.2017 um 22:38 hat Eric Blake geschrieben:
> We already have a lot of bdrv_getlength() fixes in -rc2; so I think
> this is still okay for -rc3.
> 
> v1 was here (with a typo'd subject line):
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html
> 
> Since v1:
> - patch 1: fix error message capitalization (Kevin, R-b kept)
> - fix locking bug in original patch 2 (Kevin)
> - split original patch 2 into two parts: signature update, and
> added error checking (Kevin)
> - check for unlikely integer overflow before bdrv_truncate (Jeff)
> 
> 001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
> 002/5:[down] 'qcow: Change signature of get_cluster_offset()'
> 003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
> 004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
> 005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'

Looks good to me, but as the bug is far from being critical, I'd rather
apply the more complex qcow1 patches only to block-next. The vpc and
qcow2 parts seems a lot less risky, so 2.10 should be okay for them.

What do you think?

Kevin

On 08/10/2017 08:02 AM, Kevin Wolf wrote:
> Am 09.08.2017 um 22:38 hat Eric Blake geschrieben:
>> We already have a lot of bdrv_getlength() fixes in -rc2; so I think
>> this is still okay for -rc3.
>>
>> v1 was here (with a typo'd subject line):
>> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html
>>
>> Since v1:
>> - patch 1: fix error message capitalization (Kevin, R-b kept)
>> - fix locking bug in original patch 2 (Kevin)
>> - split original patch 2 into two parts: signature update, and
>> added error checking (Kevin)
>> - check for unlikely integer overflow before bdrv_truncate (Jeff)
>>
>> 001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
>> 002/5:[down] 'qcow: Change signature of get_cluster_offset()'
>> 003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
>> 004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
>> 005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'
> 
> Looks good to me, but as the bug is far from being critical, I'd rather
> apply the more complex qcow1 patches only to block-next. The vpc and
> qcow2 parts seems a lot less risky, so 2.10 should be okay for them.
> 
> What do you think?

The argument for NOT doing the qcow changes (patches 2 and 3): the only
place where we are not checking for failures is part of
get_cluster_offset() - but in all likelihood, if we were unable to
determine or change the length of the backing file, we will have nearby
problems that will ultimately cause failure soon enough.  Furthermore,
it's not a regression (we've had several releases with the problem), and
qcow is not a good format (it's painfully slow, and we strongly
recommend qcow2 instead) - so no one will be hitting any actual bugs in
practice.

I'll trust your judgment as maintainer, so taking just 1, 4, and 5 in
2.10 is fine.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Am 10.08.2017 um 17:08 hat Eric Blake geschrieben:
> On 08/10/2017 08:02 AM, Kevin Wolf wrote:
> > Am 09.08.2017 um 22:38 hat Eric Blake geschrieben:
> >> We already have a lot of bdrv_getlength() fixes in -rc2; so I think
> >> this is still okay for -rc3.
> >>
> >> v1 was here (with a typo'd subject line):
> >> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html
> >>
> >> Since v1:
> >> - patch 1: fix error message capitalization (Kevin, R-b kept)
> >> - fix locking bug in original patch 2 (Kevin)
> >> - split original patch 2 into two parts: signature update, and
> >> added error checking (Kevin)
> >> - check for unlikely integer overflow before bdrv_truncate (Jeff)
> >>
> >> 001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
> >> 002/5:[down] 'qcow: Change signature of get_cluster_offset()'
> >> 003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
> >> 004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
> >> 005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'
> > 
> > Looks good to me, but as the bug is far from being critical, I'd rather
> > apply the more complex qcow1 patches only to block-next. The vpc and
> > qcow2 parts seems a lot less risky, so 2.10 should be okay for them.
> > 
> > What do you think?
> 
> The argument for NOT doing the qcow changes (patches 2 and 3): the only
> place where we are not checking for failures is part of
> get_cluster_offset() - but in all likelihood, if we were unable to
> determine or change the length of the backing file, we will have nearby
> problems that will ultimately cause failure soon enough.  Furthermore,
> it's not a regression (we've had several releases with the problem), and
> qcow is not a good format (it's painfully slow, and we strongly
> recommend qcow2 instead) - so no one will be hitting any actual bugs in
> practice.
> 
> I'll trust your judgment as maintainer, so taking just 1, 4, and 5 in
> 2.10 is fine.

Thanks, applied the patches to block and block-next, respectively.

Kevin