Date: Sat, 19 Jul 2014 21:29:23 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r268867 - head/lib/libc/net Message-ID: <20140719210009.O1782@besplex.bde.org> In-Reply-To: <A66005FE-1A28-417A-8268-9690BC68EFD7@freebsd.org> References: <201407190153.s6J1rqBn027367@svn.freebsd.org> <20140719171552.W874@besplex.bde.org> <A66005FE-1A28-417A-8268-9690BC68EFD7@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 19 Jul 2014, Pedro Giffuni wrote:
> Il giorno 19/lug/2014, alle ore 02:36, Bruce Evans <brde@optusnet.com.au>=
ha scritto:
>
>> On Sat, 19 Jul 2014, Pedro F. Giffuni wrote:
>>
>>> Log:
>>> Use unsigned optlen in getsourcefilter()
>>>
>>> Sizes can not be negative and the functions that use it
>>> expect an unsigned value anyways.
>>>
>>> Obtained from:=09Apple (Libc-997.90.3)
>>> MFC after:=091 week
>>
>> Most uses of unsigned types are bugs. This one is an exception, but the
>> change is still wrong. The critical use of the type needs it to be
>> socklen_t, not int or unsigned int. socklen_t happens to have type
>> uint32_t (unsigned due to old bugs). It only accidentally has the
>> same size as unsigned int. It has a different type to plain int so
>> compilers should warn about the type mismatch with certain warning
>> flags.
>>
>
> Ah, yes, we have had this discussion before =85 in relation with ext2fs ;=
).
Yes, it caused bugs in practice there.
> The compiler doesn=92t have a way to tell if socklen_t is of a certain ty=
pe "by accident=94.
Yes, this is a fundamental problem with C typedefs -- they are too similar
to their underlying type.
Any size error would be detected more reliably than the sign error in
furture when int is expanded by socklen_t stays 32 bits. Except the
assumptions that int is 32 bits is even more established now than it
was on vaxes, so no one would expand int.
> I will use socklen_t in this case instead of unsigned int, but I will not=
MFC the changes
> as the original change has no functional (end-user) effect.
>
>
>>> Modified:
>>> head/lib/libc/net/sourcefilter.c
>>>
>>> Modified: head/lib/libc/net/sourcefilter.c
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>> --- head/lib/libc/net/sourcefilter.c=09Sat Jul 19 01:15:01 2014=09(r268=
866)
>>> +++ head/lib/libc/net/sourcefilter.c=09Sat Jul 19 01:53:52 2014=09(r268=
867)
>>> @@ -337,7 +337,8 @@ getsourcefilter(int s, uint32_t interfac
>>> {
>>> =09struct __msfilterreq=09 msfr;
>>> =09sockunion_t=09=09*psu;
>>> -=09int=09=09=09 err, level, nsrcs, optlen, optname;
>>> +=09int=09=09=09 err, level, nsrcs, optname;
>>> +=09unsigned int=09=09 optlen;
>>
>> This has mounds of style bugs. 2 more now (unsorting, and not using
>> u_int, except u_int is not just a style bug)
>
> While aesthetically it may be better to keep the sorting, it doesn=92t se=
em correct to preserve it at the expense of setting the incorrect type. Wou=
ld you really want me to set optname in another line?
You already had to change the sorting to split the declaration. When
changing, it is better to put things in their correct place directly.
u_int is sorted before int because it is a more complex type with a
higher rank. socklen_t is sorted before int because it is a more
complex type with an unknown (opaque) relative rank.
>> % =09err =3D _getsockopt(s, level, optname, &msfr, &optlen);
>>
>> This use needs the correct type since the reference is indirect so the
>> prototype can't adjust the type. The arg had type "int *" in 4.4BSD
>> but has suffered from typedef poisoning so it is now "socklen_t *"
>> 4.4BSD also doesn't have socklen_t. socklen_t is specified by POSIX
>> as being an integer type with width at least 32 bits. It is not
>> required to be unsigned, and there are portability problems from this.
>> POSIX recommends that applications not store values larger than 2**31-1
>> in socklen_t. 2**31 would only work if it is unsigned.
>
> While locally the variable doesn=92t need to be unsigned, conceptually it=
will never be less than zero.
It is a common error to use unsigned types just because their value can
never be less than zero. Most systems of arithmetic (floating point,
bignums(?), ...) don't even have unsigned numbers.
> In this case, for the compiler, setting it to usigned int or to socklen_t=
makes no difference. In practice changing it from signed to unsigned has n=
o effect either as the value will not be big enough to use the extra bit.
>
> It=92s all just a waste of time I guess, so as I said, I won=92t MFC the =
change.
It avoids warnings like:
z.c:7: warning: pointer targets in passing arg 1 of `strlen' differ in sign=
edness
This warning is under -pedantic for gcc, so it is rarely seen. Other
compilers may differ. With this warning and -Werror, you can't even
pass a "u_char *" pointer to strlen(). Strings consist of plain chars
so passing a "u_char *" to strlen() is a type mismatch. IIRC, the C
standard requires a diagnostic for this, but gcc is not a C compiler
even with -pedantic so it doesn't print the diagnostic without
-pedantic. This sign mismatch may be a serious error even for the
char type, but usually isn't.
Bruce
From owner-svn-src-all@FreeBSD.ORG Sat Jul 19 14:06:24 2014
Return-Path: <owner-svn-src-all@FreeBSD.ORG>
Delivered-To: svn-src-all@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115])
(using TLSv1 with cipher ADH-AES256-SHA (256/256 bits))
(No client certificate requested)
by hub.freebsd.org (Postfix) with ESMTPS id 9211A371;
Sat, 19 Jul 2014 14:06:24 +0000 (UTC)
Received: from svn.freebsd.org (svn.freebsd.org
[IPv6:2001:1900:2254:2068::e6a:0])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client did not present a certificate)
by mx1.freebsd.org (Postfix) with ESMTPS id 6631E2EBC;
Sat, 19 Jul 2014 14:06:24 +0000 (UTC)
Received: from svn.freebsd.org ([127.0.1.70])
by svn.freebsd.org (8.14.8/8.14.8) with ESMTP id s6JE6OeZ073928;
Sat, 19 Jul 2014 14:06:24 GMT (envelope-from jilles@svn.freebsd.org)
Received: (from jilles@localhost)
by svn.freebsd.org (8.14.8/8.14.8/Submit) id s6JE6OmP073927;
Sat, 19 Jul 2014 14:06:24 GMT (envelope-from jilles@svn.freebsd.org)
Message-Id: <201407191406.s6JE6OmP073927@svn.freebsd.org>
From: Jilles Tjoelker <jilles@FreeBSD.org>
Date: Sat, 19 Jul 2014 14:06:24 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-head@freebsd.org
Subject: svn commit: r268873 - head/bin/sh
X-SVN-Group: head
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for "
user" and " projects" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>;
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 19 Jul 2014 14:06:24 -0000
Author: jilles
Date: Sat Jul 19 14:06:23 2014
New Revision: 268873
URL: http://svnweb.freebsd.org/changeset/base/268873
Log:
sh: Deduplicate some code in ulimit builtin.
Modified:
head/bin/sh/miscbltin.c
Modified: head/bin/sh/miscbltin.c
==============================================================================
--- head/bin/sh/miscbltin.c Sat Jul 19 13:22:12 2014 (r268872)
+++ head/bin/sh/miscbltin.c Sat Jul 19 14:06:23 2014 (r268873)
@@ -411,12 +411,32 @@ static const struct limits limits[] = {
{ (char *) 0, (char *)0, 0, 0, '\0' }
};
+enum limithow { SOFT = 0x1, HARD = 0x2 };
+
+static void
+printlimit(enum limithow how, const struct rlimit *limit,
+ const struct limits *l)
+{
+ rlim_t val = 0;
+
+ if (how & SOFT)
+ val = limit->rlim_cur;
+ else if (how & HARD)
+ val = limit->rlim_max;
+ if (val == RLIM_INFINITY)
+ out1str("unlimited\n");
+ else
+ {
+ val /= l->factor;
+ out1fmt("%jd\n", (intmax_t)val);
+ }
+}
+
int
ulimitcmd(int argc __unused, char **argv __unused)
{
rlim_t val = 0;
- enum { SOFT = 0x1, HARD = 0x2 }
- how = SOFT | HARD;
+ enum limithow how = SOFT | HARD;
const struct limits *l;
int set, all = 0;
int optc, what;
@@ -475,10 +495,6 @@ ulimitcmd(int argc __unused, char **argv
char optbuf[40];
if (getrlimit(l->cmd, &limit) < 0)
error("can't get limit: %s", strerror(errno));
- if (how & SOFT)
- val = limit.rlim_cur;
- else if (how & HARD)
- val = limit.rlim_max;
if (l->units)
snprintf(optbuf, sizeof(optbuf),
@@ -487,13 +503,7 @@ ulimitcmd(int argc __unused, char **argv
snprintf(optbuf, sizeof(optbuf),
"(-%c) ", l->option);
out1fmt("%-18s %18s ", l->name, optbuf);
- if (val == RLIM_INFINITY)
- out1str("unlimited\n");
- else
- {
- val /= l->factor;
- out1fmt("%jd\n", (intmax_t)val);
- }
+ printlimit(how, &limit, l);
}
return 0;
}
@@ -507,19 +517,7 @@ ulimitcmd(int argc __unused, char **argv
limit.rlim_max = val;
if (setrlimit(l->cmd, &limit) < 0)
error("bad limit: %s", strerror(errno));
- } else {
- if (how & SOFT)
- val = limit.rlim_cur;
- else if (how & HARD)
- val = limit.rlim_max;
-
- if (val == RLIM_INFINITY)
- out1str("unlimited\n");
- else
- {
- val /= l->factor;
- out1fmt("%jd\n", (intmax_t)val);
- }
- }
+ } else
+ printlimit(how, &limit, l);
return 0;
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140719210009.O1782>