Re: [PATCH v3 4/5] Sysfs: Export VMCSINFO via sysfs

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jun 28, 2012 at 04:37:38AM -0700, Greg KH wrote:
> On Thu, Jun 28, 2012 at 05:54:30PM +0800, Yanfei Zhang wrote:
> > 于 2012年06月28日 03:22, Greg KH 写道:
> > > On Wed, Jun 27, 2012 at 04:54:54PM +0800, Yanfei Zhang wrote:
> > >> This patch export offsets of fields via /sys/devices/cpu/vmcs/.
> > >> Individual offsets are contained in subfiles named by the filed's
> > >> encoding, e.g.: /sys/devices/cpu/vmcs/0800
> > >>
> > >> Signed-off-by: zhangyanfei <zhangyanfei@cn.fujitsu.com>
> > >> ---
> > >>  drivers/base/core.c |   13 +++++++++++++
> > >>  1 files changed, 13 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> > >> index 346be8b..dd05ee7 100644
> > >> --- a/drivers/base/core.c
> > >> +++ b/drivers/base/core.c
> > >> @@ -26,6 +26,7 @@
> > >>  #include <linux/async.h>
> > >>  #include <linux/pm_runtime.h>
> > >>  #include <linux/netdevice.h>
> > >> +#include <asm/vmcsinfo.h>
> > > 
> > > Did you just break the build on all other arches?  Not nice.
> > > 
> > >> @@ -1038,6 +1039,11 @@ int device_add(struct device *dev)
> > >>  	error = dpm_sysfs_add(dev);
> > >>  	if (error)
> > >>  		goto DPMError;
> > >> +#if defined(CONFIG_KVM_INTEL) || defined(CONFIG_KVM_INTEL_MODULE)
> > >> +	error = vmcs_sysfs_add(dev);
> > >> +	if (error)
> > >> +		goto VMCSError;
> > >> +#endif
> > > 
> > > Oh my no, that's no way to ever do this, you know better than that,
> > > please fix.
> > > 
> > > greg k-h
> > > 
> > 
> > Sorry for my thoughtless, Here is the new patch.
> > 
> > ---
> >  drivers/base/core.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 346be8b..7b5266a 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -30,6 +30,13 @@
> >  #include "base.h"
> >  #include "power/power.h"
> >  
> > +#if defined(CONFIG_KVM_INTEL) || defined(CONFIG_KVM_INTEL_MODULE)
> > +#include <asm/vmcsinfo.h>
> > +#else
> > +static inline int vmcs_sysfs_add(struct device *dev) { return 0; }
> > +static inline void vmcs_sysfs_remove(struct device *dev) { }
> > +#endif
> 
> {sigh}  No, again, you know better, don't do this.

Ok, as others have rightly pointed out, this wasn't the most helpful
review comment, sorry about that.

In Linux, we don't put ifdefs in .c files, we put them in .h files.  See
many examples of this all over the place.  That's my main complaints the
past two times of this patch.

But, for this, I would question why you even want / need to do this in
the drivers/base/core/ file in the first place.  Shouldn't it be in some
arch or cpu specific file instead that already handles the cpu files?

thanks,

greg k-h