Re: [Qemu-devel] [PATCH 03/18] Include qapi/error.h exactly where needed

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 01/30/2018 04:21 AM, Markus Armbruster wrote:
>> This cleanup makes the number of objects depending on qapi/error.h
>> drop from 1910 (out of 4739) to 1612 in my "build everything" tree.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  arch_init.c                             | 1 +
>>  audio/wavcapture.c                      | 1 +
>>  balloon.c                               | 1 +
>>  block.c                                 | 2 ++
>>  block/block-backend.c                   | 1 +
>>  block/iscsi.c                           | 1 +
>>  block/qapi.c                            | 1 +
>
> So several .c files have to use it explicitly,
>
>>  fsdev/qemu-fsdev-throttle.h             | 1 -
>
>>  include/crypto/random.h                 | 1 -
>>  include/crypto/xts.h                    | 1 -
>>  include/hw/ide/internal.h               | 1 -
>>  include/ui/console.h                    | 1 -
>
> because they were previously getting it from .h files that don't
> directly emit an error.  Makes sense.
>
> Patches like this are easy to test - if it still compiles, you did it
> right ;)  Out of curiousity, how are you counting how many files got
> compiled per run?  Touch the .h, then pass 'make' output to 'grep -c "^
> CC "'?

The data is up for grabs: in the .d gcc spits out for make.  I process
them like this, after fresh build:

    $ find bld -name \*.d -exec cat {} + | clean-deps

where clean-deps is this AWK script:

    #!/usr/bin/awk -f
    /\\$/ {
        l = $0
        while (sub(/\\$/, "", l) && (getline t))
            l = l t
        $0 = l
    }

    NF > 1 {
        delete a
        for (i = 2; i <= NF; i++) {
            n = $i
            # ignore system headers and non-headers
            if (match(n, /^\/usr/) || !match(n, /\.h$/))
                continue
            # strip leading ../ from target build running in sub-directory
            sub(/^\.\.\//, "", n)
            # normalize /../ and /./
            while (sub(/[^\/]*\/\.\.\//, "", n)) ;
            gsub(/\/\.\//, "/", n)
            # absolute -> relative
            sub(/\/home\/armbru\/work\//, "/work/armbru/", n)
            sub(/\/work\/armbru\/qemu\/(bld\/)?/, "", n)
            # ignore dupes
            if (n in a)
                continue
            a[n] = i
            h[n]++
        }
    }

    END {
        for (i in h)
            print i, h[i]
    }

My conversion from absolute to relative is specific to my personal
setup.  It would have to be generalized before we can put this into
scripts.

> But just to make sure nothing weird is happening, I also read through
> it, and found:
>
>> 
>> diff --git a/arch_init.c b/arch_init.c
>> index a0b8ed6167..0fb8093f92 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -21,6 +21,7 @@
>>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>   * THE SOFTWARE.
>>   */
>> +
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "cpu.h"
>
> Spurious whitespace change.  Should this belong in 1/18, even though
> arch_init.c didn't need cleanup there?  Or...
>
>> +++ b/block.c
>> @@ -21,6 +21,7 @@
>>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>   * THE SOFTWARE.
>>   */
>> +
>>  #include "qemu/osdep.h"
>>  #include "block/trace.h"
>>  #include "block/block_int.h"
>
> ...since you did it again, do you need a separate patch for ALL of these
> types of whitespace cleanups near osdep.h?

I routinely insert this blank line when I touch includes anyway.

>> +++ b/block/qcow2.c
>> @@ -21,6 +21,7 @@
>>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>   * THE SOFTWARE.
>>   */
>> +
>>  #include "qemu/osdep.h"
>>  #include "block/block_int.h"
>>  #include "sysemu/block-backend.h"
>
> and again
>
>> +++ b/chardev/char-ringbuf.c
>> @@ -21,9 +21,11 @@
>>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>   * THE SOFTWARE.
>>   */
>> +
>>  #include "qemu/osdep.h"
>>  #include "chardev/char.h"
>>  #include "qmp-commands.h"
>> +#include "qapi/error.h"
>>  #include "qemu/base64.h"
>>  
>>  /* Ring buffer chardev */
>
> Here, it's in the same hunk, so a bit more forgivable.  But there's
> definitely enough of them that a separate commit might be in order.
>
> At any rate, whether done as one patch (with a better commit message) or
> as two, I see nothing semantically wrong with the cleanups done here, so

I'll amend the commit message.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!