On Fri, May 31, 2013 at 03:01:20AM +0800, Peng Tao wrote:
> On 05/30/2013 07:54 PM, Greg Kroah-Hartman wrote:
> >On Wed, May 29, 2013 at 09:40:57PM +0800, Peng Tao wrote:
> >>Signed-off-by: Peng Tao <tao.peng@xxxxxxx>
> >>Signed-off-by: Andreas Dilger <andreas.dilger@xxxxxxxxx>
> >>---
> >> drivers/staging/lustre/lustre/Kconfig | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >Nope, I can't take this, it still fails to build on my machine, please
> >test things better, you CAN NOT break the build.
> >
> >Here's the first build error, it's as if you didn't even test this...
> >
> > CC [M] drivers/staging/lustre/lustre/fid/fid_handler.o
> >In file included from drivers/staging/lustre/lustre/fid/fid_handler.c:48:0:
> >drivers/staging/lustre/lustre/fid/../include/obd.h:914:2: error: expected identifier before numeric constant
> oops... sorry, but it builds on my machine. I've been playing with
> the four patches for two weeks and never saw this error. And line
> 914 in question looks OK as well:
> 912 enum config_flags {
> 913 CONFIG_LOG = 0x1, /* finished processing config log */
> 914 CONFIG_SYNC = 0x2, /* mdt synced 1 ost */
> 915 CONFIG_TARGET = 0x4 /* one target is added */
Having enumerated types with "CONFIG_" to start with, in a kernel
driver, is pretty foolish, don't you agree? :)
> 916 };
>
> Do you happen to have CONFIG_SYNC defined somewhere?
Yes, in the staging-next tree, where this driver also lives.
> If that is the
> case, I will need to rename it with LUSTRE prefix. However, I did
> "find . -name "*.[c,h]" | xargs grep -n CONFIG_SYNC" in staging-next
> branch but couldn't find CONFIG_SYNC defined anywhere. So I'm not
> sure if it is indeed the case. Could you please confirm if the
> attached patch fixes it? Thanks a lot!
You can't grep for a "CONFIG_" prefix in a Kconfig file...
Yes, the patch should fix it, but:
> >From 530fc131922fa6d18b72511e133cabc47e209950 Mon Sep 17 00:00:00 2001
> From: Peng Tao <bergwolf@xxxxxxxxx>
> Date: Fri, 31 May 2013 02:54:17 +0800
> Subject: [PATCH] staging/lustre: add LUSTRE prefix to config_flags
>
> It seems that we might conflic with global macros defined
> somewhere else by others... Add LUSTRE prefix to avoid it.
>
> Signed-off-by: Peng Tao <tao.peng@xxxxxxx>
> ---
> drivers/staging/lustre/lustre/include/obd.h | 6 +++---
> .../lustre/lustre/obdclass/obd_mount_server.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/obd.h b/drivers/staging/lustre/lustre/include/obd.h
> index 98fdb32..9e24861 100644
> --- a/drivers/staging/lustre/lustre/include/obd.h
> +++ b/drivers/staging/lustre/lustre/include/obd.h
> @@ -910,9 +910,9 @@ enum obd_notify_event {
>
> /* bit-mask flags for config events */
> enum config_flags {
> - CONFIG_LOG = 0x1, /* finished processing config log */
> - CONFIG_SYNC = 0x2, /* mdt synced 1 ost */
> - CONFIG_TARGET = 0x4 /* one target is added */
> + LUSTRE_CONFIG_LOG = 0x1, /* finished processing config log */
> + LUSTRE_CONFIG_SYNC = 0x2, /* mdt synced 1 ost */
> + LUSTRE_CONFIG_TARGET = 0x4 /* one target is added */
Great, you rename the enums, however:
> };
>
> /*
> diff --git a/drivers/staging/lustre/lustre/obdclass/obd_mount_server.c b/drivers/staging/lustre/lustre/obdclass/obd_mount_server.c
> index a3a4409..3e4a4f3 100644
> --- a/drivers/staging/lustre/lustre/obdclass/obd_mount_server.c
> +++ b/drivers/staging/lustre/lustre/obdclass/obd_mount_server.c
> @@ -1279,7 +1279,7 @@ static int server_start_targets(struct super_block *sb, struct vfsmount *mnt)
> server_calc_timeout(lsi, obd);
>
> /* log has been fully processed */
> - obd_notify(obd, NULL, OBD_NOTIFY_CONFIG, (void *)CONFIG_LOG);
> + obd_notify(obd, NULL, OBD_NOTIFY_CONFIG, (void *)LUSTRE_CONFIG_LOG);
You only really need one of them, and then you are just passing it to a
function that obviously doesn't do anything with it otherwise it would
also be referencing the enumerated type.
And a void *? Seriously?
Fix this up properly (i.e. delete the enums not being used, and don't
use void * for something like this), and I'll be glad to take it.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
