Re: [PATCH 6/4] [-mm patch] use the existing offsetof().
Hi Satyam,
Satyam Sharma wrote:
> > BTW I don't think these macros are such a big win in readability or
> > code clarity anyway. And I noticed that there are still some open
> > calls to vmcoreinfo_append_str() in kexec.c (such as for the OS_RELEASE
> > case) which you haven't macro-ized ... we end up having code that looks
> > like:
> >
> > vmcoreinfo_append_str(...);
> > ...
> >
> > ...
> > VMCOREINFO_OFFSET(...);
> > ...
> >
> > and it's not even obvious (unless you follow on to the definition of the
> > later macro) that the above two are *exactly* the same. So you could also
> > consider just losing the macros altogether.
I want to use these macros for calls to vmcoreinfo_append_str(), because
the strings included in each macro ("SIZE(XXX)=", "OFFSET(XXX)=", etc.)
are very important. makedumpfile command reads these strings and gets the
debugging information (structure sizes, field offsets, etc.). If losing
these macros and there is a typo like "OFESET(page.mapping)=", the command
cannot run. To prevent this mistake, these macros are very useful.
For more readability, I think it is good that all the calls are changed
to macros having a prefix "VMCOREINFO_" like the attached patch.
Thanks
Ken'ichi Ohmichi
---
Signed-off-by: Ken'ichi Ohmichi <[EMAIL PROTECTED]>
---
diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h
--- a/include/linux/kexec.h 2007-09-21 09:28:23.0 +0900
+++ b/include/linux/kexec.h 2007-09-21 10:26:24.0 +0900
@@ -127,6 +127,10 @@ void vmcoreinfo_append_str(const char *f
__attribute__ ((format (printf, 1, 2)));
unsigned long paddr_vmcoreinfo_note(void);
+#define VMCOREINFO_OSRELEASE(name) \
+ vmcoreinfo_append_str("OSRELEASE=%s\n", #name)
+#define VMCOREINFO_PAGESIZE(value) \
+ vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
#define VMCOREINFO_SYMBOL(name) \
vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned long))
#define VMCOREINFO_SIZE(name) \
@@ -134,8 +138,8 @@ unsigned long paddr_vmcoreinfo_note(void
#define VMCOREINFO_STRUCT_SIZE(name) \
vmcoreinfo_append_str("SIZE(%s)=%zu\n", #name, sizeof(struct name))
#define VMCOREINFO_OFFSET(name, field) \
- vmcoreinfo_append_str("OFFSET(%s.%s)=%lu\n", #name, #field, \
- (unsigned long)&(((struct name *)0)->field))
+ vmcoreinfo_append_str("OFFSET(%s.%s)=%zu\n", #name, #field, \
+ offsetof(struct name, field))
#define VMCOREINFO_LENGTH(name, value) \
vmcoreinfo_append_str("LENGTH(%s)=%lu\n", #name, (unsigned long)value)
#define VMCOREINFO_NUMBER(name) \
diff -rpuN a/kernel/kexec.c b/kernel/kexec.c
--- a/kernel/kexec.c2007-09-21 09:28:22.0 +0900
+++ b/kernel/kexec.c2007-09-21 10:17:56.0 +0900
@@ -1195,8 +1195,8 @@ unsigned long __attribute__ ((weak)) pad
static int __init crash_save_vmcoreinfo_init(void)
{
- vmcoreinfo_append_str("OSRELEASE=%s\n", init_uts_ns.name.release);
- vmcoreinfo_append_str("PAGESIZE=%ld\n", PAGE_SIZE);
+ VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
+ VMCOREINFO_PAGESIZE(PAGE_SIZE);
VMCOREINFO_SYMBOL(init_uts_ns);
VMCOREINFO_SYMBOL(node_online_map);
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/4] [-mm patch] use the existing offsetof().
Hi Satyam,
Satyam Sharma wrote:
BTW I don't think these macros are such a big win in readability or
code clarity anyway. And I noticed that there are still some open
calls to vmcoreinfo_append_str() in kexec.c (such as for the OS_RELEASE
case) which you haven't macro-ized ... we end up having code that looks
like:
vmcoreinfo_append_str(...);
...
...
VMCOREINFO_OFFSET(...);
...
and it's not even obvious (unless you follow on to the definition of the
later macro) that the above two are *exactly* the same. So you could also
consider just losing the macros altogether.
I want to use these macros for calls to vmcoreinfo_append_str(), because
the strings included in each macro (SIZE(XXX)=, OFFSET(XXX)=, etc.)
are very important. makedumpfile command reads these strings and gets the
debugging information (structure sizes, field offsets, etc.). If losing
these macros and there is a typo like OFESET(page.mapping)=, the command
cannot run. To prevent this mistake, these macros are very useful.
For more readability, I think it is good that all the calls are changed
to macros having a prefix VMCOREINFO_ like the attached patch.
Thanks
Ken'ichi Ohmichi
---
Signed-off-by: Ken'ichi Ohmichi [EMAIL PROTECTED]
---
diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h
--- a/include/linux/kexec.h 2007-09-21 09:28:23.0 +0900
+++ b/include/linux/kexec.h 2007-09-21 10:26:24.0 +0900
@@ -127,6 +127,10 @@ void vmcoreinfo_append_str(const char *f
__attribute__ ((format (printf, 1, 2)));
unsigned long paddr_vmcoreinfo_note(void);
+#define VMCOREINFO_OSRELEASE(name) \
+ vmcoreinfo_append_str(OSRELEASE=%s\n, #name)
+#define VMCOREINFO_PAGESIZE(value) \
+ vmcoreinfo_append_str(PAGESIZE=%ld\n, value)
#define VMCOREINFO_SYMBOL(name) \
vmcoreinfo_append_str(SYMBOL(%s)=%lx\n, #name, (unsigned long)name)
#define VMCOREINFO_SIZE(name) \
@@ -134,8 +138,8 @@ unsigned long paddr_vmcoreinfo_note(void
#define VMCOREINFO_STRUCT_SIZE(name) \
vmcoreinfo_append_str(SIZE(%s)=%zu\n, #name, sizeof(struct name))
#define VMCOREINFO_OFFSET(name, field) \
- vmcoreinfo_append_str(OFFSET(%s.%s)=%lu\n, #name, #field, \
- (unsigned long)(((struct name *)0)-field))
+ vmcoreinfo_append_str(OFFSET(%s.%s)=%zu\n, #name, #field, \
+ offsetof(struct name, field))
#define VMCOREINFO_LENGTH(name, value) \
vmcoreinfo_append_str(LENGTH(%s)=%lu\n, #name, (unsigned long)value)
#define VMCOREINFO_NUMBER(name) \
diff -rpuN a/kernel/kexec.c b/kernel/kexec.c
--- a/kernel/kexec.c2007-09-21 09:28:22.0 +0900
+++ b/kernel/kexec.c2007-09-21 10:17:56.0 +0900
@@ -1195,8 +1195,8 @@ unsigned long __attribute__ ((weak)) pad
static int __init crash_save_vmcoreinfo_init(void)
{
- vmcoreinfo_append_str(OSRELEASE=%s\n, init_uts_ns.name.release);
- vmcoreinfo_append_str(PAGESIZE=%ld\n, PAGE_SIZE);
+ VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
+ VMCOREINFO_PAGESIZE(PAGE_SIZE);
VMCOREINFO_SYMBOL(init_uts_ns);
VMCOREINFO_SYMBOL(node_online_map);
_
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/4] [-mm patch] use the existing offsetof().
On Thu, 20 Sep 2007, Ken'ichi Ohmichi wrote:
>
> [PATCH 6/4] [-mm patch] use the existing offsetof().
> It is better that offsetof() is used for VMCOREINFO_OFFSET().
> This idea is Joe Perches's.
>
>
>
> Thanks
> Ken'ichi Ohmichi
>
> ---
> Signed-off-by: Joe Perches <[EMAIL PROTECTED]>
Same deal here ...
> Signed-off-by: Ken'ichi Ohmichi <[EMAIL PROTECTED]>
> ---
> diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h
> --- a/include/linux/kexec.h 2007-09-18 15:23:22.0 +0900
> +++ b/include/linux/kexec.h 2007-09-18 15:27:29.0 +0900
> @@ -137,7 +137,7 @@ unsigned long paddr_vmcoreinfo_note(void
> (unsigned long)sizeof(struct name))
> #define VMCOREINFO_OFFSET(name, field) \
> vmcoreinfo_append_str("OFFSET(%s.%s)=%lu\n", #name, #field, \
> - (unsigned long)&(((struct name *)0)->field))
> + (unsigned long)offsetof(struct name, field))
> #define VMCOREINFO_LENGTH(name, value) \
> vmcoreinfo_append_str("LENGTH(%s)=%lu\n", #name, (unsigned long)value)
... and here. Use %zu and lose the casts.
BTW I don't think these macros are such a big win in readability or
code clarity anyway. And I noticed that there are still some open
calls to vmcoreinfo_append_str() in kexec.c (such as for the OS_RELEASE
case) which you haven't macro-ized ... we end up having code that looks
like:
vmcoreinfo_append_str(...);
...
...
VMCOREINFO_OFFSET(...);
...
and it's not even obvious (unless you follow on to the definition of the
later macro) that the above two are *exactly* the same. So you could also
consider just losing the macros altogether.
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/4] [-mm patch] use the existing offsetof().
On Thu, 20 Sep 2007, Ken'ichi Ohmichi wrote: [PATCH 6/4] [-mm patch] use the existing offsetof(). It is better that offsetof() is used for VMCOREINFO_OFFSET(). This idea is Joe Perches's. Thanks Ken'ichi Ohmichi --- Signed-off-by: Joe Perches [EMAIL PROTECTED] Same deal here ... Signed-off-by: Ken'ichi Ohmichi [EMAIL PROTECTED] --- diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h --- a/include/linux/kexec.h 2007-09-18 15:23:22.0 +0900 +++ b/include/linux/kexec.h 2007-09-18 15:27:29.0 +0900 @@ -137,7 +137,7 @@ unsigned long paddr_vmcoreinfo_note(void (unsigned long)sizeof(struct name)) #define VMCOREINFO_OFFSET(name, field) \ vmcoreinfo_append_str(OFFSET(%s.%s)=%lu\n, #name, #field, \ - (unsigned long)(((struct name *)0)-field)) + (unsigned long)offsetof(struct name, field)) #define VMCOREINFO_LENGTH(name, value) \ vmcoreinfo_append_str(LENGTH(%s)=%lu\n, #name, (unsigned long)value) ... and here. Use %zu and lose the casts. BTW I don't think these macros are such a big win in readability or code clarity anyway. And I noticed that there are still some open calls to vmcoreinfo_append_str() in kexec.c (such as for the OS_RELEASE case) which you haven't macro-ized ... we end up having code that looks like: vmcoreinfo_append_str(...); ... ... VMCOREINFO_OFFSET(...); ... and it's not even obvious (unless you follow on to the definition of the later macro) that the above two are *exactly* the same. So you could also consider just losing the macros altogether. Satyam - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/4] [-mm patch] use the existing offsetof().
[PATCH 6/4] [-mm patch] use the existing offsetof().
It is better that offsetof() is used for VMCOREINFO_OFFSET().
This idea is Joe Perches's.
Thanks
Ken'ichi Ohmichi
---
Signed-off-by: Joe Perches <[EMAIL PROTECTED]>
Signed-off-by: Ken'ichi Ohmichi <[EMAIL PROTECTED]>
---
diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h
--- a/include/linux/kexec.h 2007-09-18 15:23:22.0 +0900
+++ b/include/linux/kexec.h 2007-09-18 15:27:29.0 +0900
@@ -137,7 +137,7 @@ unsigned long paddr_vmcoreinfo_note(void
(unsigned long)sizeof(struct name))
#define VMCOREINFO_OFFSET(name, field) \
vmcoreinfo_append_str("OFFSET(%s.%s)=%lu\n", #name, #field, \
- (unsigned long)&(((struct name *)0)->field))
+ (unsigned long)offsetof(struct name, field))
#define VMCOREINFO_LENGTH(name, value) \
vmcoreinfo_append_str("LENGTH(%s)=%lu\n", #name, (unsigned long)value)
#define VMCOREINFO_NUMBER(name) \
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/4] [-mm patch] use the existing offsetof().
[PATCH 6/4] [-mm patch] use the existing offsetof(). It is better that offsetof() is used for VMCOREINFO_OFFSET(). This idea is Joe Perches's. Thanks Ken'ichi Ohmichi --- Signed-off-by: Joe Perches [EMAIL PROTECTED] Signed-off-by: Ken'ichi Ohmichi [EMAIL PROTECTED] --- diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h --- a/include/linux/kexec.h 2007-09-18 15:23:22.0 +0900 +++ b/include/linux/kexec.h 2007-09-18 15:27:29.0 +0900 @@ -137,7 +137,7 @@ unsigned long paddr_vmcoreinfo_note(void (unsigned long)sizeof(struct name)) #define VMCOREINFO_OFFSET(name, field) \ vmcoreinfo_append_str(OFFSET(%s.%s)=%lu\n, #name, #field, \ - (unsigned long)(((struct name *)0)-field)) + (unsigned long)offsetof(struct name, field)) #define VMCOREINFO_LENGTH(name, value) \ vmcoreinfo_append_str(LENGTH(%s)=%lu\n, #name, (unsigned long)value) #define VMCOREINFO_NUMBER(name) \ _ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
