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...
>
>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.
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel High Performance Data Division
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
