Re: [PATCH] vfio_pci: Add local source directory as include

[Alex Williamson]

On Mon, 7 Jan 2019 20:39:19 +0900
Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:

> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> >
> > On Mon, 7 Jan 2019 19:12:10 +0900
> > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
> >  
> > > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:  
> > > >
> > > > Laura Abbott <labbott@xxxxxxxxxx> writes:  
> > > > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > > > subdriver") introduced a trace.h file in the local directory but
> > > > > missed adding the local include path, resulting in compilation
> > > > > failures with tracepoints:
> > > > >
> > > > > In file included from drivers/vfio/pci/trace.h:102,
> > > > >                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > > > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > > >
> > > > > Fix this by adjusting the include path.
> > > > >
> > > > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > > > Signed-off-by: Laura Abbott <labbott@xxxxxxxxxx>  
> >
> > (...)
> >  
> > > > Alex I assume you'll merge this fix via the vfio tree?
> > > >
> > > > cheers
> > > >  
> > > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > > index 9662c063a6b1..08d4676a8495 100644
> > > > > --- a/drivers/vfio/pci/Makefile
> > > > > +++ b/drivers/vfio/pci/Makefile
> > > > > @@ -1,3 +1,4 @@
> > > > > +ccflags-y                               += -I$(src)
> > > > >
> > > > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > > --
> > > > > 2.20.1  
> > >
> > >
> > > Hi.
> > >
> > > If I correctly understand the usage of TRACE_INCLUDE_PATH,
> > > the correct fix should be like follows:
> > >
> > >
> > > diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> > > index 228ccdb..4d13e51 100644
> > > --- a/drivers/vfio/pci/trace.h
> > > +++ b/drivers/vfio/pci/trace.h
> > > @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> > >  #endif /* _TRACE_VFIO_PCI_H */
> > >
> > >  #undef TRACE_INCLUDE_PATH
> > > -#define TRACE_INCLUDE_PATH .
> > > +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> > >  #undef TRACE_INCLUDE_FILE
> > >  #define TRACE_INCLUDE_FILE trace  
> >
> > Going from the comments in samples/trace_events/trace-events-sample.h,
> > I think both approaches are possible, and I see both used in various
> > places.
> >
> > Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> > a path.

Numbering options for clarity:

1)
> ccflags-y += -I$(src)
> would add the header search path for all files in drivers/vfio/pci/
> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> 

2)
> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> is a bit better.
> However, it is not obvious why this extra header search path is needed
> until you find vfio_pci_nvlink2.c including trace.h
> 

3)
> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> clarifies the intention because the related code is all placed in trace.h
> 
> 
> 
> From the comment in include/trace/define_trace.h
> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h

In my scan of the tree, the most common solution seems to be 2) as this
is essentially recommended in the sample file.  3) is well represented,
with much fewer examples of 1), though it might depend how liberally
we grep out or examine the use cases.  Choice 1) also seems to be the
most shotgun approach, adding to the search path for all files.  From a
maintenance perspective I agree that 2) seems more error prone,
especially when the build system only catches the error on in-tree
builds, something I rarely do.  Therefore I'm leaning towards option
3).  The hardcoded path here doesn't seem much of an issue relative to
the negatives of the other approaches (how often do we move these
files?) and it keeps the trace support relatively self-contained.  Are
there further arguments for or against these options?  Otherwise who
wants to formally post the TRACE_INCLUDE_PATH version?  Thanks,

Alex