Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 19 Mar 2008 01:20:37 +0600
From:      Vadim Goncharov <vadim_nuclight@mail.ru>
To:        Paolo Pisati <piso@FreeBSD.org>
Cc:        cvs-src@FreeBSD.org, des@FreeBSD.org
Subject:   Re: cvs commit: src/sys/netinet/libalias alias_irc.c
Message-ID:  <200803181920.m2IJKcoD038230@hostel.avtf.net>
In-Reply-To: <200803172216.m2HMGJKj011742@repoman.freebsd.org>
References:  <200803172216.m2HMGJKj011742@repoman.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Hi Paolo Pisati! 

On Mon, 17 Mar 2008 22:16:19 +0000 (UTC); Paolo Pisati <piso@FreeBSD.org> wrote:

> piso        2008-03-17 22:16:19 UTC

>   FreeBSD src repository

>   Modified files:        (Branch: RELENG_7)
>     sys/netinet/libalias alias_irc.c 
>   Log:
>   MFC revision 1.25:
>   Don't abuse stack space while in kernel land, use heap instead.
>   
>   PR:     kern/121693
>   
>   Revision  Changes    Path
>   1.23.2.1  +10 -5     src/sys/netinet/libalias/alias_irc.c

Paolo,

this violates rules 6 and 10 from committers-guide.

The original commit log in HEAD did not contained "MFC After:", so the code
was not supposed to be MFCed to STABLE. Moreover, minimum MFC period is
3 days, and you've done MFCs to both 6-STABLE and 7-STABLE in 8 minutes
after commit to HEAD. Then, PR must not be closed so fast, as originator
has not yet confirmed that patch has fixed his problem.

This could be non-critical, but that rules exists to guard against bugs in the
code, so - let's look at the code. You are changing "char newpacket[65536]" to
"char *newpacket", and there are places there like:

                 if (iCopy + 4 > sizeof(newpacket))
                         goto lPACKET_DONE;

which you hasn't changed, while "iCopy" is "unsigned int". This will break
behaviour, of course, in 7-STABLE - in 6-STABLE it is still an array.

It is questionable whether silent change in size from 65536 to IP_MAXPACKET
(which is one less) is acceptable, but there is also another clear point:
this will be allocated with M_NOWAIT on every packet. It is better to prealloc
that memory on module load, the way you've done in 6-STABLE's MFC variant.

-- 
WBR, Vadim Goncharov. ICQ#166852181       mailto:vadim_nuclight@mail.ru
[Moderator of RU.ANTI-ECOLOGY][FreeBSD][http://antigreen.org][LJ:/nuclight]



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200803181920.m2IJKcoD038230>