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

[Alexey Kardashevskiy]

On 08/01/2019 07:13, Alex Williamson wrote:
> 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.


The shotgun approach is always used when compiling out of tree which is
what many people do anyway without realizing that there are additional
-I. Why do not we make in-tree behave the same way? Thanks,


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

-- 
Alexey