From: Branden Bonaby <brandonbonaby94@xxxxxxxxx> Sent: Thursday, August 22, 2019 8:39 PM > > > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > > > index 09829e15d4a0..c9c63a4033cd 100644 > > > --- a/drivers/hv/connection.c > > > +++ b/drivers/hv/connection.c > > > @@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data) > > > > > > trace_vmbus_on_event(channel); > > > > > > +#ifdef CONFIG_HYPERV_TESTING > > > + hv_debug_delay_test(channel, INTERRUPT_DELAY); > > > +#endif /* CONFIG_HYPERV_TESTING */ > > > > You are following Vitaly's suggestion to use #ifdef's so no code is > > generated when HYPERV_TESTING is not enabled. However, this > > direct approach to using #ifdef's really clutters the code and makes > > it harder to read and follow. The better approach is to use the > > #ifdef in the include file where the functions are defined. If > > HYPERV_TESTING is not enabled, provide a #else that defines > > the function with an empty implementation for which the compiler > > will generate no code. An as example, see the function definition > > for hyperv_init() in arch/x86/include/asm/mshyperv.h. There are > > several functions treated similarly in that include file. > > > > I checked out the code in arch/x86/include/asm/mshyperv.h, after > thinking about it, I'm wondering if it would be better just to have > two files one called hv_debugfs.c and the other hyperv_debugfs.h. > I could put the code definitions in hv_debugfs.c and at the top > include the hyperv_debugfs.h file which would house the declarations > of these functions under the ifdef. Then like you alluded too use > an #else statement that would have the null implementations of the > above functions. Then put an #include "hyperv_debugfs.h" in the > hyperv_vmbus.h file. I figured instead of putting the code directly > into the vmbus_drv.c file it might be best to put them in a seperate > file like hv_debugfs.c. This way when we start adding more tests we > don't bloat the vmbus_drv.c file unnecessarily. The hv_debugfs.c > file would have the #ifdef CONFIG_HYPERV_TESTING at the top so if > its not enabled those null implementations in "hyperv_debugfs.h" > woud kick in anywhere that included the hyperv_vmbus.h file which > is what we want. > > what do you think? > I'll preface my comments by saying that how code gets structured into files is always a bit of a judgment call. The goal is to group code into sensible chunks to make it easy to understand and to make it easy to modify and extend later. The latter is a prediction about the future, which may or may not be accurate. For the former, what's "easy to understand," is often in the eye of the beholder. So you may get different opinions from different reviewers. That said, I like the idea of a separate hv_debugfs.c file to contain the implementation of the various functions you have added to provide the fuzzing capability. I'm less convinced about the value of a separate hyperv_debugfs.h file. I think you have one big #ifdef CONFIG_HYPERV_TESTING followed by the declarations of the functions in hv_debugfs.c, followed by #else and null implementations of those functions. This is 20 lines of code or so, and could easily go in hyperv_vmbus.h. For the new hv_debugfs.c, you can avoid the need for #ifdef CONFIG_HYPERV_TESTING by modifying the Makefile in drivers/hv so that hv_debugfs.o is built only if CONFIG_HYPERV_TESTING is defined. Look at the current Makefile to see how this is done with CONFIG_HYPERV_UTILS and CONFIG_HYPERV_BALLOON. Michael
