[Qemu-devel] [PATCH for 2.10 0/2] block: simple changes

[Qemu-devel] [PATCH for 2.10 0/2] block: simple changes

[Denis V. Lunev]

First patch here is one we have agreed on during the initial discussion
about truncate() in parallels format driver, another one is simple
coding style change spotted by my student.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>

Denis V. Lunev (1):
  block: assert no image modification under BDRV_O_INACTIVE

Klim Kireev (1):
  block: fix obvious coding style mitakes in block_int.h

 block.c                   | 2 ++
 include/block/block_int.h | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] block: fix obvious coding style mitakes in block_int.h

[Denis V. Lunev]

From: Klim Kireev <proffk@virtuozzo.mipt.ru>

Signed-off-by: Klim Kireev <proffk@virtuozzo.mipt.ru>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 59400bd..bf18c5f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -252,7 +252,7 @@ struct BlockDriver {
      * Returns 0 for completed check, -errno for internal errors.
      * The check results are stored in result.
      */
-    int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result,
+    int (*bdrv_check)(BlockDriverState *bs, BdrvCheckResult *result,
         BdrvCheckMode fix);
 
     int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
@@ -454,13 +454,13 @@ struct BdrvChildRole {
     /* Returns a name that is supposedly more useful for human users than the
      * node name for identifying the node in question (in particular, a BB
      * name), or NULL if the parent can't provide a better name. */
-    const char* (*get_name)(BdrvChild *child);
+    const char *(*get_name)(BdrvChild *child);
 
     /* Returns a malloced string that describes the parent of the child for a
      * human reader. This could be a node-name, BlockBackend name, qdev ID or
      * QOM path of the device owning the BlockBackend, job type and ID etc. The
      * caller is responsible for freeing the memory. */
-    char* (*get_parent_desc)(BdrvChild *child);
+    char *(*get_parent_desc)(BdrvChild *child);
 
     /*
      * If this pair of functions is implemented, the parent doesn't issue new
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] block: fix obvious coding style mitakes in block_int.h

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On 04/05/2017 10:18 AM, Denis V. Lunev wrote:
> From: Klim Kireev <proffk@virtuozzo.mipt.ru>

s/mitakes/mistakes/ in the subject

A blank commit message body doesn't help - the subject says what was
changed but not why (for example, output of checkpatch.pl without the
patch applied); and details may help in grepping past history when
looking for the change or even to justify that the change is not just churn.

> 
> Signed-off-by: Klim Kireev <proffk@virtuozzo.mipt.ru>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block_int.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

At any rate, all of the changes (the fact that we prefer binding the
pointer operator * to the name on the right rather than the type on the
left) look sane.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block: fix obvious coding style mitakes in block_int.h

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On 05.04.2017 17:35, Eric Blake wrote:
> On 04/05/2017 10:18 AM, Denis V. Lunev wrote:
>> From: Klim Kireev <proffk@virtuozzo.mipt.ru>
> 
> s/mitakes/mistakes/ in the subject
> 
> A blank commit message body doesn't help - the subject says what was
> changed but not why (for example, output of checkpatch.pl without the
> patch applied); and details may help in grepping past history when
> looking for the change or even to justify that the change is not just churn.
> 
>>
>> Signed-off-by: Klim Kireev <proffk@virtuozzo.mipt.ru>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/block/block_int.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
> 
> At any rate, all of the changes (the fact that we prefer binding the
> pointer operator * to the name on the right rather than the type on the
> left) look sane.

I know someone who disagrees but that someone also knows that I will
support and accept this kind of patches gladly. O:-)

Max

> Reviewed-by: Eric Blake <eblake@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH 2/2] block: assert no image modification under BDRV_O_INACTIVE

[Denis V. Lunev]

As long as BDRV_O_INACTIVE is set, the image file is only opened so we
have a file descriptor for it. We're definitely not supposed to modify
the image, it's still owned by the migration source.

This commit is an addition to 09e0c771 but the assert() is added to
bdrv_truncate().

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index 927ba89..9273741 100644
--- a/block.c
+++ b/block.c
@@ -3279,6 +3279,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
     if (bs->read_only)
         return -EACCES;
 
+    assert(!(bs->open_flags & BDRV_O_INACTIVE));
+
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] block: assert no image modification under BDRV_O_INACTIVE

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On 04/05/2017 10:18 AM, Denis V. Lunev wrote:
> As long as BDRV_O_INACTIVE is set, the image file is only opened so we
> have a file descriptor for it. We're definitely not supposed to modify
> the image, it's still owned by the migration source.
> 
> This commit is an addition to 09e0c771 but the assert() is added to
> bdrv_truncate().
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)

I don't feel comfortable adding an assert this late in the 2.9 phase,
but think it makes perfect sense as a 2.10 addition.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block.c b/block.c
> index 927ba89..9273741 100644
> --- a/block.c
> +++ b/block.c
> @@ -3279,6 +3279,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
>      if (bs->read_only)
>          return -EACCES;
>  
> +    assert(!(bs->open_flags & BDRV_O_INACTIVE));
> +
>      ret = drv->bdrv_truncate(bs, offset);
>      if (ret == 0) {
>          ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] block: assert no image modification under BDRV_O_INACTIVE

[Denis V. Lunev]

On 04/05/2017 06:37 PM, Eric Blake wrote:
> On 04/05/2017 10:18 AM, Denis V. Lunev wrote:
>> As long as BDRV_O_INACTIVE is set, the image file is only opened so we
>> have a file descriptor for it. We're definitely not supposed to modify
>> the image, it's still owned by the migration source.
>>
>> This commit is an addition to 09e0c771 but the assert() is added to
>> bdrv_truncate().
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 2 ++
>>  1 file changed, 2 insertions(+)
> I don't feel comfortable adding an assert this late in the 2.9 phase,
> but think it makes perfect sense as a 2.10 addition.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
no-no, this is for 2.10 for sure. No way for 2.9. I have stated that in
cover letter :(
Sorry for inconvenience.

Den

Re: [Qemu-devel] [PATCH 2/2] block: assert no image modification under BDRV_O_INACTIVE

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On 05.04.2017 17:18, Denis V. Lunev wrote:
> As long as BDRV_O_INACTIVE is set, the image file is only opened so we
> have a file descriptor for it. We're definitely not supposed to modify
> the image, it's still owned by the migration source.
> 
> This commit is an addition to 09e0c771 but the assert() is added to
> bdrv_truncate().
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)

Thanks to the new op blockers I'm not sure this is really necessary
anymore (that assertion should cover this, once it's reinstated, that
is...), but it won't hurt, I guess.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for 2.10 0/2] block: simple changes

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On 05.04.2017 17:18, Denis V. Lunev wrote:
> First patch here is one we have agreed on during the initial discussion
> about truncate() in parallels format driver, another one is simple
> coding style change spotted by my student.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> 
> Denis V. Lunev (1):
>   block: assert no image modification under BDRV_O_INACTIVE
> 
> Klim Kireev (1):
>   block: fix obvious coding style mitakes in block_int.h
> 
>  block.c                   | 2 ++
>  include/block/block_int.h | 6 +++---
>  2 files changed, 5 insertions(+), 3 deletions(-)

Thanks, fixed the subject of the first patch and applied to my
block-next branch:

https://github.com/XanClic/qemu/commits/block-next

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]