[Qemu-devel] [PATCH] Clean up includes

Markus Armbruster posted 1 patch 5 years, 1 month ago
Test asan failed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190313162812.8885-1-armbru@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Thomas Huth <thuth@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Eduardo Habkost <ehabkost@redhat.com>, Viktor Prutyanov <viktor.prutyanov@phystech.edu>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Aurelien Jarno <aurelien@aurel32.net>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
contrib/elf2dmp/main.c   | 3 +--
contrib/elf2dmp/pdb.c    | 3 +--
hw/display/ati.c         | 1 +
hw/display/ati_2d.c      | 1 +
hw/display/ati_dbg.c     | 1 +
hw/display/ati_int.h     | 1 -
include/hw/cpu/cluster.h | 1 -
tests/fp/platform.h      | 1 -
tests/libqos/qgraph.h    | 4 ----
tests/qos-test.c         | 2 +-
10 files changed, 6 insertions(+), 12 deletions(-)
[Qemu-devel] [PATCH] Clean up includes
Posted by Markus Armbruster 5 years, 1 month ago
Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes, with the changes
to the following files manually reverted:

    contrib/libvhost-user/libvhost-user-glib.h
    contrib/libvhost-user/libvhost-user.c
    contrib/libvhost-user/libvhost-user.h
    linux-user/mips64/cpu_loop.c
    linux-user/mips64/signal.c
    linux-user/sparc64/cpu_loop.c
    linux-user/sparc64/signal.c
    linux-user/x86_64/cpu_loop.c
    linux-user/x86_64/signal.c
    slirp/src/*
    target/s390x/gen-features.c
    tests/migration/s390x/a-b-bios.c
    tests/test-rcu-simpleq.c
    tests/test-rcu-tailq.c
    tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c

We're in the process of spinning out slirp/.  tests/uefi-test-tools/
is guest software.  The remaining reverts are the same as in commit
b7d89466dde.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 contrib/elf2dmp/main.c   | 3 +--
 contrib/elf2dmp/pdb.c    | 3 +--
 hw/display/ati.c         | 1 +
 hw/display/ati_2d.c      | 1 +
 hw/display/ati_dbg.c     | 1 +
 hw/display/ati_int.h     | 1 -
 include/hw/cpu/cluster.h | 1 -
 tests/fp/platform.h      | 1 -
 tests/libqos/qgraph.h    | 4 ----
 tests/qos-test.c         | 2 +-
 10 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 1bfeb89ba7..9a2dbc2902 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -5,9 +5,8 @@
  *
  */
 
-#include <inttypes.h>
-
 #include "qemu/osdep.h"
+
 #include "err.h"
 #include "addrspace.h"
 #include "pe.h"
diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index 64af20f584..a5bd40c99d 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -18,9 +18,8 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
  */
 
-#include <inttypes.h>
-
 #include "qemu/osdep.h"
+
 #include "pdb.h"
 #include "err.h"
 
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 8322f52aff..b06c4d571a 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -16,6 +16,7 @@
  * No 3D at all yet (maybe after 2D works, but feel free to improve it)
  */
 
+#include "qemu/osdep.h"
 #include "ati_int.h"
 #include "ati_regs.h"
 #include "vga_regs.h"
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index bc98ba6eeb..f31b3c27c7 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -7,6 +7,7 @@
  * This work is licensed under the GNU GPL license version 2 or later.
  */
 
+#include "qemu/osdep.h"
 #include "ati_int.h"
 #include "ati_regs.h"
 #include "qemu/log.h"
diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
index 1e6c32624e..b045f81d06 100644
--- a/hw/display/ati_dbg.c
+++ b/hw/display/ati_dbg.c
@@ -1,3 +1,4 @@
+#include "qemu/osdep.h"
 #include "ati_int.h"
 
 #ifdef DEBUG_ATI
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index a6f3e20e63..2f426064cf 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -9,7 +9,6 @@
 #ifndef ATI_INT_H
 #define ATI_INT_H
 
-#include "qemu/osdep.h"
 #include "hw/pci/pci.h"
 #include "vga_int.h"
 
diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h
index 549c2d31d4..01c1e50cd2 100644
--- a/include/hw/cpu/cluster.h
+++ b/include/hw/cpu/cluster.h
@@ -20,7 +20,6 @@
 #ifndef HW_CPU_CLUSTER_H
 #define HW_CPU_CLUSTER_H
 
-#include "qemu/osdep.h"
 #include "hw/qdev.h"
 
 /*
diff --git a/tests/fp/platform.h b/tests/fp/platform.h
index c20ba70baa..f8c423dde3 100644
--- a/tests/fp/platform.h
+++ b/tests/fp/platform.h
@@ -29,7 +29,6 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
  * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
-#include "config-host.h"
 
 #ifndef HOST_WORDS_BIGENDIAN
 #define LITTLEENDIAN 1
diff --git a/tests/libqos/qgraph.h b/tests/libqos/qgraph.h
index ef0c73837a..e799095b30 100644
--- a/tests/libqos/qgraph.h
+++ b/tests/libqos/qgraph.h
@@ -19,11 +19,7 @@
 #ifndef QGRAPH_H
 #define QGRAPH_H
 
-#include <stdio.h>
-#include <stdlib.h>
-#include <stdbool.h>
 #include <gmodule.h>
-#include <glib.h>
 #include "qemu/module.h"
 #include "malloc.h"
 
diff --git a/tests/qos-test.c b/tests/qos-test.c
index 6b1145eccc..ae2fb5de1c 100644
--- a/tests/qos-test.c
+++ b/tests/qos-test.c
@@ -16,8 +16,8 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
 
-#include <getopt.h>
 #include "qemu/osdep.h"
+#include <getopt.h>
 #include "libqtest.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qbool.h"
-- 
2.17.2


Re: [Qemu-devel] [PATCH] Clean up includes
Posted by BALATON Zoltan 5 years, 1 month ago
On Wed, 13 Mar 2019, Markus Armbruster wrote:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
>
> This commit was created with scripts/clean-includes, with the changes
> to the following files manually reverted:
>
>    contrib/libvhost-user/libvhost-user-glib.h
>    contrib/libvhost-user/libvhost-user.c
>    contrib/libvhost-user/libvhost-user.h
>    linux-user/mips64/cpu_loop.c
>    linux-user/mips64/signal.c
>    linux-user/sparc64/cpu_loop.c
>    linux-user/sparc64/signal.c
>    linux-user/x86_64/cpu_loop.c
>    linux-user/x86_64/signal.c
>    slirp/src/*
>    target/s390x/gen-features.c
>    tests/migration/s390x/a-b-bios.c
>    tests/test-rcu-simpleq.c
>    tests/test-rcu-tailq.c
>    tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>
> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
> is guest software.  The remaining reverts are the same as in commit
> b7d89466dde.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> contrib/elf2dmp/main.c   | 3 +--
> contrib/elf2dmp/pdb.c    | 3 +--
> hw/display/ati.c         | 1 +
> hw/display/ati_2d.c      | 1 +
> hw/display/ati_dbg.c     | 1 +
> hw/display/ati_int.h     | 1 -
> include/hw/cpu/cluster.h | 1 -
> tests/fp/platform.h      | 1 -
> tests/libqos/qgraph.h    | 4 ----
> tests/qos-test.c         | 2 +-
> 10 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
> index 1bfeb89ba7..9a2dbc2902 100644
> --- a/contrib/elf2dmp/main.c
> +++ b/contrib/elf2dmp/main.c
> @@ -5,9 +5,8 @@
>  *
>  */
>
> -#include <inttypes.h>
> -
> #include "qemu/osdep.h"
> +
> #include "err.h"
> #include "addrspace.h"
> #include "pe.h"
> diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
> index 64af20f584..a5bd40c99d 100644
> --- a/contrib/elf2dmp/pdb.c
> +++ b/contrib/elf2dmp/pdb.c
> @@ -18,9 +18,8 @@
>  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>  */
>
> -#include <inttypes.h>
> -
> #include "qemu/osdep.h"
> +
> #include "pdb.h"
> #include "err.h"
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 8322f52aff..b06c4d571a 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -16,6 +16,7 @@
>  * No 3D at all yet (maybe after 2D works, but feel free to improve it)
>  */
>
> +#include "qemu/osdep.h"
> #include "ati_int.h"
> #include "ati_regs.h"
> #include "vga_regs.h"
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index bc98ba6eeb..f31b3c27c7 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -7,6 +7,7 @@
>  * This work is licensed under the GNU GPL license version 2 or later.
>  */
>
> +#include "qemu/osdep.h"
> #include "ati_int.h"
> #include "ati_regs.h"
> #include "qemu/log.h"
> diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
> index 1e6c32624e..b045f81d06 100644
> --- a/hw/display/ati_dbg.c
> +++ b/hw/display/ati_dbg.c
> @@ -1,3 +1,4 @@
> +#include "qemu/osdep.h"
> #include "ati_int.h"
>
> #ifdef DEBUG_ATI
> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> index a6f3e20e63..2f426064cf 100644
> --- a/hw/display/ati_int.h
> +++ b/hw/display/ati_int.h
> @@ -9,7 +9,6 @@
> #ifndef ATI_INT_H
> #define ATI_INT_H
>
> -#include "qemu/osdep.h"

What's wrong with ati_int.h including osdep.h first and everything else 
including ati_int.h first? I think it was OK that way so unless there's a 
good reason to explicitely include osdep in all files that also include 
ati_int.h I think these should not be changed. For the ati model we need 
ati_int.h included first so it's OK to include osdep.h from there.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] Clean up includes
Posted by Markus Armbruster 5 years, 1 month ago
BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Wed, 13 Mar 2019, Markus Armbruster wrote:
>> Clean up includes so that osdep.h is included first and headers
>> which it implies are not included manually.
>>
>> This commit was created with scripts/clean-includes, with the changes
>> to the following files manually reverted:
>>
>>    contrib/libvhost-user/libvhost-user-glib.h
>>    contrib/libvhost-user/libvhost-user.c
>>    contrib/libvhost-user/libvhost-user.h
>>    linux-user/mips64/cpu_loop.c
>>    linux-user/mips64/signal.c
>>    linux-user/sparc64/cpu_loop.c
>>    linux-user/sparc64/signal.c
>>    linux-user/x86_64/cpu_loop.c
>>    linux-user/x86_64/signal.c
>>    slirp/src/*
>>    target/s390x/gen-features.c
>>    tests/migration/s390x/a-b-bios.c
>>    tests/test-rcu-simpleq.c
>>    tests/test-rcu-tailq.c
>>    tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>>
>> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
>> is guest software.  The remaining reverts are the same as in commit
>> b7d89466dde.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]
>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>> index bc98ba6eeb..f31b3c27c7 100644
>> --- a/hw/display/ati_2d.c
>> +++ b/hw/display/ati_2d.c
>> @@ -7,6 +7,7 @@
>>  * This work is licensed under the GNU GPL license version 2 or later.
>>  */
>>
>> +#include "qemu/osdep.h"
>> #include "ati_int.h"
>> #include "ati_regs.h"
>> #include "qemu/log.h"
>> diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
>> index 1e6c32624e..b045f81d06 100644
>> --- a/hw/display/ati_dbg.c
>> +++ b/hw/display/ati_dbg.c
>> @@ -1,3 +1,4 @@
>> +#include "qemu/osdep.h"
>> #include "ati_int.h"
>>
>> #ifdef DEBUG_ATI
>> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
>> index a6f3e20e63..2f426064cf 100644
>> --- a/hw/display/ati_int.h
>> +++ b/hw/display/ati_int.h
>> @@ -9,7 +9,6 @@
>> #ifndef ATI_INT_H
>> #define ATI_INT_H
>>
>> -#include "qemu/osdep.h"
>
> What's wrong with ati_int.h including osdep.h first and everything
> else including ati_int.h first? I think it was OK that way so unless
> there's a good reason to explicitely include osdep in all files that
> also include ati_int.h I think these should not be changed. For the
> ati model we need ati_int.h included first so it's OK to include
> osdep.h from there.

./HACKING explains:

    1.2. Include directives

    Order include directives as follows:

    #include "qemu/osdep.h"  /* Always first... */
    #include <...>           /* then system headers... */
    #include "..."           /* and finally QEMU headers. */

    The "qemu/osdep.h" header contains preprocessor macros that affect the behavior
    of core system headers like <stdint.h>.  It must be the first include so that
    core system headers included by external libraries get the preprocessor macros
    that QEMU depends on.

    Do not include "qemu/osdep.h" from header files since the .c file will have
    already included it.

Re: [Qemu-devel] [PATCH] Clean up includes
Posted by BALATON Zoltan 5 years, 1 month ago
On Wed, 13 Mar 2019, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Wed, 13 Mar 2019, Markus Armbruster wrote:
>>> Clean up includes so that osdep.h is included first and headers
>>> which it implies are not included manually.
>>>
>>> This commit was created with scripts/clean-includes, with the changes
>>> to the following files manually reverted:
>>>
>>>    contrib/libvhost-user/libvhost-user-glib.h
>>>    contrib/libvhost-user/libvhost-user.c
>>>    contrib/libvhost-user/libvhost-user.h
>>>    linux-user/mips64/cpu_loop.c
>>>    linux-user/mips64/signal.c
>>>    linux-user/sparc64/cpu_loop.c
>>>    linux-user/sparc64/signal.c
>>>    linux-user/x86_64/cpu_loop.c
>>>    linux-user/x86_64/signal.c
>>>    slirp/src/*
>>>    target/s390x/gen-features.c
>>>    tests/migration/s390x/a-b-bios.c
>>>    tests/test-rcu-simpleq.c
>>>    tests/test-rcu-tailq.c
>>>    tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>>>
>>> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
>>> is guest software.  The remaining reverts are the same as in commit
>>> b7d89466dde.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> [...]
>>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>>> index bc98ba6eeb..f31b3c27c7 100644
>>> --- a/hw/display/ati_2d.c
>>> +++ b/hw/display/ati_2d.c
>>> @@ -7,6 +7,7 @@
>>>  * This work is licensed under the GNU GPL license version 2 or later.
>>>  */
>>>
>>> +#include "qemu/osdep.h"
>>> #include "ati_int.h"
>>> #include "ati_regs.h"
>>> #include "qemu/log.h"
>>> diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
>>> index 1e6c32624e..b045f81d06 100644
>>> --- a/hw/display/ati_dbg.c
>>> +++ b/hw/display/ati_dbg.c
>>> @@ -1,3 +1,4 @@
>>> +#include "qemu/osdep.h"
>>> #include "ati_int.h"
>>>
>>> #ifdef DEBUG_ATI
>>> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
>>> index a6f3e20e63..2f426064cf 100644
>>> --- a/hw/display/ati_int.h
>>> +++ b/hw/display/ati_int.h
>>> @@ -9,7 +9,6 @@
>>> #ifndef ATI_INT_H
>>> #define ATI_INT_H
>>>
>>> -#include "qemu/osdep.h"
>>
>> What's wrong with ati_int.h including osdep.h first and everything
>> else including ati_int.h first? I think it was OK that way so unless
>> there's a good reason to explicitely include osdep in all files that
>> also include ati_int.h I think these should not be changed. For the
>> ati model we need ati_int.h included first so it's OK to include
>> osdep.h from there.
>
> ./HACKING explains:
>
>    1.2. Include directives
>
>    Order include directives as follows:
>
>    #include "qemu/osdep.h"  /* Always first... */
>    #include <...>           /* then system headers... */
>    #include "..."           /* and finally QEMU headers. */
>
>    The "qemu/osdep.h" header contains preprocessor macros that affect the behavior
>    of core system headers like <stdint.h>.  It must be the first include so that
>    core system headers included by external libraries get the preprocessor macros
>    that QEMU depends on.
>
>    Do not include "qemu/osdep.h" from header files since the .c file will have
>    already included it.

I'm not convinced. The rule is to include osdep.h first and it's currently 
satisified without this change as well but if it makes clean-includes 
script happy then I don't mind.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] Clean up includes
Posted by Markus Armbruster 5 years, 1 month ago
BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Wed, 13 Mar 2019, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Wed, 13 Mar 2019, Markus Armbruster wrote:
>>>> Clean up includes so that osdep.h is included first and headers
>>>> which it implies are not included manually.
>>>>
>>>> This commit was created with scripts/clean-includes, with the changes
>>>> to the following files manually reverted:
>>>>
>>>>    contrib/libvhost-user/libvhost-user-glib.h
>>>>    contrib/libvhost-user/libvhost-user.c
>>>>    contrib/libvhost-user/libvhost-user.h
>>>>    linux-user/mips64/cpu_loop.c
>>>>    linux-user/mips64/signal.c
>>>>    linux-user/sparc64/cpu_loop.c
>>>>    linux-user/sparc64/signal.c
>>>>    linux-user/x86_64/cpu_loop.c
>>>>    linux-user/x86_64/signal.c
>>>>    slirp/src/*
>>>>    target/s390x/gen-features.c
>>>>    tests/migration/s390x/a-b-bios.c
>>>>    tests/test-rcu-simpleq.c
>>>>    tests/test-rcu-tailq.c
>>>>    tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>>>>
>>>> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
>>>> is guest software.  The remaining reverts are the same as in commit
>>>> b7d89466dde.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> [...]
>>>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>>>> index bc98ba6eeb..f31b3c27c7 100644
>>>> --- a/hw/display/ati_2d.c
>>>> +++ b/hw/display/ati_2d.c
>>>> @@ -7,6 +7,7 @@
>>>>  * This work is licensed under the GNU GPL license version 2 or later.
>>>>  */
>>>>
>>>> +#include "qemu/osdep.h"
>>>> #include "ati_int.h"
>>>> #include "ati_regs.h"
>>>> #include "qemu/log.h"
>>>> diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
>>>> index 1e6c32624e..b045f81d06 100644
>>>> --- a/hw/display/ati_dbg.c
>>>> +++ b/hw/display/ati_dbg.c
>>>> @@ -1,3 +1,4 @@
>>>> +#include "qemu/osdep.h"
>>>> #include "ati_int.h"
>>>>
>>>> #ifdef DEBUG_ATI
>>>> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
>>>> index a6f3e20e63..2f426064cf 100644
>>>> --- a/hw/display/ati_int.h
>>>> +++ b/hw/display/ati_int.h
>>>> @@ -9,7 +9,6 @@
>>>> #ifndef ATI_INT_H
>>>> #define ATI_INT_H
>>>>
>>>> -#include "qemu/osdep.h"
>>>
>>> What's wrong with ati_int.h including osdep.h first and everything
>>> else including ati_int.h first? I think it was OK that way so unless
>>> there's a good reason to explicitely include osdep in all files that
>>> also include ati_int.h I think these should not be changed. For the
>>> ati model we need ati_int.h included first so it's OK to include
>>> osdep.h from there.
>>
>> ./HACKING explains:
>>
>>    1.2. Include directives
>>
>>    Order include directives as follows:
>>
>>    #include "qemu/osdep.h"  /* Always first... */
>>    #include <...>           /* then system headers... */
>>    #include "..."           /* and finally QEMU headers. */
>>
>>    The "qemu/osdep.h" header contains preprocessor macros that affect the behavior
>>    of core system headers like <stdint.h>.  It must be the first include so that
>>    core system headers included by external libraries get the preprocessor macros
>>    that QEMU depends on.
>>
>>    Do not include "qemu/osdep.h" from header files since the .c file will have
>>    already included it.
>
> I'm not convinced. The rule is to include osdep.h first and it's
> currently satisified without this change as well but if it makes
> clean-includes script happy then I don't mind.

The rule is actually to include osdep.h first in .c, and never in .h.

We chose this rule because it makes conformance locally obvious.  You
don't have to chase down a chain of first includes to verify it ends in
"osdep.h" and none of the intermediate headers have anything before
their first #include.

The obviousness translates into robustness here.  If you hide #include
"osdep.h" in a header, you can easily lose "firstness" in a
harmless-looking change to any of the sources involved.  Pray this
breaks the build, because if it breaks only at run-time, it'll be nasty
to debug.

Re: [Qemu-devel] [PATCH] Clean up includes
Posted by Eric Blake 5 years, 1 month ago
On 3/13/19 11:28 AM, Markus Armbruster wrote:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
> 
> This commit was created with scripts/clean-includes, with the changes
> to the following files manually reverted:

Is it worth tweaking scripts/clean-includes as a pre-req patch so that
some of these files become listed exceptions and don't need manual
reversion here?

> 
>     contrib/libvhost-user/libvhost-user-glib.h
>     contrib/libvhost-user/libvhost-user.c
>     contrib/libvhost-user/libvhost-user.h
>     linux-user/mips64/cpu_loop.c
>     linux-user/mips64/signal.c
>     linux-user/sparc64/cpu_loop.c
>     linux-user/sparc64/signal.c
>     linux-user/x86_64/cpu_loop.c
>     linux-user/x86_64/signal.c
>     slirp/src/*
>     target/s390x/gen-features.c
>     tests/migration/s390x/a-b-bios.c
>     tests/test-rcu-simpleq.c
>     tests/test-rcu-tailq.c
>     tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
> 
> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
> is guest software.  The remaining reverts are the same as in commit
> b7d89466dde.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/contrib/elf2dmp/pdb.c
> @@ -18,9 +18,8 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA

Unrelated, but if someone is looking for a project, cleaning up stale
address references in older GPL copy-and-paste is a possibility.

I think these changes are okay for 4.0 soft freeze; but it also doesn't
hurt if this delays into 4.1.

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

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

Re: [Qemu-devel] [PATCH] Clean up includes
Posted by Markus Armbruster 5 years, 1 month ago
Eric Blake <eblake@redhat.com> writes:

> On 3/13/19 11:28 AM, Markus Armbruster wrote:
>> Clean up includes so that osdep.h is included first and headers
>> which it implies are not included manually.
>> 
>> This commit was created with scripts/clean-includes, with the changes
>> to the following files manually reverted:
>
> Is it worth tweaking scripts/clean-includes as a pre-req patch so that
> some of these files become listed exceptions and don't need manual
> reversion here?

If we ran it more often than we do, probably yes.

>>     contrib/libvhost-user/libvhost-user-glib.h
>>     contrib/libvhost-user/libvhost-user.c
>>     contrib/libvhost-user/libvhost-user.h
>>     linux-user/mips64/cpu_loop.c
>>     linux-user/mips64/signal.c
>>     linux-user/sparc64/cpu_loop.c
>>     linux-user/sparc64/signal.c
>>     linux-user/x86_64/cpu_loop.c
>>     linux-user/x86_64/signal.c
>>     slirp/src/*
>>     target/s390x/gen-features.c
>>     tests/migration/s390x/a-b-bios.c
>>     tests/test-rcu-simpleq.c
>>     tests/test-rcu-tailq.c
>>     tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>> 
>> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
>> is guest software.  The remaining reverts are the same as in commit
>> b7d89466dde.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/contrib/elf2dmp/pdb.c
>> @@ -18,9 +18,8 @@
>>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>
> Unrelated, but if someone is looking for a project, cleaning up stale
> address references in older GPL copy-and-paste is a possibility.

Would be nice.

> I think these changes are okay for 4.0 soft freeze; but it also doesn't
> hurt if this delays into 4.1.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH] Clean up includes
Posted by Thomas Huth 5 years, 1 month ago
On 13/03/2019 17.28, Markus Armbruster wrote:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
> 
> This commit was created with scripts/clean-includes, with the changes
> to the following files manually reverted:
> 
>     contrib/libvhost-user/libvhost-user-glib.h
>     contrib/libvhost-user/libvhost-user.c
>     contrib/libvhost-user/libvhost-user.h
>     linux-user/mips64/cpu_loop.c
>     linux-user/mips64/signal.c
>     linux-user/sparc64/cpu_loop.c
>     linux-user/sparc64/signal.c
>     linux-user/x86_64/cpu_loop.c
>     linux-user/x86_64/signal.c
>     slirp/src/*
>     target/s390x/gen-features.c
>     tests/migration/s390x/a-b-bios.c
>     tests/test-rcu-simpleq.c
>     tests/test-rcu-tailq.c
>     tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
> 
> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
> is guest software.  The remaining reverts are the same as in commit
> b7d89466dde.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  contrib/elf2dmp/main.c   | 3 +--
>  contrib/elf2dmp/pdb.c    | 3 +--
>  hw/display/ati.c         | 1 +
>  hw/display/ati_2d.c      | 1 +
>  hw/display/ati_dbg.c     | 1 +
>  hw/display/ati_int.h     | 1 -
>  include/hw/cpu/cluster.h | 1 -
>  tests/fp/platform.h      | 1 -
>  tests/libqos/qgraph.h    | 4 ----
>  tests/qos-test.c         | 2 +-
>  10 files changed, 6 insertions(+), 12 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [Qemu-trivial] [PATCH] Clean up includes
Posted by Laurent Vivier 5 years ago
On 21/03/2019 10:21, Thomas Huth wrote:
> On 13/03/2019 17.28, Markus Armbruster wrote:
>> Clean up includes so that osdep.h is included first and headers
>> which it implies are not included manually.
>>
>> This commit was created with scripts/clean-includes, with the changes
>> to the following files manually reverted:
>>
>>     contrib/libvhost-user/libvhost-user-glib.h
>>     contrib/libvhost-user/libvhost-user.c
>>     contrib/libvhost-user/libvhost-user.h
>>     linux-user/mips64/cpu_loop.c
>>     linux-user/mips64/signal.c
>>     linux-user/sparc64/cpu_loop.c
>>     linux-user/sparc64/signal.c
>>     linux-user/x86_64/cpu_loop.c
>>     linux-user/x86_64/signal.c
>>     slirp/src/*
>>     target/s390x/gen-features.c
>>     tests/migration/s390x/a-b-bios.c
>>     tests/test-rcu-simpleq.c
>>     tests/test-rcu-tailq.c
>>     tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>>
>> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
>> is guest software.  The remaining reverts are the same as in commit
>> b7d89466dde.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  contrib/elf2dmp/main.c   | 3 +--
>>  contrib/elf2dmp/pdb.c    | 3 +--
>>  hw/display/ati.c         | 1 +
>>  hw/display/ati_2d.c      | 1 +
>>  hw/display/ati_dbg.c     | 1 +
>>  hw/display/ati_int.h     | 1 -
>>  include/hw/cpu/cluster.h | 1 -
>>  tests/fp/platform.h      | 1 -
>>  tests/libqos/qgraph.h    | 4 ----
>>  tests/qos-test.c         | 2 +-
>>  10 files changed, 6 insertions(+), 12 deletions(-)
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 


Applied to my trivial-patches branch.

Thanks,
Laurent