[KJ] ok to replace with memcmp?

[KJ] ok to replace with memcmp?

[Daniel Marjamäki]

Hello friends!

I believe it should be possible to replace the code below with memcmp.
According to the comments, memcmp is not good in this case but I
believe it is. The code compares the data in 'key1' and 'key2', and
that is what memcmp is for. Do you have any comments?

I haven't actually tested my change.

Original code (net/core/flow.c):

/* I hear what you're saying, use memcmp.  But memcmp cannot make
 * important assumptions that we can here, such as alignment and
 * constant size.
 */
static int flow_key_compare(struct flowi *key1, struct flowi *key2)
{
	flow_compare_t *k1, *k1_lim, *k2;
	const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);

	if (sizeof(struct flowi) % sizeof(flow_compare_t))
		flowi_is_missized();

	k1 = (flow_compare_t *) key1;
	k1_lim = k1 + n_elem;

	k2 = (flow_compare_t *) key2;

	do {
		if (*k1++ != *k2++)
			return 1;
	} while (k1 < k1_lim);

	return 0;
}


Suggested code:

static int flow_key_compare(struct flowi *key1, struct flowi *key2)
{
	if (sizeof(struct flowi) % sizeof(flow_compare_t))
		flowi_is_missized();

	/* Compare the elements in key1 and key2.
	 * Number of elements to compare:
	 *     n_elem = sizeof(flowi) / sizeof(flow_compare_t)
	 * Number of bytes to compare:
	 *     sz = n_elem * sizeof(flow_compare_t)
	 */
	return memcmp(key1, key2, (sizeof(struct flowi) /
sizeof(flow_compare_t)) * sizeof(flow_compare_t));
}

Best regards,
Daniel
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] ok to replace with memcmp?

[Robert P. J. Day]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 920 bytes --]

On Sat, 30 Dec 2006, Daniel Marjamäki wrote:

> Hello friends!
>
> I believe it should be possible to replace the code below with memcmp.
> According to the comments, memcmp is not good in this case but I
> believe it is. The code compares the data in 'key1' and 'key2', and
> that is what memcmp is for. Do you have any comments?
>
> I haven't actually tested my change.
>
> Original code (net/core/flow.c):
>
> /* I hear what you're saying, use memcmp.  But memcmp cannot make
>  * important assumptions that we can here, such as alignment and
>  * constant size.
>  */
> static int flow_key_compare(struct flowi *key1, struct flowi *key2)
> {
> 	flow_compare_t *k1, *k1_lim, *k2;
> 	const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);

don't use this sizeof/sizeof stuff to calculate the # of elements in
an array.  kernel.h already defines an ARRAY_SIZE macro to do that.

rday

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] ok to replace with memcmp?

[Daniel Marjamäki]

Hello!

I didn't know about the ARRAY_SIZE. But it doesn't seem to be usable
in this situation, does it?

flowi is a struct (defined in include/net/flow.h), not an array.

Best regards,
Daniel


2006/12/30, Robert P. J. Day <rpjday@mindspring.com>:
> On Sat, 30 Dec 2006, Daniel Marjamäki wrote:
>
> > Hello friends!
> >
> > I believe it should be possible to replace the code below with memcmp.
> > According to the comments, memcmp is not good in this case but I
> > believe it is. The code compares the data in 'key1' and 'key2', and
> > that is what memcmp is for. Do you have any comments?
> >
> > I haven't actually tested my change.
> >
> > Original code (net/core/flow.c):
> >
> > /* I hear what you're saying, use memcmp.  But memcmp cannot make
> >  * important assumptions that we can here, such as alignment and
> >  * constant size.
> >  */
> > static int flow_key_compare(struct flowi *key1, struct flowi *key2)
> > {
> >       flow_compare_t *k1, *k1_lim, *k2;
> >       const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
>
> don't use this sizeof/sizeof stuff to calculate the # of elements in
> an array.  kernel.h already defines an ARRAY_SIZE macro to do that.
>
> rday
>

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] ok to replace with memcmp?

[Robert P. J. Day]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 486 bytes --]

On Sun, 31 Dec 2006, Daniel Marjamäki wrote:

> Hello!
>
> I didn't know about the ARRAY_SIZE. But it doesn't seem to be usable
> in this situation, does it?
>
> flowi is a struct (defined in include/net/flow.h), not an array.
>
> Best regards,
> Daniel

whoops, sorry, i didn't realize that.  normally, whenever i see
someone doing a sizeof/sizeof calculation to determine a "number of
elements," i automatically assume they're trying to calculate an
array size.

rday

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] ok to replace with memcmp?

[Tobias Klauser]

On 2006-12-30 at 22:59:35 +0100, Robert P. J. Day <rpjday@mindspring.com> wrote:
> On Sat, 30 Dec 2006, Daniel Marjamäki wrote:
> > I believe it should be possible to replace the code below with memcmp.
> > According to the comments, memcmp is not good in this case but I
> > believe it is. The code compares the data in 'key1' and 'key2', and
> > that is what memcmp is for. Do you have any comments?
> >
> > I haven't actually tested my change.
> >
> > Original code (net/core/flow.c):
> >
> > /* I hear what you're saying, use memcmp.  But memcmp cannot make
> >  * important assumptions that we can here, such as alignment and
> >  * constant size.
> >  */
> > static int flow_key_compare(struct flowi *key1, struct flowi *key2)
> > {
> > 	flow_compare_t *k1, *k1_lim, *k2;
> > 	const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
> 
> don't use this sizeof/sizeof stuff to calculate the # of elements in
> an array.  kernel.h already defines an ARRAY_SIZE macro to do that.

ARRAY_SIZE won't work here as we would be using it against a struct, not
an array.

Cheers, Tobias

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors