diff mbox

[PULL,43/46] block: Assertions for write permissions

Message ID 1488314205-16264-44-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Feb. 28, 2017, 8:36 p.m. UTC 
This adds assertions that ensure that the necessary write permissions
have been granted before someone attempts to write to a node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Richard W.M. Jones April 6, 2017, 8:59 p.m. UTC | #1 
On Tue, Feb 28, 2017 at 09:36:42PM +0100, Kevin Wolf wrote:
> This adds assertions that ensure that the necessary write permissions
> have been granted before someone attempts to write to a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 2592ca1..4c79745 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -945,6 +945,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>      size_t skip_bytes;
>      int ret;
>  
> +    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));


This assertion is thrown in the libguestfs test suite.  I filed a bug
about it against the Fedora package:

https://lists.gnu.org/archive/html/qemu-block/2017-02/msg01305.html

Rich.
Eric Blake April 6, 2017, 9:03 p.m. UTC | #2 
On 04/06/2017 03:59 PM, Richard W.M. Jones wrote:
> On Tue, Feb 28, 2017 at 09:36:42PM +0100, Kevin Wolf wrote:
>> This adds assertions that ensure that the necessary write permissions
>> have been granted before someone attempts to write to a node.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Acked-by: Fam Zheng <famz@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/io.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 2592ca1..4c79745 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -945,6 +945,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>>      size_t skip_bytes;
>>      int ret;
>>  
>> +    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
> 
> 
> This assertion is thrown in the libguestfs test suite.  I filed a bug
> about it against the Fedora package:
> 
> https://lists.gnu.org/archive/html/qemu-block/2017-02/msg01305.html

Wrong URL?
Richard W.M. Jones April 6, 2017, 9:15 p.m. UTC | #3 
On Thu, Apr 06, 2017 at 04:03:28PM -0500, Eric Blake wrote:
> On 04/06/2017 03:59 PM, Richard W.M. Jones wrote:
> > On Tue, Feb 28, 2017 at 09:36:42PM +0100, Kevin Wolf wrote:
> >> This adds assertions that ensure that the necessary write permissions
> >> have been granted before someone attempts to write to a node.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> Acked-by: Fam Zheng <famz@redhat.com>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  block/io.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/block/io.c b/block/io.c
> >> index 2592ca1..4c79745 100644
> >> --- a/block/io.c
> >> +++ b/block/io.c
> >> @@ -945,6 +945,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
> >>      size_t skip_bytes;
> >>      int ret;
> >>  
> >> +    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
> > 
> > 
> > This assertion is thrown in the libguestfs test suite.  I filed a bug
> > about it against the Fedora package:
> > 
> > https://lists.gnu.org/archive/html/qemu-block/2017-02/msg01305.html
> 
> Wrong URL?

Ooops, the right link is:

https://bugzilla.redhat.com/show_bug.cgi?id=1439922

There is also a minimal reproducer in comment 2.

Rich.
Eric Blake April 6, 2017, 9:23 p.m. UTC | #4 
On 04/06/2017 04:15 PM, Richard W.M. Jones wrote:

>>>> +++ b/block/io.c
>>>> @@ -945,6 +945,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>>>>      size_t skip_bytes;
>>>>      int ret;
>>>>  
>>>> +    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
>>>

> https://bugzilla.redhat.com/show_bug.cgi?id=1439922
> 
> There is also a minimal reproducer in comment 2.

If it helps, backtrace shows:

#4  0x0000555555c09f10 in bdrv_co_do_copy_on_readv
(child=0x555556990180, offset=0, bytes=512, qiov=0x7fffffffd0c0) at
block/io.c:948
#5  0x0000555555c0a33d in bdrv_aligned_preadv (child=0x555556990180,
req=0x7fffb6dffec0, offset=0, bytes=512, align=1, qiov=0x7fffffffd0c0,
flags=1)
    at block/io.c:1058
#6  0x0000555555c0a8c3 in bdrv_co_preadv (child=0x555556990180,
offset=0, bytes=512, qiov=0x7fffffffd0c0, flags=BDRV_REQ_COPY_ON_READ)
at block/io.c:1166
#7  0x0000555555bf7ccf in blk_co_preadv (blk=0x555556a2bc20, offset=0,
bytes=512, qiov=0x7fffffffd0c0, flags=0) at block/block-backend.c:927
#8  0x0000555555bf7e19 in blk_read_entry (opaque=0x7fffffffd0e0)
    at block/block-backend.c:974
#9  0x0000555555cc3015 in coroutine_trampoline (i0=1485566720, i1=21845)
    at util/coroutine-ucontext.c:79
diff mbox

Patch

diff --git a/block/io.c b/block/io.c
index 2592ca1..4c79745 100644
--- a/block/io.c
+++ b/block/io.c
@@ -945,6 +945,8 @@  static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
     size_t skip_bytes;
     int ret;
 
+    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
+
     /* Cover entire cluster so no additional backing file I/O is required when
      * allocating cluster in the image file.
      */
@@ -1336,6 +1338,7 @@  static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     assert(!waited || !req->serialising);
     assert(req->overlap_offset <= offset);
     assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
+    assert(child->perm & BLK_PERM_WRITE);
 
     ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);