Re: [PATCH] virt: acrn: Fix vCPU removing code build error

[Greg Kroah-Hartman]

On Fri, Feb 12, 2021 at 12:57:24PM +0800, shuo.a.liu@xxxxxxxxx wrote:
> From: Shuo Liu <shuo.a.liu@xxxxxxxxx>
> 
> vCPU removing code depends on CONFIG_HOTPLUG_CPU as it uses remove_cpu()
> and add_cpu(). Make the vCPU removing interface building with
> CONFIG_HOTPLUG_CPU.
> 
> ../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
> ../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function ‘remove_cpu’; [-Werror=implicit-function-declaration]
>    remove_cpu(cpu);
> 
> ../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function ‘add_cpu’; [-Werror=implicit-function-declaration]
>    add_cpu(cpu);
> 
> Fixes: 279dcf693ac7 ("virt: acrn: Introduce an interface for Service VM to control vCPU")
> Reported-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Acked-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx> # build-tested
> Signed-off-by: Shuo Liu <shuo.a.liu@xxxxxxxxx>
> ---
>  drivers/virt/acrn/hsm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
> index 1f6b7c54a1a4..e340788aacdf 100644
> --- a/drivers/virt/acrn/hsm.c
> +++ b/drivers/virt/acrn/hsm.c
> @@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_HOTPLUG_CPU
>  static ssize_t remove_cpu_store(struct device *dev,
>  				struct device_attribute *attr,
>  				const char *buf, size_t count)
> @@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
>  	return ret;
>  }
>  static DEVICE_ATTR_WO(remove_cpu);
> +#endif
>  
>  static struct attribute *acrn_attrs[] = {
> +#ifdef CONFIG_HOTPLUG_CPU
>  	&dev_attr_remove_cpu.attr,
> +#endif
>  	NULL
>  };
>  
> 

Shouldn't the real solution for this be that remove_cpu() and add_cpu()
have function prototypes for when this is not enabled in the kernel
build?

Putting #ifdef in .c files like this is not a good idea at all.

Then, at runtime, you can determine if you need to create this sysfs
file or not, as you do not want to expose it to userspace if the kernel
can not handle it, right?

thanks,

greg k-h