* [RFC PATCH 2/2] mkfs: remove long long type casts
2017-04-06 14:41 [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
@ 2017-04-06 14:41 ` Jan Tulak
2017-04-06 15:02 ` [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Jan Tulak @ 2017-04-06 14:41 UTC (permalink / raw)
To: linux-xfs; +Cc: mcgrof, sandeen, Jan Tulak
We have uint64, why cast it to long long for prints? Just print it as
it is.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
mkfs/xfs_mkfs.c | 144 ++++++++++++++++++++++++++++----------------------------
1 file changed, 72 insertions(+), 72 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 71382f70..885b5c0a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -900,9 +900,9 @@ fixup_log_stripe_unit(
}
*logblocks = tmp_logblocks;
} else {
- fprintf(stderr, _("log size %lld is not a multiple "
+ fprintf(stderr, _("log size %lu is not a multiple "
"of the log stripe unit %d\n"),
- (long long) *logblocks, sunit);
+ *logblocks, sunit);
usage();
}
}
@@ -929,7 +929,7 @@ fixup_internal_log_stripe(
if (*logblocks > agsize - XFS_FSB_TO_AGBNO(mp, logstart)) {
fprintf(stderr,
_("Due to stripe alignment, the internal log size "
- "(%lld) is too large.\n"), (long long) *logblocks);
+ "(%lu) is too large.\n"), *logblocks);
fprintf(stderr, _("Must fit within an allocation group.\n"));
usage();
}
@@ -941,20 +941,20 @@ validate_log_size(__uint64_t logblocks, int blocklog, int min_logblocks)
{
if (logblocks < min_logblocks) {
fprintf(stderr,
- _("log size %lld blocks too small, minimum size is %d blocks\n"),
- (long long)logblocks, min_logblocks);
+ _("log size %lu blocks too small, minimum size is %d blocks\n"),
+ logblocks, min_logblocks);
usage();
}
if (logblocks > XFS_MAX_LOG_BLOCKS) {
fprintf(stderr,
- _("log size %lld blocks too large, maximum size is %lld blocks\n"),
- (long long)logblocks, XFS_MAX_LOG_BLOCKS);
+ _("log size %lu blocks too large, maximum size is %lld blocks\n"),
+ logblocks, XFS_MAX_LOG_BLOCKS);
usage();
}
if ((logblocks << blocklog) > XFS_MAX_LOG_BYTES) {
fprintf(stderr,
- _("log size %lld bytes too large, maximum size is %lld bytes\n"),
- (long long)(logblocks << blocklog), XFS_MAX_LOG_BYTES);
+ _("log size %lu bytes too large, maximum size is %lld bytes\n"),
+ (logblocks << blocklog), XFS_MAX_LOG_BYTES);
usage();
}
}
@@ -990,43 +990,43 @@ validate_ag_geometry(
{
if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
fprintf(stderr,
- _("agsize (%lld blocks) too small, need at least %lld blocks\n"),
- (long long)agsize,
- (long long)XFS_AG_MIN_BLOCKS(blocklog));
+ _("agsize (%lu blocks) too small, need at least %lu blocks\n"),
+ agsize,
+ (__uint64_t)XFS_AG_MIN_BLOCKS(blocklog));
usage();
}
if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
fprintf(stderr,
- _("agsize (%lld blocks) too big, maximum is %lld blocks\n"),
- (long long)agsize,
- (long long)XFS_AG_MAX_BLOCKS(blocklog));
+ _("agsize (%lu blocks) too big, maximum is %lu blocks\n"),
+ agsize,
+ (__uint64_t)XFS_AG_MAX_BLOCKS(blocklog));
usage();
}
if (agsize > dblocks) {
fprintf(stderr,
- _("agsize (%lld blocks) too big, data area is %lld blocks\n"),
- (long long)agsize, (long long)dblocks);
+ _("agsize (%lu blocks) too big, data area is %lu blocks\n"),
+ agsize, dblocks);
usage();
}
if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
fprintf(stderr,
- _("too many allocation groups for size = %lld\n"),
- (long long)agsize);
- fprintf(stderr, _("need at most %lld allocation groups\n"),
- (long long)(dblocks / XFS_AG_MIN_BLOCKS(blocklog) +
+ _("too many allocation groups for size = %lu\n"),
+ agsize);
+ fprintf(stderr, _("need at most %lu allocation groups\n"),
+ (__uint64_t) (dblocks / XFS_AG_MIN_BLOCKS(blocklog) +
(dblocks % XFS_AG_MIN_BLOCKS(blocklog) != 0)));
usage();
}
if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
fprintf(stderr,
- _("too few allocation groups for size = %lld\n"), (long long)agsize);
+ _("too few allocation groups for size = %lu\n"), agsize);
fprintf(stderr,
- _("need at least %lld allocation groups\n"),
- (long long)(dblocks / XFS_AG_MAX_BLOCKS(blocklog) +
+ _("need at least %lu allocation groups\n"),
+ (__uint64_t)(dblocks / XFS_AG_MAX_BLOCKS(blocklog) +
(dblocks % XFS_AG_MAX_BLOCKS(blocklog) != 0)));
usage();
}
@@ -1038,9 +1038,9 @@ validate_ag_geometry(
if ( dblocks % agsize != 0 &&
(dblocks % agsize < XFS_AG_MIN_BLOCKS(blocklog))) {
fprintf(stderr,
- _("last AG size %lld blocks too small, minimum size is %lld blocks\n"),
- (long long)(dblocks % agsize),
- (long long)XFS_AG_MIN_BLOCKS(blocklog));
+ _("last AG size %lu blocks too small, minimum size is %lu blocks\n"),
+ (dblocks % agsize),
+ (__uint64_t)XFS_AG_MIN_BLOCKS(blocklog));
usage();
}
@@ -1049,8 +1049,8 @@ validate_ag_geometry(
*/
if (agcount > XFS_MAX_AGNUMBER + 1) {
fprintf(stderr,
- _("%lld allocation groups is too many, maximum is %lld\n"),
- (long long)agcount, (long long)XFS_MAX_AGNUMBER + 1);
+ _("%lu allocation groups is too many, maximum is %lu\n"),
+ agcount, (__uint64_t)XFS_MAX_AGNUMBER + 1);
usage();
}
}
@@ -2170,16 +2170,16 @@ _("rmapbt not supported with realtime devices\n"));
dbytes = getnum(dsize, &dopts, D_SIZE);
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
- _("illegal data length %lld, not a multiple of %d\n"),
- (long long)dbytes, XFS_MIN_BLOCKSIZE);
+ _("illegal data length %lu, not a multiple of %d\n"),
+ dbytes, XFS_MIN_BLOCKSIZE);
usage();
}
dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
if (dbytes % blocksize)
fprintf(stderr, _("warning: "
- "data length %lld not a multiple of %lu, truncated to %lld\n"),
- (long long)dbytes, blocksize,
- (long long)(dblocks << blocklog));
+ "data length %lu not a multiple of %lu, truncated to %lu\n"),
+ dbytes, blocksize,
+ (__uint64_t)(dblocks << blocklog));
}
if (ipflag) {
inodelog = blocklog - libxfs_highbit32(inopblock);
@@ -2202,16 +2202,16 @@ _("rmapbt not supported with realtime devices\n"));
logbytes = getnum(logsize, &lopts, L_SIZE);
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
- _("illegal log length %lld, not a multiple of %d\n"),
- (long long)logbytes, XFS_MIN_BLOCKSIZE);
+ _("illegal log length %lu, not a multiple of %d\n"),
+ logbytes, XFS_MIN_BLOCKSIZE);
usage();
}
logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
if (logbytes % blocksize)
fprintf(stderr,
- _("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
- (long long)logbytes, blocksize,
- (long long)(logblocks << blocklog));
+ _("warning: log length %lu not a multiple of %lu, truncated to %lu\n"),
+ logbytes, blocksize,
+ (__uint64_t)(logblocks << blocklog));
}
if (rtsize) {
__uint64_t rtbytes;
@@ -2219,16 +2219,16 @@ _("rmapbt not supported with realtime devices\n"));
rtbytes = getnum(rtsize, &ropts, R_SIZE);
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
- _("illegal rt length %lld, not a multiple of %d\n"),
- (long long)rtbytes, XFS_MIN_BLOCKSIZE);
+ _("illegal rt length %lu, not a multiple of %d\n"),
+ rtbytes, XFS_MIN_BLOCKSIZE);
usage();
}
rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
if (rtbytes % blocksize)
fprintf(stderr,
- _("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
- (long long)rtbytes, blocksize,
- (long long)(rtblocks << blocklog));
+ _("warning: rt length %lu not a multiple of %lu, truncated to %lu\n"),
+ rtbytes, blocksize,
+ (__uint64_t)(rtblocks << blocklog));
}
/*
* If specified, check rt extent size against its constraints.
@@ -2239,8 +2239,8 @@ _("rmapbt not supported with realtime devices\n"));
rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
if (rtextbytes % blocksize) {
fprintf(stderr,
- _("illegal rt extent size %lld, not a multiple of %lu\n"),
- (long long)rtextbytes, blocksize);
+ _("illegal rt extent size %lu, not a multiple of %lu\n"),
+ rtextbytes, blocksize);
usage();
}
rtextblocks = (xfs_extlen_t)(rtextbytes >> blocklog);
@@ -2367,8 +2367,8 @@ _("rmapbt not supported with realtime devices\n"));
if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
fprintf(stderr,
_("size %s specified for data subvolume is too large, "
- "maximum is %lld blocks\n"),
- dsize, (long long)DTOBT(xi.dsize));
+ "maximum is %lu blocks\n"),
+ dsize, (__uint64_t)DTOBT(xi.dsize));
usage();
} else if (!dsize && xi.dsize > 0)
dblocks = DTOBT(xi.dsize);
@@ -2378,8 +2378,8 @@ _("rmapbt not supported with realtime devices\n"));
}
if (dblocks < XFS_MIN_DATA_BLOCKS) {
fprintf(stderr,
- _("size %lld of data subvolume is too small, minimum %d blocks\n"),
- (long long)dblocks, XFS_MIN_DATA_BLOCKS);
+ _("size %lu of data subvolume is too small, minimum %d blocks\n"),
+ dblocks, XFS_MIN_DATA_BLOCKS);
usage();
}
@@ -2415,8 +2415,8 @@ reported by the device (%u).\n"),
if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
fprintf(stderr,
_("size %s specified for rt subvolume is too large, "
- "maximum is %lld blocks\n"),
- rtsize, (long long)DTOBT(xi.rtsize));
+ "maximum is %lu blocks\n"),
+ rtsize, (__uint64_t)DTOBT(xi.rtsize));
usage();
} else if (!rtsize && xi.rtsize > 0)
rtblocks = DTOBT(xi.rtsize);
@@ -2462,8 +2462,8 @@ reported by the device (%u).\n"),
*/
if (agsize % blocksize) {
fprintf(stderr,
- _("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
- (long long)agsize, blocksize);
+ _("agsize (%lu) not a multiple of fs blk size (%lu)\n"),
+ agsize, blocksize);
usage();
}
agsize /= blocksize;
@@ -2512,8 +2512,8 @@ reported by the device (%u).\n"),
(dblocks % agsize != 0);
if (dasize)
fprintf(stderr,
- _("agsize rounded to %lld, swidth = %lu\n"),
- (long long)agsize, dswidth);
+ _("agsize rounded to %lu, swidth = %lu\n"),
+ agsize, dswidth);
} else {
if (nodsflag) {
dsunit = dswidth = 0;
@@ -2629,8 +2629,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
fprintf(stderr,
-_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
- logsize, (long long)DTOBT(xi.logBBsize));
+_("size %s specified for log subvolume is too large, maximum is %lu blocks\n"),
+ logsize, (__uint64_t)DTOBT(xi.logBBsize));
usage();
} else if (!logsize && xi.logBBsize > 0) {
logblocks = DTOBT(xi.logBBsize);
@@ -2639,8 +2639,8 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
_("size specified for non-existent log subvolume\n"));
usage();
} else if (loginternal && logsize && logblocks >= dblocks) {
- fprintf(stderr, _("size %lld too large for internal log\n"),
- (long long)logblocks);
+ fprintf(stderr, _("size %lu too large for internal log\n"),
+ logblocks);
usage();
} else if (!loginternal && !xi.logdev) {
logblocks = 0;
@@ -2717,16 +2717,16 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
}
if (logblocks > agsize - libxfs_prealloc_blocks(mp)) {
fprintf(stderr,
- _("internal log size %lld too large, must fit in allocation group\n"),
- (long long)logblocks);
+ _("internal log size %lu too large, must fit in allocation group\n"),
+ logblocks);
usage();
}
if (laflag) {
if (logagno >= agcount) {
fprintf(stderr,
- _("log ag number %d too large, must be less than %lld\n"),
- logagno, (long long)agcount);
+ _("log ag number %d too large, must be less than %lu\n"),
+ logagno, agcount);
usage();
}
} else
@@ -2755,29 +2755,29 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
if (!qflag || Nflag) {
printf(_(
- "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
+ "meta-data=%-22s isize=%-6lu agcount=%lu, agsize=%lu blks\n"
" =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
" =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
- "data =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
+ "data =%-22s bsize=%-6lu blocks=%lu, imaxpct=%lu\n"
" =%-22s sunit=%-6lu swidth=%lu blks\n"
"naming =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
- "log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
+ "log =%-22s bsize=%-6d blocks=%lu, version=%d\n"
" =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
- "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
- dfile, isize, (long long)agcount, (long long)agsize,
+ "realtime =%-22s extsz=%-6d blocks=%lu, rtextents=%lu\n"),
+ dfile, isize, agcount, agsize,
"", sectorsize, sb_feat.attr_version,
!sb_feat.projid16bit,
"", sb_feat.crcs_enabled, sb_feat.finobt, sb_feat.spinodes,
sb_feat.rmapbt, sb_feat.reflink,
- "", blocksize, (long long)dblocks, imaxpct,
+ "", blocksize, dblocks, imaxpct,
"", dsunit, dswidth,
sb_feat.dir_version, dirblocksize, sb_feat.nci,
sb_feat.dirftype,
- logfile, 1 << blocklog, (long long)logblocks,
+ logfile, 1 << blocklog, logblocks,
sb_feat.log_version, "", lsectorsize, lsunit,
sb_feat.lazy_sb_counters,
rtfile, rtextblocks << blocklog,
- (long long)rtblocks, (long long)rtextents);
+ rtblocks, rtextents);
if (Nflag)
exit(0);
}
--
2.12.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-06 14:41 [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
2017-04-06 14:41 ` [RFC PATCH 2/2] mkfs: remove long long type casts Jan Tulak
@ 2017-04-06 15:02 ` Jan Tulak
2017-04-06 19:46 ` Darrick J. Wong
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Jan Tulak @ 2017-04-06 15:02 UTC (permalink / raw)
To: linux-xfs; +Cc: Luis R. Rodriguez, Eric Sandeen, Jan Tulak
On Thu, Apr 6, 2017 at 4:41 PM, Jan Tulak <jtulak@redhat.com> wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
>
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
>
> This patch changes variables declared at the beginning of main() + block/sectorsize, making only minimal changes. The following patch cleans some now-unnecessary type casts.
>
> It would be nice to change types in some of the structures too, but
> this might lead to changes outside of mkfs, so I'm skipping them for
> this moment to keep it simple.
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
> 1 file changed, 92 insertions(+), 92 deletions(-)
>
Public git tree for these changes:
https://github.com/jtulak/xfsprogs-dev/tree/github-uint
(It includes the other two small patches I submitted today too).
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-06 14:41 [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
2017-04-06 14:41 ` [RFC PATCH 2/2] mkfs: remove long long type casts Jan Tulak
2017-04-06 15:02 ` [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
@ 2017-04-06 19:46 ` Darrick J. Wong
2017-04-06 20:13 ` Eric Sandeen
2017-04-07 1:50 ` Dave Chinner
4 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2017-04-06 19:46 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs, mcgrof, sandeen
On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
>
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
>
> This patch changes variables declared at the beginning of main() +
> block/sectorsize, making only minimal changes. The following patch cleans
> some now-unnecessary type casts.
>
> It would be nice to change types in some of the structures too, but
> this might lead to changes outside of mkfs, so I'm skipping them for
> this moment to keep it simple.
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
> 1 file changed, 92 insertions(+), 92 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 5aac4d1b..71382f70 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -39,8 +39,8 @@ static int ispow2(unsigned int i);
> * The configured block and sector sizes are defined as global variables so
> * that they don't need to be passed to functions that require them.
> */
> -unsigned int blocksize;
> -unsigned int sectorsize;
> +__uint64_t blocksize;
> +__uint64_t sectorsize;
>
> #define MAX_SUBOPTS 16
> #define SUBOPT_NEEDS_VAL (-1LL)
> @@ -742,9 +742,9 @@ calc_stripe_factors(
> int dsectsz,
> int lsu,
> int lsectsz,
> - int *dsunit,
> - int *dswidth,
> - int *lsunit)
> + __uint64_t *dsunit,
> + __uint64_t *dswidth,
> + __uint64_t *lsunit)
My, what big raid stripes you have! ;)
> {
> /* Handle data sunit/swidth options */
> if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
> @@ -769,32 +769,32 @@ calc_stripe_factors(
> usage();
> }
>
> - *dsunit = (int)BTOBBT(dsu);
> + *dsunit = BTOBBT(dsu);
> *dswidth = *dsunit * dsw;
> }
>
> if (*dsunit && (*dswidth % *dsunit != 0)) {
> fprintf(stderr,
> - _("data stripe width (%d) must be a multiple of the "
> - "data stripe unit (%d)\n"), *dswidth, *dsunit);
> + _("data stripe width (%lu) must be a multiple of the "
> + "data stripe unit (%lu)\n"), *dswidth, *dsunit);
> usage();
> }
>
> /* Handle log sunit options */
>
> if (lsu)
> - *lsunit = (int)BTOBBT(lsu);
> + *lsunit = BTOBBT(lsu);
>
> /* verify if lsu/lsunit is a multiple block size */
> if (lsu % blocksize != 0) {
> fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%d) must be a multiple of the block size (%lu)\n"),
> lsu, blocksize);
> exit(1);
> }
> if ((BBTOB(*lsunit) % blocksize != 0)) {
> fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%lu) must be a multiple of the block size (%lu)\n"),
> BBTOB(*lsunit), blocksize);
> exit(1);
> }
> @@ -917,7 +917,7 @@ fixup_internal_log_stripe(
> int sunit,
> xfs_rfsblock_t *logblocks,
> int blocklog,
> - int *lalign)
> + __uint64_t *lalign)
> {
> if ((logstart % sunit) != 0) {
> logstart = ((logstart + (sunit - 1))/sunit) * sunit;
> @@ -1405,70 +1405,70 @@ main(
> __uint64_t agsize;
> xfs_alloc_rec_t *arec;
> struct xfs_btree_block *block;
> - int blflag;
> - int blocklog;
> - int bsflag;
> - int bsize;
> + __uint64_t blflag;
Why do we need 64 bits to store a boolean flag? Shouldn't bool (or just
leaving the flags as int) be enough?
> + __uint64_t blocklog;
> + __uint64_t bsflag;
> + __uint64_t bsize;
I'm wondering why __uint64_t instead of uint64_t?
> xfs_buf_t *buf;
> - int c;
> - int daflag;
> - int dasize;
> + __uint64_t c;
> + __uint64_t daflag;
> + __uint64_t dasize;
> xfs_rfsblock_t dblocks;
> char *dfile;
> - int dirblocklog;
> - int dirblocksize;
> + __uint64_t dirblocklog;
> + __uint64_t dirblocksize;
> char *dsize;
> - int dsu;
> - int dsw;
> - int dsunit;
> - int dswidth;
> - int force_overwrite;
> + __uint64_t dsu;
> + __uint64_t dsw;
> + __uint64_t dsunit;
> + __uint64_t dswidth;
> + __uint64_t force_overwrite;
> struct fsxattr fsx;
> - int ilflag;
> - int imaxpct;
> - int imflag;
> - int inodelog;
> - int inopblock;
> - int ipflag;
> - int isflag;
> - int isize;
> + __uint64_t ilflag;
> + __uint64_t imaxpct;
> + __uint64_t imflag;
> + __uint64_t inodelog;
> + __uint64_t inopblock;
> + __uint64_t ipflag;
> + __uint64_t isflag;
> + __uint64_t isize;
> char *label = NULL;
> - int laflag;
> - int lalign;
> - int ldflag;
> - int liflag;
> + __uint64_t laflag;
> + __uint64_t lalign;
> + __uint64_t ldflag;
> + __uint64_t liflag;
> xfs_agnumber_t logagno;
> xfs_rfsblock_t logblocks;
> char *logfile;
> - int loginternal;
> + __uint64_t loginternal;
> char *logsize;
> xfs_fsblock_t logstart;
> - int lvflag;
> - int lsflag;
> - int lsuflag;
> - int lsunitflag;
> - int lsectorlog;
> - int lsectorsize;
> - int lslflag;
> - int lssflag;
> - int lsu;
> - int lsunit;
> - int min_logblocks;
> + __uint64_t lvflag;
> + __uint64_t lsflag;
> + __uint64_t lsuflag;
> + __uint64_t lsunitflag;
> + __uint64_t lsectorlog;
> + __uint64_t lsectorsize;
> + __uint64_t lslflag;
> + __uint64_t lssflag;
> + __uint64_t lsu;
> + __uint64_t lsunit;
> + __uint64_t min_logblocks;
> xfs_mount_t *mp;
> xfs_mount_t mbuf;
> xfs_extlen_t nbmblocks;
> - int nlflag;
> - int nodsflag;
> - int norsflag;
> + __uint64_t nlflag;
> + __uint64_t nodsflag;
> + __uint64_t norsflag;
> xfs_alloc_rec_t *nrec;
> - int nsflag;
> - int nvflag;
> - int Nflag;
> - int discard = 1;
> + __uint64_t nsflag;
> + __uint64_t nvflag;
> + __uint64_t Nflag;
> + __uint64_t discard = 1;
> char *p;
> char *protofile;
> char *protostring;
> - int qflag;
> + __uint64_t qflag;
> xfs_rfsblock_t rtblocks;
> xfs_extlen_t rtextblocks;
> xfs_rtblock_t rtextents;
> @@ -1476,13 +1476,13 @@ main(
> char *rtfile;
> char *rtsize;
> xfs_sb_t *sbp;
> - int sectorlog;
> + __uint64_t sectorlog;
> __uint64_t sector_mask;
> - int slflag;
> - int ssflag;
> + __uint64_t slflag;
> + __uint64_t ssflag;
> __uint64_t tmp_agsize;
> uuid_t uuid;
> - int worst_freelist;
> + __uint64_t worst_freelist;
> libxfs_init_t xi;
> struct fs_topology ft;
> struct sb_feat_args sb_feat = {
> @@ -1946,7 +1946,7 @@ main(
> blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG;
> }
> if (blocksize < XFS_MIN_BLOCKSIZE || blocksize > XFS_MAX_BLOCKSIZE) {
> - fprintf(stderr, _("illegal block size %d\n"), blocksize);
> + fprintf(stderr, _("illegal block size %lu\n"), blocksize);
"illegal block size %"PRIu64"\n", because uint64_t can be unsigned long
long on 32bit.
--D
> usage();
> }
> if (sb_feat.crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> @@ -2010,7 +2010,7 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
>
> if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) {
> fprintf(stderr,
> -_("specified blocksize %d is less than device physical sector size %d\n"),
> +_("specified blocksize %lu is less than device physical sector size %d\n"),
> blocksize, ft.psectorsize);
> fprintf(stderr,
> _("switching to logical sector size %d\n"),
> @@ -2031,21 +2031,21 @@ _("switching to logical sector size %d\n"),
> if (sectorsize < XFS_MIN_SECTORSIZE ||
> sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
> if (ssflag)
> - fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
> + fprintf(stderr, _("illegal sector size %lu\n"), sectorsize);
> else
> fprintf(stderr,
> -_("block size %d cannot be smaller than logical sector size %d\n"),
> +_("block size %lu cannot be smaller than logical sector size %d\n"),
> blocksize, ft.lsectorsize);
> usage();
> }
> if (sectorsize < ft.lsectorsize) {
> - fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
> + fprintf(stderr, _("illegal sector size %lu; hw sector is %d\n"),
> sectorsize, ft.lsectorsize);
> usage();
> }
> if (lsectorsize < XFS_MIN_SECTORSIZE ||
> lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
> - fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
> + fprintf(stderr, _("illegal log sector size %lu\n"), lsectorsize);
> usage();
> } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> lsu = blocksize;
> @@ -2151,7 +2151,7 @@ _("rmapbt not supported with realtime devices\n"));
> if (nsflag || nlflag) {
> if (dirblocksize < blocksize ||
> dirblocksize > XFS_MAX_BLOCKSIZE) {
> - fprintf(stderr, _("illegal directory block size %d\n"),
> + fprintf(stderr, _("illegal directory block size %lu\n"),
> dirblocksize);
> usage();
> }
> @@ -2177,7 +2177,7 @@ _("rmapbt not supported with realtime devices\n"));
> dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
> if (dbytes % blocksize)
> fprintf(stderr, _("warning: "
> - "data length %lld not a multiple of %d, truncated to %lld\n"),
> + "data length %lld not a multiple of %lu, truncated to %lld\n"),
> (long long)dbytes, blocksize,
> (long long)(dblocks << blocklog));
> }
> @@ -2209,7 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
> logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
> if (logbytes % blocksize)
> fprintf(stderr,
> - _("warning: log length %lld not a multiple of %d, truncated to %lld\n"),
> + _("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
> (long long)logbytes, blocksize,
> (long long)(logblocks << blocklog));
> }
> @@ -2226,7 +2226,7 @@ _("rmapbt not supported with realtime devices\n"));
> rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
> if (rtbytes % blocksize)
> fprintf(stderr,
> - _("warning: rt length %lld not a multiple of %d, truncated to %lld\n"),
> + _("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
> (long long)rtbytes, blocksize,
> (long long)(rtblocks << blocklog));
> }
> @@ -2239,7 +2239,7 @@ _("rmapbt not supported with realtime devices\n"));
> rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
> if (rtextbytes % blocksize) {
> fprintf(stderr,
> - _("illegal rt extent size %lld, not a multiple of %d\n"),
> + _("illegal rt extent size %lld, not a multiple of %lu\n"),
> (long long)rtextbytes, blocksize);
> usage();
> }
> @@ -2282,16 +2282,16 @@ _("rmapbt not supported with realtime devices\n"));
> isize > XFS_DINODE_MAX_SIZE) {
> int maxsz;
>
> - fprintf(stderr, _("illegal inode size %d\n"), isize);
> + fprintf(stderr, _("illegal inode size %lu\n"), isize);
> maxsz = MIN(blocksize / XFS_MIN_INODE_PERBLOCK,
> XFS_DINODE_MAX_SIZE);
> if (XFS_DINODE_MIN_SIZE == maxsz)
> fprintf(stderr,
> - _("allowable inode size with %d byte blocks is %d\n"),
> + _("allowable inode size with %lu byte blocks is %d\n"),
> blocksize, XFS_DINODE_MIN_SIZE);
> else
> fprintf(stderr,
> - _("allowable inode size with %d byte blocks is between %d and %d\n"),
> + _("allowable inode size with %lu byte blocks is between %d and %d\n"),
> blocksize, XFS_DINODE_MIN_SIZE, maxsz);
> exit(1);
> }
> @@ -2395,19 +2395,19 @@ _("rmapbt not supported with realtime devices\n"));
>
> if (xi.dbsize > sectorsize) {
> fprintf(stderr, _(
> -"Warning: the data subvolume sector size %u is less than the sector size \n\
> +"Warning: the data subvolume sector size %lu is less than the sector size \n\
> reported by the device (%u).\n"),
> sectorsize, xi.dbsize);
> }
> if (!loginternal && xi.lbsize > lsectorsize) {
> fprintf(stderr, _(
> -"Warning: the log subvolume sector size %u is less than the sector size\n\
> +"Warning: the log subvolume sector size %lu is less than the sector size\n\
> reported by the device (%u).\n"),
> lsectorsize, xi.lbsize);
> }
> if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
> fprintf(stderr, _(
> -"Warning: the realtime subvolume sector size %u is less than the sector size\n\
> +"Warning: the realtime subvolume sector size %lu is less than the sector size\n\
> reported by the device (%u).\n"),
> sectorsize, xi.rtbsize);
> }
> @@ -2437,14 +2437,14 @@ reported by the device (%u).\n"),
> if (dsunit) {
> if (ft.dsunit && ft.dsunit != dsunit) {
> fprintf(stderr,
> - _("%s: Specified data stripe unit %d "
> + _("%s: Specified data stripe unit %lu "
> "is not the same as the volume stripe "
> "unit %d\n"),
> progname, dsunit, ft.dsunit);
> }
> if (ft.dswidth && ft.dswidth != dswidth) {
> fprintf(stderr,
> - _("%s: Specified data stripe width %d "
> + _("%s: Specified data stripe width %lu "
> "is not the same as the volume stripe "
> "width %d\n"),
> progname, dswidth, ft.dswidth);
> @@ -2462,7 +2462,7 @@ reported by the device (%u).\n"),
> */
> if (agsize % blocksize) {
> fprintf(stderr,
> - _("agsize (%lld) not a multiple of fs blk size (%d)\n"),
> + _("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
> (long long)agsize, blocksize);
> usage();
> }
> @@ -2512,7 +2512,7 @@ reported by the device (%u).\n"),
> (dblocks % agsize != 0);
> if (dasize)
> fprintf(stderr,
> - _("agsize rounded to %lld, swidth = %d\n"),
> + _("agsize rounded to %lld, swidth = %lu\n"),
> (long long)agsize, dswidth);
> } else {
> if (nodsflag) {
> @@ -2569,8 +2569,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
> dsunit = dswidth = 0;
> else {
> fprintf(stderr,
> - _("%s: Stripe unit(%d) or stripe width(%d) is "
> - "not a multiple of the block size(%d)\n"),
> + _("%s: Stripe unit(%lu) or stripe width(%lu) is "
> + "not a multiple of the block size(%lu)\n"),
> progname, BBTOB(dsunit), BBTOB(dswidth),
> blocksize);
> exit(1);
> @@ -2610,7 +2610,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
> /* Warn only if specified on commandline */
> if (lsuflag || lsunitflag) {
> fprintf(stderr,
> - _("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
> + _("log stripe unit (%lu bytes) is too large (maximum is 256KiB)\n"),
> (lsunit * blocksize));
> fprintf(stderr,
> _("log stripe unit adjusted to 32KiB\n"));
> @@ -2755,14 +2755,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
>
> if (!qflag || Nflag) {
> printf(_(
> - "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> - " =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> + "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
> + " =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
> " =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> - "data =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
> - " =%-22s sunit=%-6u swidth=%u blks\n"
> - "naming =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
> + "data =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
> + " =%-22s sunit=%-6lu swidth=%lu blks\n"
> + "naming =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
> "log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> - " =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> + " =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
> "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> dfile, isize, (long long)agcount, (long long)agsize,
> "", sectorsize, sb_feat.attr_version,
> --
> 2.12.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-06 14:41 [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
` (2 preceding siblings ...)
2017-04-06 19:46 ` Darrick J. Wong
@ 2017-04-06 20:13 ` Eric Sandeen
2017-04-07 1:50 ` Dave Chinner
4 siblings, 0 replies; 10+ messages in thread
From: Eric Sandeen @ 2017-04-06 20:13 UTC (permalink / raw)
To: Jan Tulak, linux-xfs; +Cc: mcgrof
On 4/6/17 9:41 AM, Jan Tulak wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
>
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
>
> This patch changes variables declared at the beginning of main() +
> block/sectorsize, making only minimal changes. The following patch
> cleans some now-unnecessary type casts.
>
> It would be nice to change types in some of the structures too, but
> this might lead to changes outside of mkfs, so I'm skipping them for
> this moment to keep it simple.
This changelog doesn't really say why we'd want to make this change
in all of the variables you've modified.
For example, you're taking simple flags like qflag - which sure does look
like it should be a /bool/ - and making it 64 bits:
- int qflag;
+ __uint64_t qflag;
why?
I don't understand what is improved by this sort of thing. It is only
used as a simple flag, even after your other work, as far as I can tell.
There is no casting, no conversion, nothing ... Same goes for many other
variables changed by this patch.
If choosing proper types avoids casts back and forth in some places,
sure, that makes sense - pick the proper type, or the superset type
as needed for conversions - but "consistency" is not a reason to turn
boolean flags into 64-bit variables, as far as I can tell...
Also, as Darrick mentioned I'm not sure why we'd use the __uint64_t
type instead of uint64_t in this code?
And, you've changed printf formats for these to %lu, but this will go all
to hell on a 32-bit build. As Darrick mentioned, you'll need the funky
PRIu64 formatter stuff if you want to go this way.
I know you & Luis discussed this, but I'm just not getting it; "consistency"
alone is not a reason to make every variable 64 bits, IMHO. If it serves
some greater purpose, please make that purpose more clear in the changelog...
-Eric
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 184 ++++++++++++++++++++++++++++----------------------------
> 1 file changed, 92 insertions(+), 92 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 5aac4d1b..71382f70 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -39,8 +39,8 @@ static int ispow2(unsigned int i);
> * The configured block and sector sizes are defined as global variables so
> * that they don't need to be passed to functions that require them.
> */
> -unsigned int blocksize;
> -unsigned int sectorsize;
> +__uint64_t blocksize;
> +__uint64_t sectorsize;
>
> #define MAX_SUBOPTS 16
> #define SUBOPT_NEEDS_VAL (-1LL)
> @@ -742,9 +742,9 @@ calc_stripe_factors(
> int dsectsz,
> int lsu,
> int lsectsz,
> - int *dsunit,
> - int *dswidth,
> - int *lsunit)
> + __uint64_t *dsunit,
> + __uint64_t *dswidth,
> + __uint64_t *lsunit)
> {
> /* Handle data sunit/swidth options */
> if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
> @@ -769,32 +769,32 @@ calc_stripe_factors(
> usage();
> }
>
> - *dsunit = (int)BTOBBT(dsu);
> + *dsunit = BTOBBT(dsu);
> *dswidth = *dsunit * dsw;
> }
>
> if (*dsunit && (*dswidth % *dsunit != 0)) {
> fprintf(stderr,
> - _("data stripe width (%d) must be a multiple of the "
> - "data stripe unit (%d)\n"), *dswidth, *dsunit);
> + _("data stripe width (%lu) must be a multiple of the "
> + "data stripe unit (%lu)\n"), *dswidth, *dsunit);
> usage();
> }
>
> /* Handle log sunit options */
>
> if (lsu)
> - *lsunit = (int)BTOBBT(lsu);
> + *lsunit = BTOBBT(lsu);
>
> /* verify if lsu/lsunit is a multiple block size */
> if (lsu % blocksize != 0) {
> fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%d) must be a multiple of the block size (%lu)\n"),
> lsu, blocksize);
> exit(1);
> }
> if ((BBTOB(*lsunit) % blocksize != 0)) {
> fprintf(stderr,
> -_("log stripe unit (%d) must be a multiple of the block size (%d)\n"),
> +_("log stripe unit (%lu) must be a multiple of the block size (%lu)\n"),
> BBTOB(*lsunit), blocksize);
> exit(1);
> }
> @@ -917,7 +917,7 @@ fixup_internal_log_stripe(
> int sunit,
> xfs_rfsblock_t *logblocks,
> int blocklog,
> - int *lalign)
> + __uint64_t *lalign)
> {
> if ((logstart % sunit) != 0) {
> logstart = ((logstart + (sunit - 1))/sunit) * sunit;
> @@ -1405,70 +1405,70 @@ main(
> __uint64_t agsize;
> xfs_alloc_rec_t *arec;
> struct xfs_btree_block *block;
> - int blflag;
> - int blocklog;
> - int bsflag;
> - int bsize;
> + __uint64_t blflag;
> + __uint64_t blocklog;
> + __uint64_t bsflag;
> + __uint64_t bsize;
> xfs_buf_t *buf;
> - int c;
> - int daflag;
> - int dasize;
> + __uint64_t c;
> + __uint64_t daflag;
> + __uint64_t dasize;
> xfs_rfsblock_t dblocks;
> char *dfile;
> - int dirblocklog;
> - int dirblocksize;
> + __uint64_t dirblocklog;
> + __uint64_t dirblocksize;
> char *dsize;
> - int dsu;
> - int dsw;
> - int dsunit;
> - int dswidth;
> - int force_overwrite;
> + __uint64_t dsu;
> + __uint64_t dsw;
> + __uint64_t dsunit;
> + __uint64_t dswidth;
> + __uint64_t force_overwrite;
> struct fsxattr fsx;
> - int ilflag;
> - int imaxpct;
> - int imflag;
> - int inodelog;
> - int inopblock;
> - int ipflag;
> - int isflag;
> - int isize;
> + __uint64_t ilflag;
> + __uint64_t imaxpct;
> + __uint64_t imflag;
> + __uint64_t inodelog;
> + __uint64_t inopblock;
> + __uint64_t ipflag;
> + __uint64_t isflag;
> + __uint64_t isize;
> char *label = NULL;
> - int laflag;
> - int lalign;
> - int ldflag;
> - int liflag;
> + __uint64_t laflag;
> + __uint64_t lalign;
> + __uint64_t ldflag;
> + __uint64_t liflag;
> xfs_agnumber_t logagno;
> xfs_rfsblock_t logblocks;
> char *logfile;
> - int loginternal;
> + __uint64_t loginternal;
> char *logsize;
> xfs_fsblock_t logstart;
> - int lvflag;
> - int lsflag;
> - int lsuflag;
> - int lsunitflag;
> - int lsectorlog;
> - int lsectorsize;
> - int lslflag;
> - int lssflag;
> - int lsu;
> - int lsunit;
> - int min_logblocks;
> + __uint64_t lvflag;
> + __uint64_t lsflag;
> + __uint64_t lsuflag;
> + __uint64_t lsunitflag;
> + __uint64_t lsectorlog;
> + __uint64_t lsectorsize;
> + __uint64_t lslflag;
> + __uint64_t lssflag;
> + __uint64_t lsu;
> + __uint64_t lsunit;
> + __uint64_t min_logblocks;
> xfs_mount_t *mp;
> xfs_mount_t mbuf;
> xfs_extlen_t nbmblocks;
> - int nlflag;
> - int nodsflag;
> - int norsflag;
> + __uint64_t nlflag;
> + __uint64_t nodsflag;
> + __uint64_t norsflag;
> xfs_alloc_rec_t *nrec;
> - int nsflag;
> - int nvflag;
> - int Nflag;
> - int discard = 1;
> + __uint64_t nsflag;
> + __uint64_t nvflag;
> + __uint64_t Nflag;
> + __uint64_t discard = 1;
> char *p;
> char *protofile;
> char *protostring;
> - int qflag;
> + __uint64_t qflag;
> xfs_rfsblock_t rtblocks;
> xfs_extlen_t rtextblocks;
> xfs_rtblock_t rtextents;
> @@ -1476,13 +1476,13 @@ main(
> char *rtfile;
> char *rtsize;
> xfs_sb_t *sbp;
> - int sectorlog;
> + __uint64_t sectorlog;
> __uint64_t sector_mask;
> - int slflag;
> - int ssflag;
> + __uint64_t slflag;
> + __uint64_t ssflag;
> __uint64_t tmp_agsize;
> uuid_t uuid;
> - int worst_freelist;
> + __uint64_t worst_freelist;
> libxfs_init_t xi;
> struct fs_topology ft;
> struct sb_feat_args sb_feat = {
> @@ -1946,7 +1946,7 @@ main(
> blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG;
> }
> if (blocksize < XFS_MIN_BLOCKSIZE || blocksize > XFS_MAX_BLOCKSIZE) {
> - fprintf(stderr, _("illegal block size %d\n"), blocksize);
> + fprintf(stderr, _("illegal block size %lu\n"), blocksize);
> usage();
> }
> if (sb_feat.crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> @@ -2010,7 +2010,7 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
>
> if ((blocksize < sectorsize) && (blocksize >= ft.lsectorsize)) {
> fprintf(stderr,
> -_("specified blocksize %d is less than device physical sector size %d\n"),
> +_("specified blocksize %lu is less than device physical sector size %d\n"),
> blocksize, ft.psectorsize);
> fprintf(stderr,
> _("switching to logical sector size %d\n"),
> @@ -2031,21 +2031,21 @@ _("switching to logical sector size %d\n"),
> if (sectorsize < XFS_MIN_SECTORSIZE ||
> sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
> if (ssflag)
> - fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
> + fprintf(stderr, _("illegal sector size %lu\n"), sectorsize);
> else
> fprintf(stderr,
> -_("block size %d cannot be smaller than logical sector size %d\n"),
> +_("block size %lu cannot be smaller than logical sector size %d\n"),
> blocksize, ft.lsectorsize);
> usage();
> }
> if (sectorsize < ft.lsectorsize) {
> - fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
> + fprintf(stderr, _("illegal sector size %lu; hw sector is %d\n"),
> sectorsize, ft.lsectorsize);
> usage();
> }
> if (lsectorsize < XFS_MIN_SECTORSIZE ||
> lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
> - fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
> + fprintf(stderr, _("illegal log sector size %lu\n"), lsectorsize);
> usage();
> } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> lsu = blocksize;
> @@ -2151,7 +2151,7 @@ _("rmapbt not supported with realtime devices\n"));
> if (nsflag || nlflag) {
> if (dirblocksize < blocksize ||
> dirblocksize > XFS_MAX_BLOCKSIZE) {
> - fprintf(stderr, _("illegal directory block size %d\n"),
> + fprintf(stderr, _("illegal directory block size %lu\n"),
> dirblocksize);
> usage();
> }
> @@ -2177,7 +2177,7 @@ _("rmapbt not supported with realtime devices\n"));
> dblocks = (xfs_rfsblock_t)(dbytes >> blocklog);
> if (dbytes % blocksize)
> fprintf(stderr, _("warning: "
> - "data length %lld not a multiple of %d, truncated to %lld\n"),
> + "data length %lld not a multiple of %lu, truncated to %lld\n"),
> (long long)dbytes, blocksize,
> (long long)(dblocks << blocklog));
> }
> @@ -2209,7 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
> logblocks = (xfs_rfsblock_t)(logbytes >> blocklog);
> if (logbytes % blocksize)
> fprintf(stderr,
> - _("warning: log length %lld not a multiple of %d, truncated to %lld\n"),
> + _("warning: log length %lld not a multiple of %lu, truncated to %lld\n"),
> (long long)logbytes, blocksize,
> (long long)(logblocks << blocklog));
> }
> @@ -2226,7 +2226,7 @@ _("rmapbt not supported with realtime devices\n"));
> rtblocks = (xfs_rfsblock_t)(rtbytes >> blocklog);
> if (rtbytes % blocksize)
> fprintf(stderr,
> - _("warning: rt length %lld not a multiple of %d, truncated to %lld\n"),
> + _("warning: rt length %lld not a multiple of %lu, truncated to %lld\n"),
> (long long)rtbytes, blocksize,
> (long long)(rtblocks << blocklog));
> }
> @@ -2239,7 +2239,7 @@ _("rmapbt not supported with realtime devices\n"));
> rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
> if (rtextbytes % blocksize) {
> fprintf(stderr,
> - _("illegal rt extent size %lld, not a multiple of %d\n"),
> + _("illegal rt extent size %lld, not a multiple of %lu\n"),
> (long long)rtextbytes, blocksize);
> usage();
> }
> @@ -2282,16 +2282,16 @@ _("rmapbt not supported with realtime devices\n"));
> isize > XFS_DINODE_MAX_SIZE) {
> int maxsz;
>
> - fprintf(stderr, _("illegal inode size %d\n"), isize);
> + fprintf(stderr, _("illegal inode size %lu\n"), isize);
> maxsz = MIN(blocksize / XFS_MIN_INODE_PERBLOCK,
> XFS_DINODE_MAX_SIZE);
> if (XFS_DINODE_MIN_SIZE == maxsz)
> fprintf(stderr,
> - _("allowable inode size with %d byte blocks is %d\n"),
> + _("allowable inode size with %lu byte blocks is %d\n"),
> blocksize, XFS_DINODE_MIN_SIZE);
> else
> fprintf(stderr,
> - _("allowable inode size with %d byte blocks is between %d and %d\n"),
> + _("allowable inode size with %lu byte blocks is between %d and %d\n"),
> blocksize, XFS_DINODE_MIN_SIZE, maxsz);
> exit(1);
> }
> @@ -2395,19 +2395,19 @@ _("rmapbt not supported with realtime devices\n"));
>
> if (xi.dbsize > sectorsize) {
> fprintf(stderr, _(
> -"Warning: the data subvolume sector size %u is less than the sector size \n\
> +"Warning: the data subvolume sector size %lu is less than the sector size \n\
> reported by the device (%u).\n"),
> sectorsize, xi.dbsize);
> }
> if (!loginternal && xi.lbsize > lsectorsize) {
> fprintf(stderr, _(
> -"Warning: the log subvolume sector size %u is less than the sector size\n\
> +"Warning: the log subvolume sector size %lu is less than the sector size\n\
> reported by the device (%u).\n"),
> lsectorsize, xi.lbsize);
> }
> if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
> fprintf(stderr, _(
> -"Warning: the realtime subvolume sector size %u is less than the sector size\n\
> +"Warning: the realtime subvolume sector size %lu is less than the sector size\n\
> reported by the device (%u).\n"),
> sectorsize, xi.rtbsize);
> }
> @@ -2437,14 +2437,14 @@ reported by the device (%u).\n"),
> if (dsunit) {
> if (ft.dsunit && ft.dsunit != dsunit) {
> fprintf(stderr,
> - _("%s: Specified data stripe unit %d "
> + _("%s: Specified data stripe unit %lu "
> "is not the same as the volume stripe "
> "unit %d\n"),
> progname, dsunit, ft.dsunit);
> }
> if (ft.dswidth && ft.dswidth != dswidth) {
> fprintf(stderr,
> - _("%s: Specified data stripe width %d "
> + _("%s: Specified data stripe width %lu "
> "is not the same as the volume stripe "
> "width %d\n"),
> progname, dswidth, ft.dswidth);
> @@ -2462,7 +2462,7 @@ reported by the device (%u).\n"),
> */
> if (agsize % blocksize) {
> fprintf(stderr,
> - _("agsize (%lld) not a multiple of fs blk size (%d)\n"),
> + _("agsize (%lld) not a multiple of fs blk size (%lu)\n"),
> (long long)agsize, blocksize);
> usage();
> }
> @@ -2512,7 +2512,7 @@ reported by the device (%u).\n"),
> (dblocks % agsize != 0);
> if (dasize)
> fprintf(stderr,
> - _("agsize rounded to %lld, swidth = %d\n"),
> + _("agsize rounded to %lld, swidth = %lu\n"),
> (long long)agsize, dswidth);
> } else {
> if (nodsflag) {
> @@ -2569,8 +2569,8 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
> dsunit = dswidth = 0;
> else {
> fprintf(stderr,
> - _("%s: Stripe unit(%d) or stripe width(%d) is "
> - "not a multiple of the block size(%d)\n"),
> + _("%s: Stripe unit(%lu) or stripe width(%lu) is "
> + "not a multiple of the block size(%lu)\n"),
> progname, BBTOB(dsunit), BBTOB(dswidth),
> blocksize);
> exit(1);
> @@ -2610,7 +2610,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
> /* Warn only if specified on commandline */
> if (lsuflag || lsunitflag) {
> fprintf(stderr,
> - _("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
> + _("log stripe unit (%lu bytes) is too large (maximum is 256KiB)\n"),
> (lsunit * blocksize));
> fprintf(stderr,
> _("log stripe unit adjusted to 32KiB\n"));
> @@ -2755,14 +2755,14 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
>
> if (!qflag || Nflag) {
> printf(_(
> - "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> - " =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> + "meta-data=%-22s isize=%-6lu agcount=%lld, agsize=%lld blks\n"
> + " =%-22s sectsz=%-5lu attr=%u, projid32bit=%u\n"
> " =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> - "data =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
> - " =%-22s sunit=%-6u swidth=%u blks\n"
> - "naming =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
> + "data =%-22s bsize=%-6lu blocks=%llu, imaxpct=%lu\n"
> + " =%-22s sunit=%-6lu swidth=%lu blks\n"
> + "naming =version %-14u bsize=%-6lu ascii-ci=%d ftype=%d\n"
> "log =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> - " =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> + " =%-22s sectsz=%-5lu sunit=%lu blks, lazy-count=%d\n"
> "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> dfile, isize, (long long)agcount, (long long)agsize,
> "", sectorsize, sb_feat.attr_version,
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-06 14:41 [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main() Jan Tulak
` (3 preceding siblings ...)
2017-04-06 20:13 ` Eric Sandeen
@ 2017-04-07 1:50 ` Dave Chinner
2017-04-07 13:17 ` Jan Tulak
4 siblings, 1 reply; 10+ messages in thread
From: Dave Chinner @ 2017-04-07 1:50 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs, mcgrof, sandeen
On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>
> In the past, when mkfs was first written, it used atoi and
> similar calls, so the variables were ints. However, the situation moved
> since then and in course of the time, mkfs began to use other types too.
>
> Clean and unify it. We don't need negative values anywhere in the code and
> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> type.
I'm with Darrick and Eric on this - it's not the right conversion to
make for all the reasons they've pointed out. Further, I think it's
the wrong direction to be working in.
What I originally intended the config option table to be used for
was to /replace all these config variables/ with config option table
lookups. We don't need tens of variables to say we certain options
set - once option parsing is complete we can just lookup the config
table and use the option value directly. i.e. we need to work
towards removing all the variables, not try to make them pretty....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-07 1:50 ` Dave Chinner
@ 2017-04-07 13:17 ` Jan Tulak
2017-04-08 23:59 ` Dave Chinner
0 siblings, 1 reply; 10+ messages in thread
From: Jan Tulak @ 2017-04-07 13:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, Luis R. Rodriguez, Eric Sandeen
On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
>> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>>
>> In the past, when mkfs was first written, it used atoi and
>> similar calls, so the variables were ints. However, the situation moved
>> since then and in course of the time, mkfs began to use other types too.
>>
>> Clean and unify it. We don't need negative values anywhere in the code and
>> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
>> type.
>
> I'm with Darrick and Eric on this - it's not the right conversion to
> make for all the reasons they've pointed out. Further, I think it's
> the wrong direction to be working in.
>
> What I originally intended the config option table to be used for
> was to /replace all these config variables/ with config option table
> lookups. We don't need tens of variables to say we certain options
> set - once option parsing is complete we can just lookup the config
> table and use the option value directly. i.e. we need to work
> towards removing all the variables, not try to make them pretty....
>
Removing them entirely is not as easy... Right now, there is this
thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
opts struct values" in the main:
(Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
- __uint64_t agcount;
+ __uint64_t *agcount;
...
+ /*
+ * Set up pointers, so we can use shorter names and to let gcc
+ * check the correct type. We don't want to inadvertently use an int as
+ * unsigned int and so on...
+ */
+ agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
...
The comment states, why the variables are still here and with this
code, it still makes sense to make (at least for some variables) the
transformation. I tried to come up with another way how to make a
short and type-safe access to the options without the variables, but
so far, I didn't got anywhere.
Clearly, we can't use a macro, because we need to access the value of
a member of a struct to decide what type the option is. The only thing
I could come up that seems remotely working is to make a set of
functions like these for every type:
int get_opt_int(opt, sub)
int set_opt_int(opt, sub, number)
It would require us to always remember what suffix we have to use and
it is not possible to do direct assignments with "="(that is why I
transformed the variables into pointers in the patchset). But at least
it could be possible to catch an incorrect type use easily, something
we couldn't do in the macro:
int get_opt_int(opt, sub)
{
if (opts[opt].subopt_params[sub].type != INT)
print an error and exit();
return opts[opt].subopt_params[sub].value.int;
}
If you find this better than the pointers, I can change it and then
this unifying patch is rather useless. But if not, then it makes sense
to do something with the types now, rather than in the linked patch.
Ultimately, if we decide to have only one numeric type, then the type
safety is not much of an issue. Because you can't get it wrong and one
macro for a number and one for a string would suffice then.
So to the other points from Eric and Darrick:
>> - int *dsunit,
>> - int *dswidth,
>> - int *lsunit)
>> + __uint64_t *dsunit,
>> + __uint64_t *dswidth,
>> + __uint64_t *lsunit)
>
> My, what big raid stripes you have! ;)
Well, yes, 64 bits is not necessary here, but I would say that having
just one size of uint removes some (even if small) ambiguity, while
performance-wise, it won't do anything noticeable.
> For example, you're taking simple flags like qflag - which sure does look
> like it should be a /bool/ - and making it 64 bits:
and
>> - int bsflag;
>> - int bsize;
>> + __uint64_t blflag;
>
> Why do we need 64 bits to store a boolean flag? Shouldn't bool (or just
> leaving the flags as int) be enough?
The same as above. I admit it is wasting of space, though, and in case
of booleans it make more sense to turn them into a proper bool.
>
>> + __uint64_t blocklog;
>> + __uint64_t bsflag;
>> + __uint64_t bsize;
>
> I'm wondering why __uint64_t instead of uint64_t?
Because __uint64_t was already used for some numbers (e.g. agcount),
while uint64_t not, and it is defined in xfs_linux.h. So I thought
that it is safer to use xfs's own type, if it exists for some reason.
>> - fprintf(stderr, _("illegal block size %d\n"), blocksize);
>> + fprintf(stderr, _("illegal block size %lu\n"), blocksize);
>
> "illegal block size %"PRIu64"\n", because uint64_t can be unsigned long
> long on 32bit.
Mmm, ok, I'm making a note to have a 32bit VM at hand and try to at
least compile things there too. Then I would get a warning that would
force me to find the PRIu64 alternative...
> This changelog doesn't really say why we'd want to make this change
> in all of the variables you've modified.
I thought this as also a way to simplify the types in the opts
structure, ideally to just one numeric type and a string. So this is
in part how I picked most of the variables. And I added the rest
thinking: "Well, I'm changing most of them, so why keep the last few
ints in there? Let's convert them too."
But I now, after reading your comments, I think that adding a boolean
into the mix makes sense. Using 64 bits even where we won't need so
big numbers is not an issue, I would still use the 64 bits there, but
the boolean can be bool instead.
I hope I didn't forget anything and that it is understandable...
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-07 13:17 ` Jan Tulak
@ 2017-04-08 23:59 ` Dave Chinner
2017-04-10 8:42 ` Jan Tulak
0 siblings, 1 reply; 10+ messages in thread
From: Dave Chinner @ 2017-04-08 23:59 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs, Luis R. Rodriguez, Eric Sandeen
On Fri, Apr 07, 2017 at 03:17:20PM +0200, Jan Tulak wrote:
> On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
> >> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
> >>
> >> In the past, when mkfs was first written, it used atoi and
> >> similar calls, so the variables were ints. However, the situation moved
> >> since then and in course of the time, mkfs began to use other types too.
> >>
> >> Clean and unify it. We don't need negative values anywhere in the code and
> >> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
> >> type.
> >
> > I'm with Darrick and Eric on this - it's not the right conversion to
> > make for all the reasons they've pointed out. Further, I think it's
> > the wrong direction to be working in.
> >
> > What I originally intended the config option table to be used for
> > was to /replace all these config variables/ with config option table
> > lookups. We don't need tens of variables to say we certain options
> > set - once option parsing is complete we can just lookup the config
> > table and use the option value directly. i.e. we need to work
> > towards removing all the variables, not try to make them pretty....
> >
>
> Removing them entirely is not as easy... Right now, there is this
> thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
> opts struct values" in the main:
>
> (Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
>
> - __uint64_t agcount;
> + __uint64_t *agcount;
> ...
>
> + /*
> + * Set up pointers, so we can use shorter names and to let gcc
> + * check the correct type. We don't want to inadvertently use an int as
> + * unsigned int and so on...
> + */
> + agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
That's .... an interesting interpretation....
What I intended was replacing all the uses of the agcount variable
with calls like:
get_config_val(OPT_D, D_AGCOUNT)
[....]
> transformed the variables into pointers in the patchset). But at least
> it could be possible to catch an incorrect type use easily, something
> we couldn't do in the macro:
>
> int get_opt_int(opt, sub)
> {
> if (opts[opt].subopt_params[sub].type != INT)
> print an error and exit();
> return opts[opt].subopt_params[sub].value.int;
> }
Yes, but why do we need to add type checking like this? It seems to
me that you're trying to over-engineer a simple thing that does not
need to be complex. For options with integer/flag values:
/* return default value if nothing specified on the CLI */
static inline uint64_t
get_config_val(int opt, int subopt)
{
if (!opts[opt].subopt_params[subopt].seen)
return opts[opt].subopt_params[subopt].defaultval;
return opts[opt].subopt_params[subopt].value;
}
And for options that are strings (e.g. device names):
static inline char *
get_config_str(int opt, int subopt)
{
if (!opts[opt].subopt_params[subopt].str_seen)
return NULL;
return opts[opt].subopt_params[subopt].string;
}
That's all that is necessary. The config table is guaranteed to
contain valid default values, and by the time we get to checking
options we've done all the conflict/validity checking so we can
trust the config settings to be valid and just use them directly
like this.
> >> + __uint64_t *dswidth,
> >> + __uint64_t *lsunit)
> >
> > My, what big raid stripes you have! ;)
>
> Well, yes, 64 bits is not necessary here, but I would say that having
> just one size of uint removes some (even if small) ambiguity, while
> performance-wise, it won't do anything noticeable.
However, it makes me cringe when I read code written like this. You
say it removes ambiguity but to me, after more than 20 years of C
coding using appropriate types for variable contents, using uint64_t
for all variables (especially those that don't require 64 bit types)
introduces ambiguity and raises questions about the code quality.
e.g. declaring a flag as boolean means the author intended it to
only have two values (i.e. it's self documenting then intent of a
flag value!) whereas declaring them all as uint64_t makes me wonder
why the author of this code didn't know what the valid value range
for this variable is, why none of the reviewers cared either, what
happens if I put a really large value into the field instead of 0 or
1, if that was tested, etc. Using the right type removes all this
potential ambiguity in the use/value of the variables.....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-08 23:59 ` Dave Chinner
@ 2017-04-10 8:42 ` Jan Tulak
2017-04-13 9:41 ` Jan Tulak
0 siblings, 1 reply; 10+ messages in thread
From: Jan Tulak @ 2017-04-10 8:42 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, Luis R. Rodriguez, Eric Sandeen
On Sun, Apr 9, 2017 at 1:59 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Apr 07, 2017 at 03:17:20PM +0200, Jan Tulak wrote:
>> On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
>> >> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>> >>
>> >> In the past, when mkfs was first written, it used atoi and
>> >> similar calls, so the variables were ints. However, the situation moved
>> >> since then and in course of the time, mkfs began to use other types too.
>> >>
>> >> Clean and unify it. We don't need negative values anywhere in the code and
>> >> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
>> >> type.
>> >
>> > I'm with Darrick and Eric on this - it's not the right conversion to
>> > make for all the reasons they've pointed out. Further, I think it's
>> > the wrong direction to be working in.
>> >
>> > What I originally intended the config option table to be used for
>> > was to /replace all these config variables/ with config option table
>> > lookups. We don't need tens of variables to say we certain options
>> > set - once option parsing is complete we can just lookup the config
>> > table and use the option value directly. i.e. we need to work
>> > towards removing all the variables, not try to make them pretty....
>> >
>>
>> Removing them entirely is not as easy... Right now, there is this
>> thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
>> opts struct values" in the main:
>>
>> (Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
>>
>> - __uint64_t agcount;
>> + __uint64_t *agcount;
>> ...
>>
>> + /*
>> + * Set up pointers, so we can use shorter names and to let gcc
>> + * check the correct type. We don't want to inadvertently use an int as
>> + * unsigned int and so on...
>> + */
>> + agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
>
> That's .... an interesting interpretation....
>
> What I intended was replacing all the uses of the agcount variable
> with calls like:
>
> get_config_val(OPT_D, D_AGCOUNT)
>
> [....]
>
>> transformed the variables into pointers in the patchset). But at least
>> it could be possible to catch an incorrect type use easily, something
>> we couldn't do in the macro:
>>
>> int get_opt_int(opt, sub)
>> {
>> if (opts[opt].subopt_params[sub].type != INT)
>> print an error and exit();
>> return opts[opt].subopt_params[sub].value.int;
>> }
>
> Yes, but why do we need to add type checking like this? It seems to
> me that you're trying to over-engineer a simple thing that does not
> need to be complex. For options with integer/flag values:
>
> /* return default value if nothing specified on the CLI */
> static inline uint64_t
> get_config_val(int opt, int subopt)
> {
> if (!opts[opt].subopt_params[subopt].seen)
> return opts[opt].subopt_params[subopt].defaultval;
> return opts[opt].subopt_params[subopt].value;
> }
>
> And for options that are strings (e.g. device names):
>
> static inline char *
> get_config_str(int opt, int subopt)
> {
> if (!opts[opt].subopt_params[subopt].str_seen)
> return NULL;
> return opts[opt].subopt_params[subopt].string;
> }
>
> That's all that is necessary. The config table is guaranteed to
> contain valid default values, and by the time we get to checking
> options we've done all the conflict/validity checking so we can
> trust the config settings to be valid and just use them directly
> like this.
>
Yes, but to be able to do this, we have to remove the other numeric
types. As long we have int, uint, various sizes... then there isn't an
overarching data type that can be used, so we either lose part of the
value range, or we need multiple functions. If we have only the
uint64, then this works.
>> >> + __uint64_t *dswidth,
>> >> + __uint64_t *lsunit)
>> >
>> > My, what big raid stripes you have! ;)
>>
>> Well, yes, 64 bits is not necessary here, but I would say that having
>> just one size of uint removes some (even if small) ambiguity, while
>> performance-wise, it won't do anything noticeable.
>
> However, it makes me cringe when I read code written like this. You
> say it removes ambiguity but to me, after more than 20 years of C
> coding using appropriate types for variable contents, using uint64_t
> for all variables (especially those that don't require 64 bit types)
> introduces ambiguity and raises questions about the code quality.
>
> e.g. declaring a flag as boolean means the author intended it to
> only have two values (i.e. it's self documenting then intent of a
> flag value!) whereas declaring them all as uint64_t makes me wonder
> why the author of this code didn't know what the valid value range
> for this variable is, why none of the reviewers cared either, what
> happens if I put a really large value into the field instead of 0 or
> 1, if that was tested, etc. Using the right type removes all this
> potential ambiguity in the use/value of the variables.....
The boolean flag is something I acknowledged in my previous email; it
indeed makes sense to keep that as a boolean. With other types though,
that goes against the idea of a single get_config_val().
Cheers,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH 1/2] mkfs: unify numeric types of main variables in main()
2017-04-10 8:42 ` Jan Tulak
@ 2017-04-13 9:41 ` Jan Tulak
0 siblings, 0 replies; 10+ messages in thread
From: Jan Tulak @ 2017-04-13 9:41 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, Luis R. Rodriguez, Eric Sandeen
On Mon, Apr 10, 2017 at 10:42 AM, Jan Tulak <jtulak@redhat.com> wrote:
> On Sun, Apr 9, 2017 at 1:59 AM, Dave Chinner <david@fromorbit.com> wrote:
>> On Fri, Apr 07, 2017 at 03:17:20PM +0200, Jan Tulak wrote:
>>> On Fri, Apr 7, 2017 at 3:50 AM, Dave Chinner <david@fromorbit.com> wrote:
>>> > On Thu, Apr 06, 2017 at 04:41:38PM +0200, Jan Tulak wrote:
>>> >> Followup of my "[xfsprogs] Do we need so many data types for user input?" email.
>>> >>
>>> >> In the past, when mkfs was first written, it used atoi and
>>> >> similar calls, so the variables were ints. However, the situation moved
>>> >> since then and in course of the time, mkfs began to use other types too.
>>> >>
>>> >> Clean and unify it. We don't need negative values anywhere in the code and
>>> >> some numbers has to be 64bit. Thus, uint64 is the best candidate as the target
>>> >> type.
>>> >
>>> > I'm with Darrick and Eric on this - it's not the right conversion to
>>> > make for all the reasons they've pointed out. Further, I think it's
>>> > the wrong direction to be working in.
>>> >
>>> > What I originally intended the config option table to be used for
>>> > was to /replace all these config variables/ with config option table
>>> > lookups. We don't need tens of variables to say we certain options
>>> > set - once option parsing is complete we can just lookup the config
>>> > table and use the option value directly. i.e. we need to work
>>> > towards removing all the variables, not try to make them pretty....
>>> >
>>>
>>> Removing them entirely is not as easy... Right now, there is this
>>> thing in "[PATCH 17/22] mkfs: use old variables as pointers to the new
>>> opts struct values" in the main:
>>>
>>> (Link to the patch: https://www.spinics.net/lists/linux-xfs/msg04977.html)
>>>
>>> - __uint64_t agcount;
>>> + __uint64_t *agcount;
>>> ...
>>>
>>> + /*
>>> + * Set up pointers, so we can use shorter names and to let gcc
>>> + * check the correct type. We don't want to inadvertently use an int as
>>> + * unsigned int and so on...
>>> + */
>>> + agcount = &opts[OPT_D].subopt_params[D_AGCOUNT].value.uint64;
>>
>> That's .... an interesting interpretation....
>>
>> What I intended was replacing all the uses of the agcount variable
>> with calls like:
>>
>> get_config_val(OPT_D, D_AGCOUNT)
>>
>> [....]
>>
>>> transformed the variables into pointers in the patchset). But at least
>>> it could be possible to catch an incorrect type use easily, something
>>> we couldn't do in the macro:
>>>
>>> int get_opt_int(opt, sub)
>>> {
>>> if (opts[opt].subopt_params[sub].type != INT)
>>> print an error and exit();
>>> return opts[opt].subopt_params[sub].value.int;
>>> }
>>
>> Yes, but why do we need to add type checking like this? It seems to
>> me that you're trying to over-engineer a simple thing that does not
>> need to be complex. For options with integer/flag values:
>>
>> /* return default value if nothing specified on the CLI */
>> static inline uint64_t
>> get_config_val(int opt, int subopt)
>> {
>> if (!opts[opt].subopt_params[subopt].seen)
>> return opts[opt].subopt_params[subopt].defaultval;
>> return opts[opt].subopt_params[subopt].value;
>> }
>>
>> And for options that are strings (e.g. device names):
>>
>> static inline char *
>> get_config_str(int opt, int subopt)
>> {
>> if (!opts[opt].subopt_params[subopt].str_seen)
>> return NULL;
>> return opts[opt].subopt_params[subopt].string;
>> }
>>
>> That's all that is necessary. The config table is guaranteed to
>> contain valid default values, and by the time we get to checking
>> options we've done all the conflict/validity checking so we can
>> trust the config settings to be valid and just use them directly
>> like this.
>>
>
> Yes, but to be able to do this, we have to remove the other numeric
> types. As long we have int, uint, various sizes... then there isn't an
> overarching data type that can be used, so we either lose part of the
> value range, or we need multiple functions. If we have only the
> uint64, then this works.
>
>>> >> + __uint64_t *dswidth,
>>> >> + __uint64_t *lsunit)
>>> >
>>> > My, what big raid stripes you have! ;)
>>>
>>> Well, yes, 64 bits is not necessary here, but I would say that having
>>> just one size of uint removes some (even if small) ambiguity, while
>>> performance-wise, it won't do anything noticeable.
>>
>> However, it makes me cringe when I read code written like this. You
>> say it removes ambiguity but to me, after more than 20 years of C
>> coding using appropriate types for variable contents, using uint64_t
>> for all variables (especially those that don't require 64 bit types)
>> introduces ambiguity and raises questions about the code quality.
>>
>> e.g. declaring a flag as boolean means the author intended it to
>> only have two values (i.e. it's self documenting then intent of a
>> flag value!) whereas declaring them all as uint64_t makes me wonder
>> why the author of this code didn't know what the valid value range
>> for this variable is, why none of the reviewers cared either, what
>> happens if I put a really large value into the field instead of 0 or
>> 1, if that was tested, etc. Using the right type removes all this
>> potential ambiguity in the use/value of the variables.....
>
> The boolean flag is something I acknowledged in my previous email; it
> indeed makes sense to keep that as a boolean. With other types though,
> that goes against the idea of a single get_config_val().
Pinging this a bit. I changed all the flags to bool - these shouldn't
be an issue now. How about the other options like dswidth, though? Is
it ok if those are kept uint64?
And I'm not sure whether to use uint64_t or __uint64_t - as I wrote
before, I picked the __uint64_t because it was already used in this
file and is defined in xfs_linux.h, but I'm open to change it if it is
a legacy type and standard uint64_t is preferred...
Thanks.
Cheers,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 10+ messages in thread