| Message ID | 1455246507-5589-2-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: > in the keyhandler.h file. Otherwise on ARM builds if we > just use the keyhandler file - the compile will fail. > > CC: ian.campbell@citrix.com > CC: wei.liu2@citrix.com > CC: stefano.stabellini@citrix.com > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> See commit d1d181328d. Jan
On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote: > in the keyhandler.h file. Otherwise on ARM builds if we > just use the keyhandler file - the compile will fail. > > CC: ian.campbell@citrix.com > CC: wei.liu2@citrix.com > CC: stefano.stabellini@citrix.com > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/include/xen/keyhandler.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h > index 06c05c8..e361558 100644 > --- a/xen/include/xen/keyhandler.h > +++ b/xen/include/xen/keyhandler.h > @@ -19,6 +19,7 @@ > */ > typedef void (keyhandler_fn_t)(unsigned char key); > > +struct cpu_user_regs; > /* > * Callback type for irq_keyhandler. > * I think that the right fix would be to #include <asm/processor.h>.
>>> On 12.02.16 at 12:37, <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote: >> --- a/xen/include/xen/keyhandler.h >> +++ b/xen/include/xen/keyhandler.h >> @@ -19,6 +19,7 @@ >> */ >> typedef void (keyhandler_fn_t)(unsigned char key); >> >> +struct cpu_user_regs; >> /* >> * Callback type for irq_keyhandler. >> * > > I think that the right fix would be to #include <asm/processor.h>. I disagree - for one this isn't where the structure gets defined, and then this is the "include everything everywhere" attitude which tends to needlessly slow down builds (avoiding the need to include everything everywhere is actually one of the purposes of such forward declarations, which allows going as far as having the full definition in just a single source file). Jan
On Fri, 12 Feb 2016, Jan Beulich wrote: > >>> On 12.02.16 at 12:37, <stefano.stabellini@eu.citrix.com> wrote: > > On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote: > >> --- a/xen/include/xen/keyhandler.h > >> +++ b/xen/include/xen/keyhandler.h > >> @@ -19,6 +19,7 @@ > >> */ > >> typedef void (keyhandler_fn_t)(unsigned char key); > >> > >> +struct cpu_user_regs; > >> /* > >> * Callback type for irq_keyhandler. > >> * > > > > I think that the right fix would be to #include <asm/processor.h>. > > I disagree - for one this isn't where the structure gets defined, Ah, sorry, it is on ARM (actually to be precise, the header files are asm-arm/arm64/processor.h and asm-arm/arm32/processor.h, included by asm-arm/processor.h depending on the architecture). I see now that the corresponding x86 header is actually arch-x86/xen.h. > and then this is the "include everything everywhere" attitude > which tends to needlessly slow down builds (avoiding the need > to include everything everywhere is actually one of the > purposes of such forward declarations, which allows going as > far as having the full definition in just a single source file). Fair enough, but keyhandler.h directly uses struct cpu_user_regs as parameter in two functions, so I think it would be right to include an header file with the definition. It's too bad that the names for the ARM headers and the x86 headers don't match. Given that, I understand why Konrad went with this solution instead, and I am OK with it.
On Fri, Feb 12, 2016 at 01:51:28AM -0700, Jan Beulich wrote: > >>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: > > in the keyhandler.h file. Otherwise on ARM builds if we > > just use the keyhandler file - the compile will fail. > > > > CC: ian.campbell@citrix.com > > CC: wei.liu2@citrix.com > > CC: stefano.stabellini@citrix.com > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > See commit d1d181328d. Weird. When I did git rebase origin/staging it didn't notice that commit. And it just applied it on top - so I had two 'struct cpu_regs;' in the header file.
>>> On 12.02.16 at 13:46, <stefano.stabellini@eu.citrix.com> wrote: > On Fri, 12 Feb 2016, Jan Beulich wrote: >> and then this is the "include everything everywhere" attitude >> which tends to needlessly slow down builds (avoiding the need >> to include everything everywhere is actually one of the >> purposes of such forward declarations, which allows going as >> far as having the full definition in just a single source file). > > Fair enough, but keyhandler.h directly uses struct cpu_user_regs as > parameter in two functions, so I think it would be right to include an > header file with the definition. No, full definitions need only be visible when structure instances are used, not when just pointers to them get declared. The sole reason why the forward declaration is needed is because otherwise declaration and definition of the function wouldn't match, as without the forward declaration the scope of the structure is that of the function (instead of the global scope). Jan
>>> On 12.02.16 at 15:06, <konrad.wilk@oracle.com> wrote: > On Fri, Feb 12, 2016 at 01:51:28AM -0700, Jan Beulich wrote: >> >>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: >> > in the keyhandler.h file. Otherwise on ARM builds if we >> > just use the keyhandler file - the compile will fail. >> > >> > CC: ian.campbell@citrix.com >> > CC: wei.liu2@citrix.com >> > CC: stefano.stabellini@citrix.com >> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> >> See commit d1d181328d. > > Weird. When I did git rebase origin/staging it didn't > notice that commit. And it just applied it on top - so I had > two 'struct cpu_regs;' in the header file. That's likely because I moved the declaration down a few lines while applying. Jan
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h index 06c05c8..e361558 100644 --- a/xen/include/xen/keyhandler.h +++ b/xen/include/xen/keyhandler.h @@ -19,6 +19,7 @@ */ typedef void (keyhandler_fn_t)(unsigned char key); +struct cpu_user_regs; /* * Callback type for irq_keyhandler. *
in the keyhandler.h file. Otherwise on ARM builds if we just use the keyhandler file - the compile will fail. CC: ian.campbell@citrix.com CC: wei.liu2@citrix.com CC: stefano.stabellini@citrix.com Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/include/xen/keyhandler.h | 1 + 1 file changed, 1 insertion(+)