> -----Original Message----- > From: Dilger, Andreas [mailto:andreas.dilger@xxxxxxxxx] > Sent: Friday, May 31, 2013 7:11 AM > To: Greg Kroah-Hartman; Peng Tao > Cc: devel@xxxxxxxxxxxxxxxxxxxx; Peng, Tao > Subject: Re: [PATCH 4/4] staging/lustre: drop CONFIG_BROKEN dependency > > On 2013/30/05 4:21 PM, "Greg Kroah-Hartman" <gregkh@xxxxxxxxxxxxxxxxxxx> > wrote: > > >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... > > Yeah, I shouldn't have tried to fix things at 3 am when my brain cannot be trusted. :) > >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. > > This is a generic upcall that is used in the server code, and not by the > client > (which is the only part that has been submitted so far). It isn't unlikely > that the "obd_mount_server.c" file is needed for the client, since it was > explicitly split off from "obd_mount.c" for the upstream client > submission... > It is likely the whole file could just be deleted from the tree. > Thanks for the suggestion. In fact the file was not even built. It must have slipped in when I used scripts to copy all the source files. Greg, please find the attachment to fix the build error. Thank you for your help! Cheers, Tao
Attachment:
0001-staging-lustre-remove-enum-config_flags-and-obd_moun.patch
Description: 0001-staging-lustre-remove-enum-config_flags-and-obd_moun.patch
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
