Taking the Min and Max macro job

It seems that no one is assigned the Min and Max macro job. I'll take it if no one seems interested.

Well, I hope this to become my last brain-dead patch series :).

Quoted 4 lines · Ahmed S. Darwish

It seems that no one is assigned the Min and Max macro job. I'll take it if no one seems interested.

Well, I hope this to become my last brain-dead patch series :).

Be very very careful to not add integer overflows.

Quoted 3 lines · Ahmed S. Darwish

It seems that no one is assigned the Min and Max macro job. I'll take it if no one seems interested.

Quoted 1 line

Well, I hope this to become my last brain-dead patch series :).

Be very very careful to not add integer overflows.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Quoted 7 lines · Alexey Dobriyan

Quoted 4 lines · Ahmed S. Darwish

It seems that no one is assigned the Min and Max macro job. I'll take it if no one seems interested.

Well, I hope this to become my last brain-dead patch series :).

Be very very careful to not add integer overflows.

Thanks, I'll keep that in mind.

--
Ahmed S. Darwish
http://darwish-07.blogspot.com
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Quoted 7 lines · Alexey Dobriyan

Quoted 4 lines · Ahmed S. Darwish

It seems that no one is assigned the Min and Max macro job. I'll take it if no one seems interested.

Well, I hope this to become my last brain-dead patch series :).

Quoted 2 lines

Be very very careful to not add integer overflows.

I had started the activity. changed the handcrafted MAX macro to *max* from the kernel.h in the xdr4.c file ( no quite sure of the complete file name)

the max macro is applied in struct , i changed it and recompile failed.

Quoted 6 lines · Mehul Jani

Quoted 5 lines · Alexey Dobriyan

Quoted 3 lines · Ahmed S. Darwish

It seems that no one is assigned the Min and Max macro job. I'll take it if no one seems interested.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Well, I hope this to become my last brain-dead patch series :).

Be very very careful to not add integer overflows.

I had started the activity. changed the handcrafted MAX macro to *max*

from

Quoted 1 line

the kernel.h in the xdr4.c file ( no quite sure of the complete file

name)

Quoted 2 lines

the max macro is applied in struct , i changed it and recompile failed.

Hi Mehul,

(Please use reply-to-all, and let the quoted text be on the top not on the bottom)

Will you continue the task for the whole tree ?.

--
Ahmed S. Darwish
http://darwish-07.blogspot.com

I will do that. And let you know the progress .....

I will do that. And let you know the progress .....

Thanks for pointing out for *reply all*

Mehul

On 2/8/07, Ahmed S. Darwish <[email protected]> wrote:

Quoted 9 lines

I will do that. And let you know the progress .....

Thanks for pointing out for *reply all*

Mehul

On 2/8/07, Ahmed S. Darwish <[email protected]> wrote:

...

don't top-post. always put your reply *after* the text that you're responding to.

*and* delete any text that isn't relevant to your reply.

rday

Quoted 17 lines

I will do that. And let you know the progress .....

Quoted 7 lines

Thanks for pointing out for *reply all*

Mehul

On 2/8/07, Ahmed S. Darwish <[email protected]> wrote:

...

don't top-post. always put your reply *after* the text that you're responding to.

*and* delete any text that isn't relevant to your reply.

Quoted 1 line

rday

Make sense .. will take care from this time ;-)

Mehul

xdr4.c

Mehul like I said...xdr4.c needs a simple macro that does not check type at all.

If my understanding of the min/max use is correct then they use default type checking and min_t/max_t use the type that 'we' specify...so end result-they are both bound to type-meaning they would both have statements in expression-resulting in compilation failure as xdr*.c uses them inside struct initializations.

But the macro that would work in xdr*.c would look something like the below:

#define min_s(x,y) ((x)>(y)?(x):(y))

But with the above we need to make sure that x and y are not expressions if we dont want them to be evaluated twice and hence cause other probs ;)

So either we modify min/max in kernel.h - this means a lot more cleanup or add another macro say min_s/max_s --- s for simple :)

Unless we don't do that, it may not be possible to cleanup files like xdr*.c

Quoted 6 lines · Vignesh Babu BM

Mehul like I said...xdr4.c needs a simple macro that does not check type at all.

But the macro that would work in xdr*.c would look something like the below:

Quoted 9 lines

#define min_s(x,y) ((x)>(y)?(x):(y))

So either we modify min/max in kernel.h - this means a lot more cleanup or add another macro say min_s/max_s --- s for simple :)

Unless we don't do that, it may not be possible to cleanup files like xdr*.c

What about __min/__max? At least I usually read the '__'-prefix as something that is a subset of something (and most likely less secure). I wrote about this in "[KJ] [RFC] Regarding min/max" (07-02-03) bit since I was not able to find an example I took it as a mistake on my part.

Richard Knutsson

Quoted 6 lines · Vignesh Babu BM

Mehul like I said...xdr4.c needs a simple macro that does not check type at all.

But the macro that would work in xdr*.c would look something like the below:

Quoted 8 lines

#define min_s(x,y) ((x)>(y)?(x):(y))

So either we modify min/max in kernel.h - this means a lot more cleanup or add another macro say min_s/max_s --- s for simple :)

Unless we don't do that, it may not be possible to cleanup files like xdr*.c

What about __min/__max? At least I usually read the '__'-prefix as something that is a subset of something (and most likely less secure). I wrote about this in "[KJ] [RFC] Regarding min/max" (07-02-03) bit since I was not able to find an example I took it as a mistake on my part.

Richard Knutsson

Just went through the archives....

I tried to recreate the compile-failure (was some time ago since it
happened) when using min()/max() in a struct but it happily compiled it so (thankfully) you are correct, just need to use min()/max(). :)

Richard Knutsson

Do you mean to say that you were able to compile when the macros were used inside struct? Im still not able to get that to happen...

I tried that change in linux-2.6/fs/lockd/mon.c

static struct rpc_procinfo nsm_procedures[] = {

[SM_MON] = {
		.p_proc		= SM_MON,
		.p_encode	= (kxdrproc_t) xdr_encode_mon,
		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
		.p_bufsiz	= max(SM_mon_sz, SM_monres_sz) << 2,
		.p_statidx	= SM_MON,
		.p_name		= "MONITOR",
	},
[SM_UNMON] = {

		.p_proc		= SM_UNMON,
		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
		.p_decode	= (kxdrproc_t) xdr_decode_stat,
		.p_bufsiz	= max(SM_mon_id_sz, SM_unmonres_sz) << 2,
		.p_statidx	= SM_UNMON,
		.p_name		= "UNMONITOR",
	},
};

Quoted 24 lines · Vignesh Babu BM

Just went through the archives....

I tried to recreate the compile-failure (was some time ago since it
happened) when using min()/max() in a struct but it happily compiled it so (thankfully) you are correct, just need to use min()/max(). :)

Richard Knutsson

Do you mean to say that you were able to compile when the macros were used inside struct? Im still not able to get that to happen...

I tried that change in linux-2.6/fs/lockd/mon.c

static struct rpc_procinfo nsm_procedures[] = {

[SM_MON] = {
		.p_proc		= SM_MON,
		.p_encode	= (kxdrproc_t) xdr_encode_mon,
		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
		.p_bufsiz	= max(SM_mon_sz, SM_monres_sz) << 2,
		.p_statidx	= SM_MON,
		.p_name		= "MONITOR",
	},
[SM_UNMON] = {

Quoted 9 lines

		.p_proc		= SM_UNMON,
		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
		.p_decode	= (kxdrproc_t) xdr_decode_stat,
		.p_bufsiz	= max(SM_mon_id_sz, SM_unmonres_sz) << 2,
		.p_statidx	= SM_UNMON,
		.p_name		= "UNMONITOR",
	},
};

No, I "recreated" it by making a small test-file where I used min()/max() in a struct, and it worked without a pip from the compiler. Can not say I'm sure what the difference were, but xdr4.c and mon.c didn't seem to like it.

What do you say of something like this below? (It is not tested)

(Signed-off-by:) Richard Knutsson
---

include/linux/kernel.h +10 -2

index 63fb18d..0533b2d 100644
@@ -252,6 +252,14 @@ static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char *
 #error "Please fix asm/byteorder.h"
 #endif /* __LITTLE_ENDIAN */
 
+
+/*
+ * __min()/__max() is unsafe.
+ * Use min()/max() when available.
+ */
+#define __min(x,y)	((x) < (y) ? (x) : (y))
+#define __max(x,y)	((x) > (y) ? (x) : (y))
+
 /*
  * min()/max() macros that also do
  * strict type-checking.. See the

@@ -261,13 +269,13 @@ static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char *
 	typeof(x) _x = (x);	\
 	typeof(y) _y = (y);	\
 	(void) (&_x = &_y);		\
-	_x < _y ? _x : _y; })
+	__min(_x, _y); })
 
 #define max(x,y) ({ \
 	typeof(x) _x = (x);	\
 	typeof(y) _y = (y);	\
 	(void) (&_x = &_y);		\
-	_x > _y ? _x : _y; })
+	__max(_x, _y); })
 
 /*
  * ..and if you can't take the strict

@@ -276,9 +284,9 @@ static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char *
  * Or not use min/max at all, of course.

Quoted 14 lines · Richard Knutsson

Quoted 12 lines · Vignesh Babu BM

Just went through the archives....

I tried to recreate the compile-failure (was some time ago since it
happened) when using min()/max() in a struct but it happily compiled it so (thankfully) you are correct, just need to use min()/max(). :)

Richard Knutsson

Do you mean to say that you were able to compile when the macros were used inside struct? Im still not able to get that to happen...

Quoted 8 lines

Quoted 8 lines

I tried that change in linux-2.6/fs/lockd/mon.c

static struct rpc_procinfo nsm_procedures[] = {

[SM_MON] = {
		.p_proc		= SM_MON,
		.p_encode	= (kxdrproc_t) xdr_encode_mon,
		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
		.p_bufsiz	= max(SM_mon_sz, SM_monres_sz) << 2,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Just went through the archives....

I tried to recreate the compile-failure (was some time ago

since

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

it
happened) when using min()/max() in a struct but it happily

compiled it

Quoted 1 line

Quoted 1 line

Quoted 1 line

so (thankfully) you are correct, just need to use min()/max().

:)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Richard Knutsson

Do you mean to say that you were able to compile when the macros were

used inside struct?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Im still not able to get that to happen...

I tried that change in linux-2.6/fs/lockd/mon.c

static struct rpc_procinfo nsm_procedures[] = {
[SM_MON] = {

            .p_proc         = SM_MON,
            .p_encode       = (kxdrproc_t) xdr_encode_mon,
            .p_decode       = (kxdrproc_t) xdr_decode_stat_res,
            .p_bufsiz       = max(SM_mon_sz, SM_monres_sz) << 2,

Will work since both SM_mon_sz and SM_monres_sz are compile-time constants, not true?

[ ... snip ... ]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Just went through the archives....

I tried to recreate the compile-failure (was some time

ago since
>>> it
>>> happened) when using min()/max() in a struct but it
happily compiled it
>>> so (thankfully) you are correct, just need to use
min()/max(). :)
>>>
>>> Richard Knutsson
>>>
>>>Do you mean to say that you were able to compile when the macros were used inside struct?
>>>Im still not able to get that to happen...
>>>
>>>I tried that change in linux-2.6/fs/lockd/mon.c >>>
>>>static struct rpc_procinfo nsm_procedures[] = {
>>>[SM_MON] = {
>>> .p_proc = SM_MON,
>>> .p_encode = (kxdrproc_t) xdr_encode_mon,
>>> .p_decode = (kxdrproc_t) xdr_decode_stat_res,
>>> .p_bufsiz = max(SM_mon_sz, SM_monres_sz) << 2,

Will work since both SM_mon_sz and SM_monres_sz are compile-time constants, not true?

[ ... snip ... ]

So...

The patch actually modifies the macro's last line in kernel.h

(quoted diff) +1 -1

-       _x < _y ? _x : _y; })
+       __min(_x, _y); })

But what I am unable to understand is why is the compiler unhappy with
* _x < _y ? _x : _y; })*

and happy with new macro been called internally *__min(_x,_y);*

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

I tried that change in linux-2.6/fs/lockd/mon.c

static struct rpc_procinfo nsm_procedures[] = {
[SM_MON] = {

            .p_proc         = SM_MON,
            .p_encode       = (kxdrproc_t) xdr_encode_mon,
            .p_decode       = (kxdrproc_t) xdr_decode_stat_res,
            .p_bufsiz       = max(SM_mon_sz, SM_monres_sz) << 2,

Will work since both SM_mon_sz and SM_monres_sz are compile-time constants, not true?

[ ... snip ... ]

So...

The patch actually modifies the macro's last line in kernel.h

(quoted diff) +1 -1

-       _x < _y ? _x : _y; })
+       __min(_x, _y); })

But what I am unable to understand is why is the compiler unhappy with
* _x < _y ? _x : _y; })*

and happy with new macro been called internally *__min(_x,_y);*

Quoted 25 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I tried that change in linux-2.6/fs/lockd/mon.c

static struct rpc_procinfo nsm_procedures[] = {
[SM_MON] = {

            .p_proc         = SM_MON,
            .p_encode       = (kxdrproc_t) xdr_encode_mon,
            .p_decode       = (kxdrproc_t) xdr_decode_stat_res,
            .p_bufsiz       = max(SM_mon_sz, SM_monres_sz) << 2,

Will work since both SM_mon_sz and SM_monres_sz are compile-time constants, not true?

[ ... snip ... ]

So...

The patch actually modifies the macro's last line in kernel.h

(quoted diff) +1 -1

-       _x < _y ? _x : _y; })
+       __min(_x, _y); })

But what I am unable to understand is why is the compiler unhappy with
* _x < _y ? _x : _y; })*

Quoted 1 line

and happy with new macro been called internally *__min(_x,_y);*

Ok, _now_ I see. I used the xdr4.c but mon.c seems happy with the new max() (as you asked). Can only say: dunno. It should not make a difference.

Richard Knutsson

I believe Vignesh had confirmed the patch.....

So now this is going to be crazy we have 3 pairs of min/max to deal with

min()/max()
min_t()/max_t()
__min()/__max()

Can the first one be considered *redundant*

I was wondering that from the very first. calling MACRO(MACRO()) somehow is dirty....Any suggestions !!

Quoted 9 lines · Mehul Jani

I believe Vignesh had confirmed the patch.....

So now this is going to be crazy we have 3 pairs of min/max to deal with

min()/max()
min_t()/max_t()
__min()/__max()

Can the first one be considered *redundant*

min()/max()? That is our crown-jewel. I think the order above is then one how to implement min/max. Use ex min() when possible. If min() does not work, use min_t(). If min_t() does not work, use __min() (if it will be accepted, that is). But you have to be more careful when using __min() since it can have side-effects.

Quoted 2 lines

I was wondering that from the very first. calling MACRO(MACRO()) somehow is dirty....Any suggestions !!

Why? Calling function1(function2()) is ok (right?) and also when dealing with pci-ids you use macros in macros. And the way __min() is in min() and min_t() is also just common practice to reduce multiple declarations.

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Quoted 5 lines

Quoted 5 lines

I believe Vignesh had confirmed the patch.....

So now this is going to be crazy we have 3 pairs of min/max to deal

with

Quoted 11 lines

Quoted 6 lines

min()/max()
min_t()/max_t()
__min()/__max()

Can the first one be considered *redundant*

min()/max()? That is our crown-jewel. I think the order above is then one how to implement min/max. Use ex min() when possible. If min() does not work, use min_t(). If min_t() does not work, use __min() (if it will be accepted, that is). But you have to be more careful when using __min() since it can have side-effects.

Quoted 5 lines

Quoted 2 lines

I was wondering that from the very first. calling MACRO(MACRO()) somehow is dirty....Any suggestions !!

Why? Calling function1(function2()) is ok (right?) and also when dealing with pci-ids you use macros in macros. And the way __min() is in min() and min_t() is also just common practice to reduce multiple

declarations.

Quoted 1 line

Mehul I have tested the patch-compiles cleanly.

Great !!!

Will you patch the complete patching the complete tree ?