[CFR] printf format changes

[Bruce Evans]

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