Date: Mon, 11 Nov 2013 13:00:35 -0800 From: Sean Bruno <sean_bruno@yahoo.com> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs <freebsd-fs@freebsd.org> Subject: Re: [CFR] printf format changes Message-ID: <1384203635.1748.49.camel@powernoodle.corp.yahoo.com> In-Reply-To: <20131109183432.R907@besplex.bde.org> References: <1383961188.1526.10.camel@powernoodle.corp.yahoo.com> <20131109183432.R907@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-c6B1ZI+Ea46ONCCQllKK Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable On Sat, 2013-11-09 at 19:36 +1100, Bruce Evans wrote: > On Fri, 8 Nov 2013, Sean Bruno wrote: >=20 > > 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. >=20 > 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. >=20 > After fetching the patch: >=20 > % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.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 > % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (revisi= on 257859) > % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (workin= g copy) > % @@ -2134,7 +2134,7 @@ > % localtime_r(&time, &t) =3D=3D NULL || > % strftime(propbuf, proplen, "%a %b %e %k:%M %Y", > % &t) =3D=3D 0) > % - (void) snprintf(propbuf, proplen, "%llu", val); > % + (void) snprintf(propbuf, proplen, "%ju", (intmax_t)val); > % } > % break; > % >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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). >=20 > % @@ -2648,7 +2648,7 @@ > % return (err); > %=20 > % if (literal) { > % - (void) snprintf(propbuf, proplen, "%llu", propvalue); > % + (void) snprintf(propbuf, proplen, "%ju", (intmax_t)propvalue); > % } else if (propvalue =3D=3D 0 && > % (type =3D=3D ZFS_PROP_USERQUOTA || type =3D=3D ZFS_PROP_GROUPQUOT= A)) { > % (void) strlcpy(propbuf, "none", proplen); >=20 > As above, except for the long line. >=20 > Unfortunately, using the vendor spelling of the cast will make the line t= oo > long. >=20 > All the changes have much the same bugs. No further comment on ones that > have a subset of the above. >=20 > % ... > % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.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 > % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c (revision = 257859) > % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c (working c= opy) > % @@ -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); > % } > % } >=20 > '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. >=20 > % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.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 > % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (revision = 257859) > % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (working c= opy) > % @@ -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")); >=20 > 'loss' is signed (int64_t), so the signed format is clearly correct. >=20 > % Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.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 > % --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (revis= ion 257859) > % +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (worki= ng copy) > % @@ -2083,7 +2083,7 @@ > % needagain =3D B_TRUE; > % else > % progress =3D B_TRUE; > % - sprintf(guidname, "%lu", thisguid); > % + sprintf(guidname, "%ju", (uintmax_t)thisguid); > % nvlist_add_boolean(deleted, guidname); > % continue; > % } >=20 > The vendor used a very wrong format here. No furthe comment on this. >=20 > % @@ -3081,8 +3081,8 @@ > % zfs_nicenum(bytes, buf1, sizeof (buf1)); > % zfs_nicenum(bytes/delta, buf2, sizeof (buf1)); > %=20 > % - (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); > % } > %=20 > % return (0); >=20 > 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 time= s > (POSIX). It can be signed or unsigned, but it is usually signed. It onl= y > 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. >=20 > 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. >=20 > Bruce Updated patch for libzfs. I've continued the (longlong_t) nonsense as it matches the vendor code, wrapped any lines that go past 80 and reset the cast and printf() for the time_t argument. This patch is being sent to illumos for review as issue 4309. http://people.freebsd.org/~sbruno/libzfs_update_casting.txt Patch txt as follows: Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.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 --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (revision 2= 57998) +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (working co= py) @@ -2134,7 +2134,8 @@ localtime_r(&time, &t) =3D=3D NULL || strftime(propbuf, proplen, "%a %b %e %k:%M %Y", &t) =3D=3D 0) - (void) snprintf(propbuf, proplen, "%llu", val); + (void) snprintf(propbuf, proplen, "%llu", + (u_longlong_t)val); } break; =20 @@ -2648,7 +2649,8 @@ return (err); =20 if (literal) { - (void) snprintf(propbuf, proplen, "%llu", propvalue); + (void) snprintf(propbuf, proplen, "%llu", + (u_longlong_t)propvalue); } else if (propvalue =3D=3D 0 && (type =3D=3D ZFS_PROP_USERQUOTA || type =3D=3D ZFS_PROP_GROUPQUOTA)) = { (void) strlcpy(propbuf, "none", proplen); @@ -2705,7 +2707,8 @@ return (err); =20 if (literal) { - (void) snprintf(propbuf, proplen, "%llu", propvalue); + (void) snprintf(propbuf, proplen, "%llu", + (u_longlong_t)propvalue); } else { zfs_nicenum(propvalue, propbuf, proplen); } Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.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 --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c (revision 2579= 98) +++ 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 %lld in %s"), (longlong_t)obj, dsname); return (-1); } } @@ -410,7 +410,7 @@ (void) snprintf(di->errbuf, sizeof (di->errbuf), dgettext(TEXT_DOMAIN, "next allocated object (> %lld) find failure"), - zc.zc_obj); + (longlong_t)(zc.zc_obj)); di->zerr =3D errno; break; } Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.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 --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (revision 2579= 98) +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (working copy) @@ -261,7 +261,8 @@ =20 case ZPOOL_PROP_GUID: intval =3D zpool_get_prop_int(zhp, prop, &src); - (void) snprintf(buf, len, "%llu", intval); + (void) snprintf(buf, len, "%llu", + (u_longlong_t)intval); break; =20 case ZPOOL_PROP_ALTROOT: @@ -337,7 +338,8 @@ } /* FALLTHROUGH */ default: - (void) snprintf(buf, len, "%llu", intval); + (void) snprintf(buf, len, "%llu", + (u_longlong_t)intval); } break; =20 @@ -1474,13 +1476,14 @@ (void) printf(dgettext(TEXT_DOMAIN, "%s approximately %lld "), dryrun ? "Would discard" : "Discarded", - (loss + 30) / 60); + (longlong_t)(loss + 30) / 60); (void) printf(dgettext(TEXT_DOMAIN, "minutes of transactions.\n")); } else if (loss > 0) { (void) printf(dgettext(TEXT_DOMAIN, "%s approximately %lld "), - dryrun ? "Would discard" : "Discarded", loss); + dryrun ? "Would discard" : "Discarded", + (longlong_t)loss); (void) printf(dgettext(TEXT_DOMAIN, "seconds of transactions.\n")); } @@ -1534,11 +1537,13 @@ if (loss > 120) { (void) printf(dgettext(TEXT_DOMAIN, "Approximately %lld minutes of data\n" - "\tmust be discarded, irreversibly. "), (loss + 30) / 60); + "\tmust be discarded, irreversibly. "), + (longlong_t)(loss + 30) / 60); } else if (loss > 0) { (void) printf(dgettext(TEXT_DOMAIN, "Approximately %lld seconds of data\n" - "\tmust be discarded, irreversibly. "), loss); + "\tmust be discarded, irreversibly. "), + (longlong_t)loss); } if (edata !=3D 0 && edata !=3D UINT64_MAX) { if (edata =3D=3D 1) { @@ -2524,7 +2529,7 @@ libzfs_handle_t *hdl =3D zhp->zpool_hdl; =20 (void) snprintf(msg, sizeof (msg), - dgettext(TEXT_DOMAIN, "cannot fault %llu"), guid); + dgettext(TEXT_DOMAIN, "cannot fault %llu"), (u_longlong_t)guid); =20 (void) strlcpy(zc.zc_name, zhp->zpool_name, sizeof (zc.zc_name)); zc.zc_guid =3D guid; @@ -2559,7 +2564,7 @@ libzfs_handle_t *hdl =3D zhp->zpool_hdl; =20 (void) snprintf(msg, sizeof (msg), - dgettext(TEXT_DOMAIN, "cannot degrade %llu"), guid); + dgettext(TEXT_DOMAIN, "cannot degrade %llu"), (u_longlong_t)guid); =20 (void) strlcpy(zc.zc_name, zhp->zpool_name, sizeof (zc.zc_name)); zc.zc_guid =3D guid; @@ -3228,7 +3233,7 @@ =20 (void) snprintf(msg, sizeof (msg), dgettext(TEXT_DOMAIN, "cannot clear errors for %llx"), - guid); + (longlong_t)guid); =20 (void) strlcpy(zc.zc_name, zhp->zpool_name, sizeof (zc.zc_name)); zc.zc_guid =3D guid; @@ -3793,7 +3798,8 @@ =20 if (dsobj =3D=3D 0) { /* special case for the MOS */ - (void) snprintf(pathname, len, "<metadata>:<0x%llx>", obj); + (void) snprintf(pathname, len, "<metadata>:<0x%llx>", + (longlong_t)obj); return; } =20 @@ -3804,7 +3810,7 @@ ZFS_IOC_DSOBJ_TO_DSNAME, &zc) !=3D 0) { /* just write out a path of two object numbers */ (void) snprintf(pathname, len, "<0x%llx>:<0x%llx>", - dsobj, obj); + (longlong_t)dsobj, (longlong_t)obj); return; } (void) strlcpy(dsname, zc.zc_value, sizeof (dsname)); @@ -3825,7 +3831,8 @@ dsname, zc.zc_value); } } else { - (void) snprintf(pathname, len, "%s:<0x%llx>", dsname, obj); + (void) snprintf(pathname, len, "%s:<0x%llx>", dsname, + (longlong_t)obj); } free(mntpnt); } Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.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 --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (revision = 257998) +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (working c= opy) @@ -2083,7 +2083,8 @@ needagain =3D B_TRUE; else progress =3D B_TRUE; - sprintf(guidname, "%lu", thisguid); + sprintf(guidname, "%llu", + (u_longlong_t)thisguid); nvlist_add_boolean(deleted, guidname); continue; } @@ -2140,7 +2141,7 @@ needagain =3D B_TRUE; else progress =3D B_TRUE; - sprintf(guidname, "%lu", parent_fromsnap_guid); + sprintf(guidname, "%llu", (u_longlong_t)parent_fromsnap_guid); nvlist_add_boolean(deleted, guidname); continue; } @@ -2173,7 +2174,7 @@ if (stream_parent_fromsnap_guid !=3D 0 && parent_fromsnap_guid !=3D 0 && stream_parent_fromsnap_guid !=3D parent_fromsnap_guid)= { - sprintf(guidname, "%lu", parent_fromsnap_guid); + sprintf(guidname, "%ju", (uintmax_t)parent_fromsnap_guid); if (nvlist_exists(deleted, guidname)) { progress =3D B_TRUE; needagain =3D B_TRUE; Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.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 --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.c (revision 2579= 98) +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.c (working copy) @@ -586,13 +586,13 @@ u =3D " KMGTPE"[index]; =20 if (index =3D=3D 0) { - (void) snprintf(buf, buflen, "%llu", n); + (void) snprintf(buf, buflen, "%llu", (u_longlong_t)n); } else if ((num & ((1ULL << 10 * index) - 1)) =3D=3D 0) { /* * If this is an even multiple of the base, always display * without any decimal precision. */ - (void) snprintf(buf, buflen, "%llu%c", n, u); + (void) snprintf(buf, buflen, "%llu%c", (u_longlong_t)n, u); } else { /* * We want to choose a precision that reflects the best choice --=-c6B1ZI+Ea46ONCCQllKK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (FreeBSD) iQEcBAABAgAGBQJSgUVyAAoJEBkJRdwI6BaHBagH/iQq25mba7DjXpnyQrizweak zC0ZNjXO1FXFLsgDqj01tFP9GA3Zbja6a8v1AnLMgqSX+Wqdxdbx9GpSX8kKZp05 tfctN773aYc1rZnOXWyBgZhyDGvv6vbeqsrZIy7ZHOw+62CoaU4ztWPHKgPXVgMk 9kq5kte+R/7NuQRheOVL7eOp3btm0uHreVXTi46DblaiH/JKrEQmTTDicptBkpHC CRRSqZnfrk78O+kb2/sk/fwouE1fVUSmOY9/L+N4qb1CNk52FRrNgLFZhgFLU4tY eQdVJY1UY+3XRkP7liz9V0zpjH9G8OZuiLzI0kMMa8WvHKZxWZHCVH6aQ0v+O3k= =rBtX -----END PGP SIGNATURE----- --=-c6B1ZI+Ea46ONCCQllKK--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1384203635.1748.49.camel>