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

[Eric Blake]

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 "'?

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?

> +++ 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

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature