Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200921162346.188997-1-stefanha@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200921162346.188997-1-stefanha@redhat.com
Subject: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200918103430.297167-1-thuth@redhat.com -> patchew/20200918103430.297167-1-thuth@redhat.com
Switched to a new branch 'test'
25ca702 qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

=== OUTPUT BEGIN ===
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#2968: FILE: include/qemu/atomic.h:152:
+#define qemu_atomic_rcu_read__nocheck(ptr, valptr)      \
     __atomic_load(ptr, valptr, __ATOMIC_RELAXED);       \
     smp_read_barrier_depends();

ERROR: space required before that '*' (ctx:VxB)
#3123: FILE: include/qemu/atomic.h:347:
+#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
                                                                  ^

ERROR: Use of volatile is usually wrong, please add a comment
#3123: FILE: include/qemu/atomic.h:347:
+#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))

ERROR: space required before that '*' (ctx:VxB)
#3125: FILE: include/qemu/atomic.h:349:
+    ((*(__typeof__(*(p)) volatile*) (p)) = (i))
                                  ^

ERROR: Use of volatile is usually wrong, please add a comment
#3125: FILE: include/qemu/atomic.h:349:
+    ((*(__typeof__(*(p)) volatile*) (p)) = (i))

ERROR: space required after that ',' (ctx:VxV)
#3130: FILE: include/qemu/atomic.h:352:
+#define qemu_atomic_set(ptr, i)     qemu_atomic_set__nocheck(ptr,i)
                                                                 ^

ERROR: memory barrier without comment
#3205: FILE: include/qemu/atomic.h:410:
+#define qemu_atomic_xchg(ptr, i) (smp_mb(), __sync_lock_test_and_set(ptr, i))

WARNING: Block comments use a leading /* on a separate line
#3280: FILE: include/qemu/atomic.h:462:
+/* qemu_atomic_mb_read/set semantics map Java volatile variables. They are

WARNING: Block comments use a leading /* on a separate line
#6394: FILE: util/bitmap.c:214:
+        /* If we avoided the full barrier in qemu_atomic_or(), issue a

WARNING: Block comments use a leading /* on a separate line
#7430: FILE: util/rcu.c:85:
+        /* Instead of using qemu_atomic_mb_set for index->waiting, and

WARNING: Block comments use a leading /* on a separate line
#7456: FILE: util/rcu.c:154:
+        /* In either case, the qemu_atomic_mb_set below blocks stores that free

total: 7 errors, 4 warnings, 6507 lines checked

Commit 25ca7029b2f2 (qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200921162346.188997-1-stefanha@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Eric Blake]

On 9/21/20 11:23 AM, Stefan Hajnoczi wrote:
> clang's C11 atomic_fetch_*() functions only take a C11 atomic type
> pointer argument. QEMU uses direct types (int, etc) and this causes a
> compiler error when a QEMU code calls these functions in a source file
> that also included <stdatomic.h> via a system header file:
> 
>    $ CC=clang CXX=clang++ ./configure ... && make
>    ../util/async.c:79:17: error: address argument to atomic operation must be a pointer to _Atomic type ('unsigned int *' invalid)
> 
> Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
> used by <stdatomic.h>. Prefix QEMU's APIs with qemu_ so that atomic.h
> and <stdatomic.h> can co-exist.
> 
> This patch was generated using:
> 
>    $ git diff | grep -o '\<atomic_[a-z0-9_]\+' | sort -u >/tmp/changed_identifiers

Missing a step in the recipe: namely, you probably modified 
include/qemu/atomic*.h prior to running 'git diff' (so that you actually 
had input to feed to grep -o).  But spelling it 'git diff HEAD^ 
include/qemu/atomic*.h | ...' does indeed give me a sane list of 
identifiers that looks like what you touched in the rest of the patch.

>    $ for identifier in $(</tmp/changed_identifiers64); do \

Also not quite the right recipe, based on the file name used in the line 
above.

>         sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l "\<$identifier\>") \
>      done
> 

Fortunately, running "git grep -c '\<atomic_[a-z0-9_]\+'" on the 
pre-patch state of the tree gives me a list that is somewhat close to 
yours, where the obvious difference in line counts is explained by:

> I manually fixed line-wrap issues and misaligned rST tables.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

First, focusing on the change summary:

>   docs/devel/lockcnt.txt                        |  14 +-
>   docs/devel/rcu.txt                            |  40 +--
>   accel/tcg/atomic_template.h                   |  20 +-
>   include/block/aio-wait.h                      |   4 +-
>   include/block/aio.h                           |   8 +-
>   include/exec/cpu_ldst.h                       |   2 +-
>   include/exec/exec-all.h                       |   6 +-
>   include/exec/log.h                            |   6 +-
>   include/exec/memory.h                         |   2 +-
>   include/exec/ram_addr.h                       |  27 +-
>   include/exec/ramlist.h                        |   2 +-
>   include/exec/tb-lookup.h                      |   4 +-
>   include/hw/core/cpu.h                         |   2 +-
>   include/qemu/atomic.h                         | 258 +++++++-------
>   include/qemu/atomic128.h                      |   6 +-

These two are the most important for the sake of this patch; perhaps 
it's worth a temporary override of your git orderfile if you have to 
respin, to list them first?

>   include/qemu/bitops.h                         |   2 +-
>   include/qemu/coroutine.h                      |   2 +-
>   include/qemu/log.h                            |   6 +-
>   include/qemu/queue.h                          |   8 +-
>   include/qemu/rcu.h                            |  10 +-
>   include/qemu/rcu_queue.h                      | 103 +++---

Presumably, this and any other file with an odd number of changes was 
due to a difference in lines after reformatting long lines.

>   include/qemu/seqlock.h                        |   8 +-
...

>   util/stats64.c                                |  34 +-
>   docs/devel/atomics.rst                        | 326 +++++++++---------
>   .../opensbi-riscv32-generic-fw_dynamic.elf    | Bin 558668 -> 558698 bytes
>   .../opensbi-riscv64-generic-fw_dynamic.elf    | Bin 620424 -> 620454 bytes

Why are we regenerating .elf files in this patch?  Is your change even 
correct for those two files?

>   scripts/kernel-doc                            |   2 +-
>   tcg/aarch64/tcg-target.c.inc                  |   2 +-
>   tcg/mips/tcg-target.c.inc                     |   2 +-
>   tcg/ppc/tcg-target.c.inc                      |   6 +-
>   tcg/sparc/tcg-target.c.inc                    |   5 +-
>   135 files changed, 1195 insertions(+), 1130 deletions(-)

I don't spot accel/tcg/atomic_common.c.inc in the list (which declares 
functions such as atomic_trace_rmw_pre) - I guess that's intentional 
based on how you tried to edit only the identifiers you touched in 
include/qemu/atomic*.h.

For the rest of this patch, I only spot-checked in places, trusting the 
mechanical nature of this patch, and not spotting anything wrong in the 
places I checked.  But the two .elf files worry me enough to withhold 
R-b.  At the same time, because it's big, it will probably be a source 
of conflicts if we don't get it in soon, but can also be regenerated (if 
your recipe is corrected) without too much difficulty.  So I am in favor 
of the idea.

> diff --git a/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf b/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf
> index eb9ebf5674d3953ab25de6bf4db134abe904eb20..35b80512446dcf5c49424ae90caacf3c579be1b5 100644
> GIT binary patch
> delta 98
> zcmX@pqx7mrsiB3jg{g(Pg=Gt?!4uZP)ZEhe?LdZ@5QIJ5?Hg+mgxS918!HgAZQt>Y
> ceSHN~X?i|K5fhYsvykI97nHrFhGPaN0Hp#a^8f$<
> 
> delta 62
> zcmaFWqjaW6siB3jg{g(Pg=Gt?!ISN#Pguo-rU!guEowjhjTMO5wjck-zP@66bv{QC
> S)Amn=9Jjf)U#j7l!3h9Zj2qGb
> 
> diff --git a/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf b/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf
> index 642a64e240d09b2ddb9fc12c74718ae8d386b9d3..9cf2cf23b747cb5be1d3389a0611d8697609c6f8 100644
> GIT binary patch
> delta 102
> zcmeBpue$8LYC{WS3sVbo3(FSPMGsgDQ*%q>w*wh6LJ;=!eV<s1Ak21y&#XYq2E^>!
> e4L)<s&w(mGAJ19D1Z6uWaUSP_vN>`&8@K>*RVYvZ
> 
> delta 66
> zcmZ4XUbW-BYC{WS3sVbo3(FSPMGv+wf50juH2uUU)}nU%&#XYq2E^>!?LTwO&)NPs
> Up0kK)dsGtVajxxZxttAL0Np<vJ^%m!
> 
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index 030b5c8691..9ec38a1bf1 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -1625,7 +1625,7 @@ sub dump_function($$) {
>       # If you mess with these regexps, it's a good idea to check that
>       # the following functions' documentation still comes out right:
>       # - parport_register_device (function pointer parameters)
> -    # - atomic_set (macro)
> +    # - qemu_atomic_set (macro)
>       # - pci_match_device, __copy_to_user (long return type)

Does the result of sphinx still look good, as mentioned in this comment?

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

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Paolo Bonzini]

On 21/09/20 18:23, Stefan Hajnoczi wrote:
> clang's C11 atomic_fetch_*() functions only take a C11 atomic type
> pointer argument. QEMU uses direct types (int, etc) and this causes a
> compiler error when a QEMU code calls these functions in a source file
> that also included <stdatomic.h> via a system header file:
> 
>   $ CC=clang CXX=clang++ ./configure ... && make
>   ../util/async.c:79:17: error: address argument to atomic operation must be a pointer to _Atomic type ('unsigned int *' invalid)
> 
> Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
> used by <stdatomic.h>. Prefix QEMU's APIs with qemu_ so that atomic.h
> and <stdatomic.h> can co-exist.
> 
> This patch was generated using:
> 
>   $ git diff | grep -o '\<atomic_[a-z0-9_]\+' | sort -u >/tmp/changed_identifiers
>   $ for identifier in $(</tmp/changed_identifiers64); do \
>        sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l "\<$identifier\>") \
>     done

It's certainly a good idea but it's quite verbose.

What about using atomic__* as the prefix?  It is not very common in QEMU
but there are some cases (and I cannot think of anything better).

Paolo

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[David Hildenbrand]

On 22.09.20 08:27, Paolo Bonzini wrote:
> On 21/09/20 18:23, Stefan Hajnoczi wrote:
>> clang's C11 atomic_fetch_*() functions only take a C11 atomic type
>> pointer argument. QEMU uses direct types (int, etc) and this causes a
>> compiler error when a QEMU code calls these functions in a source file
>> that also included <stdatomic.h> via a system header file:
>>
>>   $ CC=clang CXX=clang++ ./configure ... && make
>>   ../util/async.c:79:17: error: address argument to atomic operation must be a pointer to _Atomic type ('unsigned int *' invalid)
>>
>> Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
>> used by <stdatomic.h>. Prefix QEMU's APIs with qemu_ so that atomic.h
>> and <stdatomic.h> can co-exist.
>>
>> This patch was generated using:
>>
>>   $ git diff | grep -o '\<atomic_[a-z0-9_]\+' | sort -u >/tmp/changed_identifiers
>>   $ for identifier in $(</tmp/changed_identifiers64); do \
>>        sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l "\<$identifier\>") \
>>     done
> 
> It's certainly a good idea but it's quite verbose.
> 
> What about using atomic__* as the prefix?  It is not very common in QEMU
> but there are some cases (and I cannot think of anything better).
> 

aqomic_*, lol :)

> Paolo
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Paolo Bonzini]

On 22/09/20 08:45, David Hildenbrand wrote:
>> It's certainly a good idea but it's quite verbose.
>>
>> What about using atomic__* as the prefix?  It is not very common in QEMU
>> but there are some cases (and I cannot think of anything better).
>
> aqomic_*, lol :)

Actually qatomic_ would be a good one, wouldn't it?

Paolo

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[David Hildenbrand]

On 22.09.20 08:56, Paolo Bonzini wrote:
> On 22/09/20 08:45, David Hildenbrand wrote:
>>> It's certainly a good idea but it's quite verbose.
>>>
>>> What about using atomic__* as the prefix?  It is not very common in QEMU
>>> but there are some cases (and I cannot think of anything better).
>>
>> aqomic_*, lol :)
> 
> Actually qatomic_ would be a good one, wouldn't it?

agreed!

-- 
Thanks,

David / dhildenb

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Stefan Hajnoczi]

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

On Mon, Sep 21, 2020 at 01:56:08PM -0700, no-reply@patchew.org wrote:
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #2968: FILE: include/qemu/atomic.h:152:
> +#define qemu_atomic_rcu_read__nocheck(ptr, valptr)      \
>      __atomic_load(ptr, valptr, __ATOMIC_RELAXED);       \
>      smp_read_barrier_depends();
> 
> ERROR: space required before that '*' (ctx:VxB)
> #3123: FILE: include/qemu/atomic.h:347:
> +#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
>                                                                   ^
> 
> ERROR: Use of volatile is usually wrong, please add a comment
> #3123: FILE: include/qemu/atomic.h:347:
> +#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
> 
> ERROR: space required before that '*' (ctx:VxB)
> #3125: FILE: include/qemu/atomic.h:349:
> +    ((*(__typeof__(*(p)) volatile*) (p)) = (i))
>                                   ^
> 
> ERROR: Use of volatile is usually wrong, please add a comment
> #3125: FILE: include/qemu/atomic.h:349:
> +    ((*(__typeof__(*(p)) volatile*) (p)) = (i))
> 
> ERROR: space required after that ',' (ctx:VxV)
> #3130: FILE: include/qemu/atomic.h:352:
> +#define qemu_atomic_set(ptr, i)     qemu_atomic_set__nocheck(ptr,i)
>                                                                  ^
> 
> ERROR: memory barrier without comment
> #3205: FILE: include/qemu/atomic.h:410:
> +#define qemu_atomic_xchg(ptr, i) (smp_mb(), __sync_lock_test_and_set(ptr, i))
> 
> WARNING: Block comments use a leading /* on a separate line
> #3280: FILE: include/qemu/atomic.h:462:
> +/* qemu_atomic_mb_read/set semantics map Java volatile variables. They are
> 
> WARNING: Block comments use a leading /* on a separate line
> #6394: FILE: util/bitmap.c:214:
> +        /* If we avoided the full barrier in qemu_atomic_or(), issue a
> 
> WARNING: Block comments use a leading /* on a separate line
> #7430: FILE: util/rcu.c:85:
> +        /* Instead of using qemu_atomic_mb_set for index->waiting, and
> 
> WARNING: Block comments use a leading /* on a separate line
> #7456: FILE: util/rcu.c:154:
> +        /* In either case, the qemu_atomic_mb_set below blocks stores that free
> 
> total: 7 errors, 4 warnings, 6507 lines checked

These are pre-existing coding style issues. This is a big patch that
tries to make as few actual changes as possible so I would rather not
try to fix them.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Daniel P. Berrangé]

On Tue, Sep 22, 2020 at 08:56:06AM +0200, Paolo Bonzini wrote:
> On 22/09/20 08:45, David Hildenbrand wrote:
> >> It's certainly a good idea but it's quite verbose.
> >>
> >> What about using atomic__* as the prefix?  It is not very common in QEMU
> >> but there are some cases (and I cannot think of anything better).
> >
> > aqomic_*, lol :)
> 
> Actually qatomic_ would be a good one, wouldn't it?

Yes, I think just adding a 'q' on the front of methods is more than
sufficient (see also all the qcrypto_*, qio_* APIs I wrote). The
only think a plain 'q' prefix is likely to clash with is the Qt
library and that isn't something we're likely to link with (famous
last words...).

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Philippe Mathieu-Daudé]

On 9/22/20 10:17 AM, Stefan Hajnoczi wrote:
> On Mon, Sep 21, 2020 at 01:56:08PM -0700, no-reply@patchew.org wrote:
>> ERROR: Macros with multiple statements should be enclosed in a do - while loop
>> #2968: FILE: include/qemu/atomic.h:152:
>> +#define qemu_atomic_rcu_read__nocheck(ptr, valptr)      \
>>      __atomic_load(ptr, valptr, __ATOMIC_RELAXED);       \
>>      smp_read_barrier_depends();
>>
...
>> WARNING: Block comments use a leading /* on a separate line
>> #7456: FILE: util/rcu.c:154:
>> +        /* In either case, the qemu_atomic_mb_set below blocks stores that free
>>
>> total: 7 errors, 4 warnings, 6507 lines checked
> 
> These are pre-existing coding style issues. This is a big patch that
> tries to make as few actual changes as possible so I would rather not
> try to fix them.

What I do with automated patches triggering checkpatch errors:

- run automated conversion
- fix errors until checkpatch is happy
- run automated conversion inversed
- result is the checkpatch changes, commit them
- run automated conversion again, commit

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Stefan Hajnoczi]

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

On Mon, Sep 21, 2020 at 04:29:10PM -0500, Eric Blake wrote:
> On 9/21/20 11:23 AM, Stefan Hajnoczi wrote:

Thanks for the review! Your feedback prompted me to do this more
systematically. I fixed the command-lines and published a diff of just
the manual changes I made on top of the mechanical changes (see v2).

> > clang's C11 atomic_fetch_*() functions only take a C11 atomic type
> > pointer argument. QEMU uses direct types (int, etc) and this causes a
> > compiler error when a QEMU code calls these functions in a source file
> > that also included <stdatomic.h> via a system header file:
> > 
> >    $ CC=clang CXX=clang++ ./configure ... && make
> >    ../util/async.c:79:17: error: address argument to atomic operation must be a pointer to _Atomic type ('unsigned int *' invalid)
> > 
> > Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
> > used by <stdatomic.h>. Prefix QEMU's APIs with qemu_ so that atomic.h
> > and <stdatomic.h> can co-exist.
> > 
> > This patch was generated using:
> > 
> >    $ git diff | grep -o '\<atomic_[a-z0-9_]\+' | sort -u >/tmp/changed_identifiers
> 
> Missing a step in the recipe: namely, you probably modified
> include/qemu/atomic*.h prior to running 'git diff' (so that you actually had
> input to feed to grep -o).  But spelling it 'git diff HEAD^
> include/qemu/atomic*.h | ...' does indeed give me a sane list of identifiers
> that looks like what you touched in the rest of the patch.

Yes, I edited the file first and then used this command-line. The one
you posted it better :).

> 
> >    $ for identifier in $(</tmp/changed_identifiers64); do \
> 
> Also not quite the right recipe, based on the file name used in the line
> above.

Yes, "64" is when I realized the original grep expression hadn't matched
the atomic64 APIs.

These commands only show the gist of it. It involved a few manual steps.

> 
> >         sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l "\<$identifier\>") \
> >      done
> > 
> 
> Fortunately, running "git grep -c '\<atomic_[a-z0-9_]\+'" on the pre-patch
> state of the tree gives me a list that is somewhat close to yours, where the
> obvious difference in line counts is explained by:
> 
> > I manually fixed line-wrap issues and misaligned rST tables.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> 
> First, focusing on the change summary:
> 
> >   docs/devel/lockcnt.txt                        |  14 +-
> >   docs/devel/rcu.txt                            |  40 +--
> >   accel/tcg/atomic_template.h                   |  20 +-
> >   include/block/aio-wait.h                      |   4 +-
> >   include/block/aio.h                           |   8 +-
> >   include/exec/cpu_ldst.h                       |   2 +-
> >   include/exec/exec-all.h                       |   6 +-
> >   include/exec/log.h                            |   6 +-
> >   include/exec/memory.h                         |   2 +-
> >   include/exec/ram_addr.h                       |  27 +-
> >   include/exec/ramlist.h                        |   2 +-
> >   include/exec/tb-lookup.h                      |   4 +-
> >   include/hw/core/cpu.h                         |   2 +-
> >   include/qemu/atomic.h                         | 258 +++++++-------
> >   include/qemu/atomic128.h                      |   6 +-
> 
> These two are the most important for the sake of this patch; perhaps it's
> worth a temporary override of your git orderfile if you have to respin, to
> list them first?

Will do in v2.

> 
> >   include/qemu/bitops.h                         |   2 +-
> >   include/qemu/coroutine.h                      |   2 +-
> >   include/qemu/log.h                            |   6 +-
> >   include/qemu/queue.h                          |   8 +-
> >   include/qemu/rcu.h                            |  10 +-
> >   include/qemu/rcu_queue.h                      | 103 +++---
> 
> Presumably, this and any other file with an odd number of changes was due to
> a difference in lines after reformatting long lines.

Yes, line-wrapping required many changes in this file.

> 
> >   include/qemu/seqlock.h                        |   8 +-
> ...
> 
> >   util/stats64.c                                |  34 +-
> >   docs/devel/atomics.rst                        | 326 +++++++++---------
> >   .../opensbi-riscv32-generic-fw_dynamic.elf    | Bin 558668 -> 558698 bytes
> >   .../opensbi-riscv64-generic-fw_dynamic.elf    | Bin 620424 -> 620454 bytes
> 
> Why are we regenerating .elf files in this patch?  Is your change even
> correct for those two files?

Thanks for noticing this! The git-grep(1) man page documents the -I
option for skipping binary files. I thought this was the default.

Will fix in v2.

> 
> >   scripts/kernel-doc                            |   2 +-
> >   tcg/aarch64/tcg-target.c.inc                  |   2 +-
> >   tcg/mips/tcg-target.c.inc                     |   2 +-
> >   tcg/ppc/tcg-target.c.inc                      |   6 +-
> >   tcg/sparc/tcg-target.c.inc                    |   5 +-
> >   135 files changed, 1195 insertions(+), 1130 deletions(-)
> 
> I don't spot accel/tcg/atomic_common.c.inc in the list (which declares
> functions such as atomic_trace_rmw_pre) - I guess that's intentional based
> on how you tried to edit only the identifiers you touched in
> include/qemu/atomic*.h.

Yes. The namespace cleaning only applies to atomic.h. Any other part of
QEMU uses atomic_ is unaffected.

> > diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> > index 030b5c8691..9ec38a1bf1 100755
> > --- a/scripts/kernel-doc
> > +++ b/scripts/kernel-doc
> > @@ -1625,7 +1625,7 @@ sub dump_function($$) {
> >       # If you mess with these regexps, it's a good idea to check that
> >       # the following functions' documentation still comes out right:
> >       # - parport_register_device (function pointer parameters)
> > -    # - atomic_set (macro)
> > +    # - qemu_atomic_set (macro)
> >       # - pci_match_device, __copy_to_user (long return type)
> 
> Does the result of sphinx still look good, as mentioned in this comment?

Yes.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions

[Stefan Hajnoczi]

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

On Tue, Sep 22, 2020 at 09:18:49AM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 22, 2020 at 08:56:06AM +0200, Paolo Bonzini wrote:
> > On 22/09/20 08:45, David Hildenbrand wrote:
> > >> It's certainly a good idea but it's quite verbose.
> > >>
> > >> What about using atomic__* as the prefix?  It is not very common in QEMU
> > >> but there are some cases (and I cannot think of anything better).
> > >
> > > aqomic_*, lol :)
> > 
> > Actually qatomic_ would be a good one, wouldn't it?
> 
> Yes, I think just adding a 'q' on the front of methods is more than
> sufficient (see also all the qcrypto_*, qio_* APIs I wrote). The
> only think a plain 'q' prefix is likely to clash with is the Qt
> library and that isn't something we're likely to link with (famous
> last words...).

This is why I didn't use "qatomic". "atomic" feels too common to prefix
with just a single letter.

But I grepped /usr/include and code searched GitHub. I can't find any
uses of "qatomic_" so it looks safe. FWIW Qt does have qatomic.h but
doesn't use the name for identifiers in the code.

Let's do it!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]