[CFR] printf format changes
Sean Bruno
sean_bruno at yahoo.com
Mon Nov 11 21:00:43 UTC 2013
On Sat, 2013-11-09 at 19:36 +1100, Bruce Evans wrote:
> 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
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
===================================================================
--- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (revision 257998)
+++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c (working copy)
@@ -2134,7 +2134,8 @@
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, "%llu",
+ (u_longlong_t)val);
}
break;
@@ -2648,7 +2649,8 @@
return (err);
if (literal) {
- (void) snprintf(propbuf, proplen, "%llu", propvalue);
+ (void) snprintf(propbuf, proplen, "%llu",
+ (u_longlong_t)propvalue);
} else if (propvalue == 0 &&
(type == ZFS_PROP_USERQUOTA || type == ZFS_PROP_GROUPQUOTA)) {
(void) strlcpy(propbuf, "none", proplen);
@@ -2705,7 +2707,8 @@
return (err);
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
===================================================================
--- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_diff.c (revision 257998)
+++ 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 = errno;
break;
}
Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c
===================================================================
--- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (revision 257998)
+++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c (working copy)
@@ -261,7 +261,8 @@
case ZPOOL_PROP_GUID:
intval = zpool_get_prop_int(zhp, prop, &src);
- (void) snprintf(buf, len, "%llu", intval);
+ (void) snprintf(buf, len, "%llu",
+ (u_longlong_t)intval);
break;
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;
@@ -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 != 0 && edata != UINT64_MAX) {
if (edata == 1) {
@@ -2524,7 +2529,7 @@
libzfs_handle_t *hdl = zhp->zpool_hdl;
(void) snprintf(msg, sizeof (msg),
- dgettext(TEXT_DOMAIN, "cannot fault %llu"), guid);
+ dgettext(TEXT_DOMAIN, "cannot fault %llu"), (u_longlong_t)guid);
(void) strlcpy(zc.zc_name, zhp->zpool_name, sizeof (zc.zc_name));
zc.zc_guid = guid;
@@ -2559,7 +2564,7 @@
libzfs_handle_t *hdl = zhp->zpool_hdl;
(void) snprintf(msg, sizeof (msg),
- dgettext(TEXT_DOMAIN, "cannot degrade %llu"), guid);
+ dgettext(TEXT_DOMAIN, "cannot degrade %llu"), (u_longlong_t)guid);
(void) strlcpy(zc.zc_name, zhp->zpool_name, sizeof (zc.zc_name));
zc.zc_guid = guid;
@@ -3228,7 +3233,7 @@
(void) snprintf(msg, sizeof (msg),
dgettext(TEXT_DOMAIN, "cannot clear errors for %llx"),
- guid);
+ (longlong_t)guid);
(void) strlcpy(zc.zc_name, zhp->zpool_name, sizeof (zc.zc_name));
zc.zc_guid = guid;
@@ -3793,7 +3798,8 @@
if (dsobj == 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;
}
@@ -3804,7 +3810,7 @@
ZFS_IOC_DSOBJ_TO_DSNAME, &zc) != 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
===================================================================
--- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (revision 257998)
+++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c (working copy)
@@ -2083,7 +2083,8 @@
needagain = B_TRUE;
else
progress = B_TRUE;
- sprintf(guidname, "%lu", thisguid);
+ sprintf(guidname, "%llu",
+ (u_longlong_t)thisguid);
nvlist_add_boolean(deleted, guidname);
continue;
}
@@ -2140,7 +2141,7 @@
needagain = B_TRUE;
else
progress = 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 != 0 &&
parent_fromsnap_guid != 0 &&
stream_parent_fromsnap_guid != parent_fromsnap_guid) {
- sprintf(guidname, "%lu", parent_fromsnap_guid);
+ sprintf(guidname, "%ju", (uintmax_t)parent_fromsnap_guid);
if (nvlist_exists(deleted, guidname)) {
progress = B_TRUE;
needagain = B_TRUE;
Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.c
===================================================================
--- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.c (revision 257998)
+++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.c (working copy)
@@ -586,13 +586,13 @@
u = " KMGTPE"[index];
if (index == 0) {
- (void) snprintf(buf, buflen, "%llu", n);
+ (void) snprintf(buf, buflen, "%llu", (u_longlong_t)n);
} else if ((num & ((1ULL << 10 * index) - 1)) == 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20131111/03fe67ed/attachment.sig>
More information about the freebsd-fs
mailing list