mbox series

[v9,0/7] coroutines: generate wrapper code

Message ID 20200924185414.28642-1-vsementsov@virtuozzo.com
Headers show
Series coroutines: generate wrapper code | expand

Message

Vladimir Sementsov-Ogievskiy Sept. 24, 2020, 6:54 p.m. UTC
Hi all!

The aim of the series is to reduce code-duplication and writing
parameters structure-packing by hand around coroutine function wrappers.

Benefits:
 - no code duplication
 - less indirection

v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving from his git.
01: add Philippe's, Stefan's r-bs
02: - add Philippe's, Stefan's r-bs
    - commit message tweaks stolen from Eric's git :)
03: add Philippe's, Stefan's r-bs
04: - wording/grammar by Eric (partly, stolen from repo)
    - ref new file in docs/devel/index.rst
    - use 644 rights and recommended header for python script
    - call gen_header() once
    - rename gen_wrappers_file to gen_wrappers
05: add Stefan's r-b
06: add Philippe's, Stefan's r-bs
07: Stefan's r-b

Vladimir Sementsov-Ogievskiy (7):
  block: return error-code from bdrv_invalidate_cache
  block/io: refactor coroutine wrappers
  block: declare some coroutine functions in block/coroutines.h
  scripts: add block-coroutine-wrapper.py
  block: generate coroutine-wrapper code
  block: drop bdrv_prwv
  block/io: refactor save/load vmstate

 docs/devel/block-coroutine-wrapper.rst |  54 ++++
 docs/devel/index.rst                   |   1 +
 block/block-gen.h                      |  49 ++++
 block/coroutines.h                     |  65 +++++
 include/block/block.h                  |  34 ++-
 block.c                                |  97 +------
 block/io.c                             | 337 ++++---------------------
 tests/test-bdrv-drain.c                |   2 +-
 block/meson.build                      |   8 +
 scripts/block-coroutine-wrapper.py     | 188 ++++++++++++++
 10 files changed, 454 insertions(+), 381 deletions(-)
 create mode 100644 docs/devel/block-coroutine-wrapper.rst
 create mode 100644 block/block-gen.h
 create mode 100644 block/coroutines.h
 create mode 100644 scripts/block-coroutine-wrapper.py

Comments

Eric Blake Sept. 24, 2020, 7 p.m. UTC | #1
On 9/24/20 1:54 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!

> 

> The aim of the series is to reduce code-duplication and writing

> parameters structure-packing by hand around coroutine function wrappers.

> 

> Benefits:

>   - no code duplication

>   - less indirection

> 

> v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving from his git.

> 01: add Philippe's, Stefan's r-bs

> 02: - add Philippe's, Stefan's r-bs

>      - commit message tweaks stolen from Eric's git :)


LOL: "stolen" makes it sound like a crime was committed ;)  I find it to 
be one of the joys of open source when my ideas show up in someone 
else's work when properly attributed, as you did here.  [Maybe the 
recent push towards a conscious language initiative has let me hone in 
on something that turned out humorous to me, but might not be as obvious 
to someone less fluent in English idioms]

At any rate, I'm glad my rebase efforts helped.

> 03: add Philippe's, Stefan's r-bs

> 04: - wording/grammar by Eric (partly, stolen from repo)

>      - ref new file in docs/devel/index.rst

>      - use 644 rights and recommended header for python script

>      - call gen_header() once

>      - rename gen_wrappers_file to gen_wrappers

> 05: add Stefan's r-b

> 06: add Philippe's, Stefan's r-bs

> 07: Stefan's r-b

> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
no-reply@patchew.org Sept. 24, 2020, 8:32 p.m. UTC | #2
Patchew URL: https://patchew.org/QEMU/20200924185414.28642-1-vsementsov@virtuozzo.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

C linker for the host machine: cc ld.bfd 2.27-43
Host machine cpu family: x86_64
Host machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128)
Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp-uncond.c.inc'.
make: *** [block/block-gen.c.stamp] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 709, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5', '-u', '1001', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-rctf_xsm/src/docker-src.2020-09-24-16.29.53.14429:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rctf_xsm/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    3m4.047s
user    0m21.648s


The full log is available at
http://patchew.org/logs/20200924185414.28642-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Vladimir Sementsov-Ogievskiy Sept. 25, 2020, 8:04 a.m. UTC | #3
24.09.2020 23:32, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200924185414.28642-1-vsementsov@virtuozzo.com/

> 

> 

> 

> Hi,

> 

> This series failed the docker-quick@centos7 build test. Please find the testing commands and

> their output below. If you have Docker installed, you can probably reproduce it

> locally.

> 

> === TEST SCRIPT BEGIN ===

> #!/bin/bash

> make docker-image-centos7 V=1 NETWORK=1

> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1

> === TEST SCRIPT END ===

> 

> C linker for the host machine: cc ld.bfd 2.27-43

> Host machine cpu family: x86_64

> Host machine cpu: x86_64

> ../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.

> Program sh found: YES

> Program python3 found: YES (/usr/bin/python3)

> Configuring ninjatool using configuration

> ---

>      return codecs.ascii_decode(input, self.errors)[0]

> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128)

> Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp-uncond.c.inc'.

> make: *** [block/block-gen.c.stamp] Error 1

> make: *** Waiting for unfinished jobs....

> Traceback (most recent call last):

>    File "./tests/docker/docker.py", line 709, in <module>

> ---

>      raise CalledProcessError(retcode, cmd)

> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5', '-u', '1001', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-rctf_xsm/src/docker-src.2020-09-24-16.29.53.14429:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.

> filter=--filter=label=com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5

> make[1]: *** [docker-run] Error 1

> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rctf_xsm/src'

> make: *** [docker-run-test-quick@centos7] Error 2

> 

> real    3m4.047s

> user    0m21.648s

> 

> 

> The full log is available at

> http://patchew.org/logs/20200924185414.28642-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.

> ---

> Email generated automatically by Patchew [https://patchew.org/].

> Please send your feedback to patchew-devel@redhat.com

> 


Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp.c.inc'.
Traceback (most recent call last):
   File "/tmp/qemu-test/src/block/../scripts/block-coroutine-wrapper.py", line 187, in <module>
     f_out.write(gen_wrappers(f_in.read()))
   File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode
     return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128)


Interesting:

[root@kvm up-coroutine-wrapper]# grep --color='auto' -P -n '[^\x00-\x7F]' include/block/block.h
307:     * Child from which to read all data that isn’t allocated in the
                                                      ^

The file really contains one non-ascii symbol. I think it worth a separate patch. Still, it shouldn't break build process. On my system it works as is, probably unicode is default for me.

Aha, from "open" specification:

    if encoding is not specified the encoding used is platform dependent: locale.getpreferredencoding(False) is called to get the current locale encoding.



Is it ok, that utf-8 is not default on test system?

So, possible solutions are:

1. Enforce utf-8 io in scripts/block-coroutine-wrapper.py (patch 4)
2. Drop non-ascii quotation mark from block.h
3. Fix the test system default to be utf-8

Do we want them all?

-- 
Best regards,
Vladimir
Eric Blake Sept. 25, 2020, 12:57 p.m. UTC | #4
On 9/25/20 3:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2020 23:32, no-reply@patchew.org wrote:

>> Patchew URL: 

>> https://patchew.org/QEMU/20200924185414.28642-1-vsementsov@virtuozzo.com/

>>


>> Program python3 found: YES (/usr/bin/python3)

>> Configuring ninjatool using configuration

>> ---

>>      return codecs.ascii_decode(input, self.errors)[0]

>> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 

>> 11406: ordinal not in range(128)


> Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp.c.inc'.

> Traceback (most recent call last):

>    File 

> "/tmp/qemu-test/src/block/../scripts/block-coroutine-wrapper.py", line 

> 187, in <module>

>      f_out.write(gen_wrappers(f_in.read()))

>    File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode

>      return codecs.ascii_decode(input, self.errors)[0]

> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 

> 11406: ordinal not in range(128)

> 

> 

> Interesting:

> 

> [root@kvm up-coroutine-wrapper]# grep --color='auto' -P -n 

> '[^\x00-\x7F]' include/block/block.h

> 307:     * Child from which to read all data that isn’t allocated in the

>                                                       ^

> 

> The file really contains one non-ascii symbol. I think it worth a 

> separate patch. Still, it shouldn't break build process. On my system it 

> works as is, probably unicode is default for me.


Python 3 has had an interesting history when it comes to 8-bit cleanness 
by default.  Which means we DO have to be explicit about utf8.

> 

> Aha, from "open" specification:

> 

>     if encoding is not specified the encoding used is platform 

> dependent: locale.getpreferredencoding(False) is called to get the 

> current locale encoding.

> 

> 

> 

> Is it ok, that utf-8 is not default on test system?


It's intentional.

> 

> So, possible solutions are:

> 

> 1. Enforce utf-8 io in scripts/block-coroutine-wrapper.py (patch 4)


Yes, we should do that regardless (we do it in our other python scripts).

> 2. Drop non-ascii quotation mark from block.h


Yes, we should do that as well (it's only in a comment, but it is 
inconsistent).

> 3. Fix the test system default to be utf-8


No.  That one we want to keep where it is, because it helps us flush out 
these sorts of issues.

> 

> Do we want them all?

> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Stefan Hajnoczi Sept. 25, 2020, 4:02 p.m. UTC | #5
On Thu, Sep 24, 2020 at 09:54:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> The aim of the series is to reduce code-duplication and writing
> parameters structure-packing by hand around coroutine function wrappers.
> 
> Benefits:
>  - no code duplication
>  - less indirection
> 
> v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving from his git.

Thanks, applied to my block tree with the encoding='utf-8' and
spelling/grammar fixes:
https://github.com/stefanha/qemu/commits/block

Stefan