From owner-freebsd-fs@FreeBSD.ORG Sat Nov 9 08:37:01 2013 Return-Path: Delivered-To: freebsd-fs@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 ESMTP id 175EB54C; Sat, 9 Nov 2013 08:37:01 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id B296B21D3; Sat, 9 Nov 2013 08:36:59 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id E75D23C17D3; Sat, 9 Nov 2013 19:36:51 +1100 (EST) Date: Sat, 9 Nov 2013 19:36:49 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: sbruno@freebsd.org Subject: Re: [CFR] printf format changes In-Reply-To: <1383961188.1526.10.camel@powernoodle.corp.yahoo.com> Message-ID: <20131109183432.R907@besplex.bde.org> References: <1383961188.1526.10.camel@powernoodle.corp.yahoo.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=DstvpgP+ c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=rXxADq7T2ngA:10 a=6I5d2MoRAAAA:8 a=3oGK8yAXRtgIlRceRwsA:9 a=CjuIK1q_8ugA:10 Cc: freebsd-fs X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 09 Nov 2013 08:37:01 -0000 On Fri, 8 Nov 2013, Sean Bruno wrote: > http://people.freebsd.org/~sbruno/freebsd_libzfs_printf_changes.txt > > Some minor nits from compiles. Tested on i386/amd64 with clang. > > Not sure how to test with gcc though. Every part of the patch that I can see in the mail (that is, none) is correct. svn mail sends much larger diffs than this. After fetching the patch: % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c % =================================================================== % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (revision 257859) % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (working copy) % @@ -2134,7 +2134,7 @@ % localtime_r(&time, &t) == NULL || % strftime(propbuf, proplen, "%a %b %e %k:%M %Y", % &t) == 0) % - (void) snprintf(propbuf, proplen, "%llu", val); % + (void) snprintf(propbuf, proplen, "%ju", (intmax_t)val); % } % break; % Style bug: line expanded beyond 80 columns. The vendor code is fairly careful about this, but has plenty of other things that I don't like, starting with obfuscating format strings to avoid long lines. Style bug: non-use of the long long abomination. The vendor won't like your fix. They prefer to use the abomination. They keep using the abomination in the format (%llu) and spell the cast '(ulonglong_t)' in many places in this file, but this patch shows that they missed many. Type mismatches: `val' starts as an unsigned type (uint64_t) but is cast to a signed type (intmax_t). This gives another type mismatch with the the format, which requires a signed type. You may be able to prove that the behaviour is defined and correct on all arches after studying the C standard for less than a week. It is much easier to see that it is correct, and possibly even defined, in the usual case where intmax_t and uintmax_t are 64 bits. The vendor code has these type mismatches in 2 places (Sep 10 version). The vendor casts to longlong_t where they should cast to ulonglong_t. They consistently use the unsigned format (%llu; never %lld). % @@ -2648,7 +2648,7 @@ % return (err); % % if (literal) { % - (void) snprintf(propbuf, proplen, "%llu", propvalue); % + (void) snprintf(propbuf, proplen, "%ju", (intmax_t)propvalue); % } else if (propvalue == 0 && % (type == ZFS_PROP_USERQUOTA || type == ZFS_PROP_GROUPQUOTA)) { % (void) strlcpy(propbuf, "none", proplen); As above, except for the long line. Unfortunately, using the vendor spelling of the cast will make the line too long. All the changes have much the same bugs. No further comment on ones that have a subset of the above. % ... % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c % =================================================================== % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c (revision 257859) % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c (working copy) % @@ -116,7 +116,7 @@ % (void) snprintf(di->errbuf, sizeof (di->errbuf), % dgettext(TEXT_DOMAIN, % "Unable to determine path or stats for " % - "object %lld in %s"), obj, dsname); % + "object %jd in %s"), (intmax_t)obj, dsname); % return (-1); % } % } 'obj' starts as an unsigned type (uint64_t), but is converted to a signed type for printing in a signed format. Perhaps that is intended. No further comment on this. % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c % =================================================================== % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (revision 257859) % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (working copy) % @@ -1472,15 +1472,15 @@ % } % if (loss > 120) { % (void) printf(dgettext(TEXT_DOMAIN, % - "%s approximately %lld "), % + "%s approximately %jd "), % dryrun ? "Would discard" : "Discarded", % - (loss + 30) / 60); % + (intmax_t)(loss + 30) / 60); % (void) printf(dgettext(TEXT_DOMAIN, % "minutes of transactions.\n")); 'loss' is signed (int64_t), so the signed format is clearly correct. % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c % =================================================================== % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (revision 257859) % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (working copy) % @@ -2083,7 +2083,7 @@ % needagain = B_TRUE; % else % progress = B_TRUE; % - sprintf(guidname, "%lu", thisguid); % + sprintf(guidname, "%ju", (uintmax_t)thisguid); % nvlist_add_boolean(deleted, guidname); % continue; % } The vendor used a very wrong format here. No furthe comment on this. % @@ -3081,8 +3081,8 @@ % zfs_nicenum(bytes, buf1, sizeof (buf1)); % zfs_nicenum(bytes/delta, buf2, sizeof (buf1)); % % - (void) printf("received %sB stream in %lu seconds (%sB/sec)\n", % - buf1, delta, buf2); % + (void) printf("received %sB stream in %ld seconds (%sB/sec)\n", % + buf1, (unsigned long)delta, buf2); % } % % return (0); Various type mismatches: delta is time_t which was supposed to be fully opaque (C) but is an integer type with a specified representation of times (POSIX). It can be signed or unsigned, but it is usually signed. It only needs to be signed to represent times before the Epoch and negative time differences. Printing it using a signed format is the easiest way to allow for it having negative differences without having to decide if they can happen, although this is not strictly correct. Here you change the format to signed for no apparent reason and then get a type mismatch by casting the arg to unsigned. Both should be to signed. But let the vendor decide this and keep the unsigned format. Overflow errors: some systems have 64-bit time_t to avoid overflow errors. Casting to long or unsigned long breaks this on 32-bit systems for times later than 2038. Times later than 2038 are probably not needed here, so let the vendor decide this too, by keeping the %lu format and casting the arg to match it. Bruce