* [xfsprogs] Do we need so many data types for user input?
@ 2017-04-04 8:54 Jan Tulak
2017-04-04 19:48 ` Luis R. Rodriguez
0 siblings, 1 reply; 9+ messages in thread
From: Jan Tulak @ 2017-04-04 8:54 UTC (permalink / raw)
To: linux-xfs
Hi guys,
as I'm still working on the mkfs changes, there is an opportunity to
clean up data types a bit. Right now, we are saving user input into
long long, bool, uint64, int, uint and string. Custom data types are
not saved into directly but such values are usually a result of a
computation.
I can't find any place (in the user-input section) where would we
really need anything else except uint64 and string. Some examples:
* We load an option into an int and then raise an error if it is a
negative number
* Or we even allow for 0, 1 (and 2) only
* It seems that the only place where we are not limiting the int
values immediately are logarithmic options. But unless my math
knowledge failed me, only complex logarithm is defined for negative
numbers and I really doubt we want to have "3+4.5 i" as blocksize...
;-)
Simply stated, signed values seem to be useless here. And at the end
of the day, we are converting all numbers into unsigned in one part of
the parsing anyway, so I don't see how such a change could change any
behaviour.
So what do you think, can I remove the other types in user-input part
of the code and save all numbers as uint64?
Cheers,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-04 8:54 [xfsprogs] Do we need so many data types for user input? Jan Tulak
@ 2017-04-04 19:48 ` Luis R. Rodriguez
2017-04-04 20:17 ` Eric Sandeen
2017-04-05 12:15 ` Dave Chinner
0 siblings, 2 replies; 9+ messages in thread
From: Luis R. Rodriguez @ 2017-04-04 19:48 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs
On Tue, Apr 04, 2017 at 10:54:09AM +0200, Jan Tulak wrote:
> Simply stated, signed values seem to be useless here. And at the end
> of the day, we are converting all numbers into unsigned in one part of
> the parsing anyway, so I don't see how such a change could change any
> behaviour.
>
> So what do you think, can I remove the other types in user-input part
> of the code and save all numbers as uint64?
This seems reasonable to me, I could not find any valid use for negative values,
yet we have int all over. No bueno.
That seems like a small pedantic correction we can make, but to be clear I think it
something we can fix not by changing struct subopt_param but rather just the
data types for the specific values, for instance:
int inodelog;
int isize;
int ilflag;
My point about struct subopt_param is that the usage of long long is there from
a practical point of view of the tradition to give us the ability to later
convert any value to any data type in between. So while I agree that negative
values are not used right now, and we should use a correct unsigned data type,
I think keeping long long on struct subopt_param makes sense.
>From what I can tell we only need generally for all input values 0 - UINT_MAX,
and if we were to use for instance uint64_t we would still generally cap to
UINT_MAX on mkfs.xfs for now and if we want later we bump to ULLONG_MAX without
much changes.
Perhaps the two exceptions worth reviewing in light of whether or not to generally
max out to UINT_MAX or ULONG_MAX right now seem to be possibly be:
__uint64_t agcount;
__uint64_t agsize;
agcount has a boundary limit though: XFS_MAX_AGNUMBER :
mkfs/xfs_mkfs.c: .maxval = XFS_MAX_AGNUMBER,
include/xfs_multidisk.h:#define XFS_MAX_AGNUMBER ((xfs_agnumber_t)(NULLAGNUMBER - 1))
libxfs/xfs_types.h:#define NULLAGNUMBER ((xfs_agnumber_t)-1)
typedef __uint32_t xfs_agnumber_t; /* allocation group number */
Sicne this is u32 UINT_MAX should suffice.
And then agsize is capped artificially later via validate_ag_geometry():
if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
...
if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
...
if (agsize > dblocks) {
...
if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
...
Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
If it is capped at UINT_MAX then at least from the parsed input values from
user on mkfs.xfs then it would seem all would have a max cap at UINT_MAX and
all would also have a general min at 0.
As for actually making the code changes though:
Although changing data types for all these input values is quite a bit of code
churn I think picking something unsigned would be good practice for us (I noted
why uint64_t would be good above). If we use these local variables however to
call out to *other* code in other files, the code churn will probably be too
much for now and perhaps we can leave this for later as an after step.
Something like coccinelle can help us change all that crap later.
If changing the types however would only have local impact I think its a worthy
change to consider now.
FWIW since I had made the mkfs.xfs.conf RFCs I determined what the local variables
of impact would be, they are (I added struct mkfs_xfs_opts to stuff them all on one
data structure):
struct mkfs_xfs_opts {
int blocklog;
int blflag;
int bsflag;
struct fsxattr fsx;
libxfs_init_t xi;
__uint64_t agcount;
__uint64_t agsize;
int daflag;
int ssflag;
int dasize;
char *dsize;
int dsunit;
int dswidth;
int dsu;
int dsw;
int nodsflag;
int sectorlog;
int slflag;
struct sb_feat_args sb_feat;
int inodelog;
int isize;
int ilflag;
int imaxpct;
int imflag;
int ipflag;
int isflag;
int inopblock;
xfs_agnumber_t logagno;
int laflag;
int liflag;
int lsuflag;
int lsu;
int loginternal;
char *logfile;
char *logsize;
int lsectorlog;
int lsectorsize;
int lsunit;
int lsunitflag;
int ldflag;
int lvflag;
int lslflag;
int lssflag;
uuid_t uuid;
int dirblocklog;
int dirblocksize;
int nlflag;
int nvflag; /* unused */
int nsflag;
char *rtextsize;
char *rtsize;
int norsflag;
};
Of course if we had other variables on main() which were set based on the above
ones then those would also be impacted.
Luis
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-04 19:48 ` Luis R. Rodriguez
@ 2017-04-04 20:17 ` Eric Sandeen
2017-04-05 8:26 ` Jan Tulak
2017-04-05 12:15 ` Dave Chinner
1 sibling, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2017-04-04 20:17 UTC (permalink / raw)
To: Luis R. Rodriguez, Jan Tulak; +Cc: linux-xfs
On 4/4/17 2:48 PM, Luis R. Rodriguez wrote:
> On Tue, Apr 04, 2017 at 10:54:09AM +0200, Jan Tulak wrote:
>> Simply stated, signed values seem to be useless here. And at the end
>> of the day, we are converting all numbers into unsigned in one part of
>> the parsing anyway, so I don't see how such a change could change any
>> behaviour.
>>
>> So what do you think, can I remove the other types in user-input part
>> of the code and save all numbers as uint64?
>
> This seems reasonable to me, I could not find any valid use for negative values,
> yet we have int all over. No bueno.
>
> That seems like a small pedantic correction we can make, but to be clear I think it
> something we can fix not by changing struct subopt_param but rather just the
> data types for the specific values, for instance:
>
> int inodelog;
> int isize;
> int ilflag;
>
> My point about struct subopt_param is that the usage of long long is there from
> a practical point of view of the tradition to give us the ability to later
> convert any value to any data type in between. So while I agree that negative
> values are not used right now, and we should use a correct unsigned data type,
> I think keeping long long on struct subopt_param makes sense.
>
> From what I can tell we only need generally for all input values 0 - UINT_MAX,
> and if we were to use for instance uint64_t we would still generally cap to
> UINT_MAX on mkfs.xfs for now and if we want later we bump to ULLONG_MAX without
> much changes.
>
> Perhaps the two exceptions worth reviewing in light of whether or not to generally
> max out to UINT_MAX or ULONG_MAX right now seem to be possibly be:
>
> __uint64_t agcount;
> __uint64_t agsize;
>
> agcount has a boundary limit though: XFS_MAX_AGNUMBER :
>
> mkfs/xfs_mkfs.c: .maxval = XFS_MAX_AGNUMBER,
> include/xfs_multidisk.h:#define XFS_MAX_AGNUMBER ((xfs_agnumber_t)(NULLAGNUMBER - 1))
> libxfs/xfs_types.h:#define NULLAGNUMBER ((xfs_agnumber_t)-1)
> typedef __uint32_t xfs_agnumber_t; /* allocation group number */
>
> Sicne this is u32 UINT_MAX should suffice.
yep.
>
> And then agsize is capped artificially later via validate_ag_geometry():
>
> if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
> ...
> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
> ...
> if (agsize > dblocks) {
> ...
> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
> ...
>
> Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
.maxval = XFS_AG_MAX_BYTES,
#define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
#define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
so the maximum is (long long)(512) << 31
so, no - agsize won't fit in a 32 bit var, if that's the question...
-Eric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-04 20:17 ` Eric Sandeen
@ 2017-04-05 8:26 ` Jan Tulak
2017-04-05 14:08 ` Eric Sandeen
0 siblings, 1 reply; 9+ messages in thread
From: Jan Tulak @ 2017-04-05 8:26 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs
On Tue, Apr 4, 2017 at 10:17 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 4/4/17 2:48 PM, Luis R. Rodriguez wrote:
>>
>> And then agsize is capped artificially later via validate_ag_geometry():
>>
>> if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>> ...
>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>> ...
>> if (agsize > dblocks) {
>> ...
>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>> ...
>>
>> Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
>
> .maxval = XFS_AG_MAX_BYTES,
>
> #define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
> #define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
>
> so the maximum is (long long)(512) << 31
>
> so, no - agsize won't fit in a 32 bit var, if that's the question...
>
Yeah. I think that we can make it uint64 everywhere unless it is
causing an issue in a specific case with a bit shift or something, and
just limit the input value where appropriate explicitly.
And...
> And at the end of the day, we are converting all numbers into unsigned
I take back this part, we are going through long long, not unsigned long long.
Everything else is still valid.
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-04 19:48 ` Luis R. Rodriguez
2017-04-04 20:17 ` Eric Sandeen
@ 2017-04-05 12:15 ` Dave Chinner
1 sibling, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2017-04-05 12:15 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: Jan Tulak, linux-xfs
On Tue, Apr 04, 2017 at 09:48:25PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 04, 2017 at 10:54:09AM +0200, Jan Tulak wrote:
> > Simply stated, signed values seem to be useless here. And at the end
> > of the day, we are converting all numbers into unsigned in one part of
> > the parsing anyway, so I don't see how such a change could change any
> > behaviour.
> >
> > So what do you think, can I remove the other types in user-input part
> > of the code and save all numbers as uint64?
>
> This seems reasonable to me, I could not find any valid use for negative values,
> yet we have int all over. No bueno.
Keep in mind you're looking at coding pattersn that were laid down
more than 20 years ago by people with different coding standards and
habits. e.g. lots of the values were originally ints because they
were set by calls to atoi() in command line parsing. the atoi()
calls are long gone, but the variables are still ints...
....
> FWIW since I had made the mkfs.xfs.conf RFCs I determined what the local variables
> of impact would be, they are (I added struct mkfs_xfs_opts to stuff them all on one
> data structure):
>
> struct mkfs_xfs_opts {
> int blocklog;
> int blflag;
> int bsflag;
[....]
My intent when starting the table based config mods was that all
these intermediate values and flags would go away. i.e. they'd be
replace with a config table lookup for the option value because they
are redundant values and using the config table would remove the
confusion on what values should be used in the code.
And once everything is in the config option table, main() can be
more easily factored in sane, maintainable chunks....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-05 8:26 ` Jan Tulak
@ 2017-04-05 14:08 ` Eric Sandeen
2017-04-05 14:24 ` Jan Tulak
0 siblings, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2017-04-05 14:08 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On 4/5/17 3:26 AM, Jan Tulak wrote:
> On Tue, Apr 4, 2017 at 10:17 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> On 4/4/17 2:48 PM, Luis R. Rodriguez wrote:
>>>
>>> And then agsize is capped artificially later via validate_ag_geometry():
>>>
>>> if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>>> ...
>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>>> ...
>>> if (agsize > dblocks) {
>>> ...
>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>>> ...
>>>
>>> Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
>>
>> .maxval = XFS_AG_MAX_BYTES,
>>
>> #define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
>> #define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
>>
>> so the maximum is (long long)(512) << 31
>>
>> so, no - agsize won't fit in a 32 bit var, if that's the question...
>>
>
> Yeah. I think that we can make it uint64 everywhere unless it is
> causing an issue in a specific case with a bit shift or something, and
> just limit the input value where appropriate explicitly.
>
>
> And...
>> And at the end of the day, we are converting all numbers into unsigned
>
> I take back this part, we are going through long long, not unsigned long long.
> Everything else is still valid.
Ok, for those who aren't immersed 24/7 in mkfs ;) can you help us understand
where you're going with this? What's the change you're proposing, and how
does it help?
Thanks,
-Eric
> Jan
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-05 14:08 ` Eric Sandeen
@ 2017-04-05 14:24 ` Jan Tulak
2017-04-06 12:40 ` Luis R. Rodriguez
0 siblings, 1 reply; 9+ messages in thread
From: Jan Tulak @ 2017-04-05 14:24 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Luis R. Rodriguez, linux-xfs
On Wed, Apr 5, 2017 at 4:08 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>
> On 4/5/17 3:26 AM, Jan Tulak wrote:
>> On Tue, Apr 4, 2017 at 10:17 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>>> On 4/4/17 2:48 PM, Luis R. Rodriguez wrote:
>>>>
>>>> And then agsize is capped artificially later via validate_ag_geometry():
>>>>
>>>> if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>>>> ...
>>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>>>> ...
>>>> if (agsize > dblocks) {
>>>> ...
>>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>>>> ...
>>>>
>>>> Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
>>>
>>> .maxval = XFS_AG_MAX_BYTES,
>>>
>>> #define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
>>> #define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
>>>
>>> so the maximum is (long long)(512) << 31
>>>
>>> so, no - agsize won't fit in a 32 bit var, if that's the question...
>>>
>>
>> Yeah. I think that we can make it uint64 everywhere unless it is
>> causing an issue in a specific case with a bit shift or something, and
>> just limit the input value where appropriate explicitly.
>>
>>
>> And...
>>> And at the end of the day, we are converting all numbers into unsigned
>>
>> I take back this part, we are going through long long, not unsigned long long.
>> Everything else is still valid.
>
> Ok, for those who aren't immersed 24/7 in mkfs ;) can you help us understand
> where you're going with this? What's the change you're proposing, and how
> does it help?
>
If you ask only about the last sentence - I was just taking back my
claim that we already convert all numbers into unsigned in getnum. We
convert them to long long right now. (It was too late evening for me,
apparently. :-) )
If you ask more about summarising what I want to do, then:
I'm already moving these standalone variables into an options table,
that holds all the information in one place. As I'm already touching
them, I can as well clean the differences and changes accumulated over
the years and change a lot of mkfs's internal variables from a mix of
signed and unsigned, 32bit and 64bit, to a single type, unsigned
64bits long.
That would simplify the code in some places and make it clear what
type numerical values are - less things like foo((long long) bar);
Cheers,
Jan
> Thanks,
> -Eric
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-05 14:24 ` Jan Tulak
@ 2017-04-06 12:40 ` Luis R. Rodriguez
2017-04-06 13:03 ` Jan Tulak
0 siblings, 1 reply; 9+ messages in thread
From: Luis R. Rodriguez @ 2017-04-06 12:40 UTC (permalink / raw)
To: Jan Tulak; +Cc: Eric Sandeen, Luis R. Rodriguez, linux-xfs
On Wed, Apr 05, 2017 at 04:24:18PM +0200, Jan Tulak wrote:
> On Wed, Apr 5, 2017 at 4:08 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> >
> > On 4/5/17 3:26 AM, Jan Tulak wrote:
> >> On Tue, Apr 4, 2017 at 10:17 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> >>> On 4/4/17 2:48 PM, Luis R. Rodriguez wrote:
> >>>>
> >>>> And then agsize is capped artificially later via validate_ag_geometry():
> >>>>
> >>>> if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
> >>>> ...
> >>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
> >>>> ...
> >>>> if (agsize > dblocks) {
> >>>> ...
> >>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
> >>>> ...
> >>>>
> >>>> Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
> >>>
> >>> .maxval = XFS_AG_MAX_BYTES,
> >>>
> >>> #define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
> >>> #define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
> >>>
> >>> so the maximum is (long long)(512) << 31
> >>>
> >>> so, no - agsize won't fit in a 32 bit var, if that's the question...
> >>>
> >>
> >> Yeah. I think that we can make it uint64 everywhere unless it is
> >> causing an issue in a specific case with a bit shift or something, and
> >> just limit the input value where appropriate explicitly.
> >>
> >>
> >> And...
> >>> And at the end of the day, we are converting all numbers into unsigned
> >>
> >> I take back this part, we are going through long long, not unsigned long long.
> >> Everything else is still valid.
> >
> > Ok, for those who aren't immersed 24/7 in mkfs ;) can you help us understand
> > where you're going with this? What's the change you're proposing, and how
> > does it help?
> >
>
> If you ask only about the last sentence - I was just taking back my
> claim that we already convert all numbers into unsigned in getnum. We
> convert them to long long right now.
Yes but this is just general best practice -- typically strtoll() is used
and then folks cast as needed, this is done to avoid loosing information.
The casting is also where a lot of potential bugs can lurk.
getnum() returns long long, we currently cast this mostly to int.
> (It was too late evening for me,
> apparently. :-) )
>
> If you ask more about summarising what I want to do, then:
>
> I'm already moving these standalone variables into an options table,
> that holds all the information in one place. As I'm already touching
> them, I can as well clean the differences and changes accumulated over
> the years and change a lot of mkfs's internal variables from a mix of
> signed and unsigned, 32bit and 64bit, to a single type, unsigned
> 64bits long.
>
> That would simplify the code in some places and make it clear what
> type numerical values are - less things like foo((long long) bar);
Yes please ! Modulo -- as I noted I think in practice this may be:
a) self contained work -- if the changes from say int to another unsigned
type on xfs_mkfs.c only affect this file.
b) a lot changes: if the type changes require much more changes to API calls
outside of this xfs_mkfs.c file, say when the file calls to helpers.
I think a) seems reasonable to do now. b) I think this should be welcomed but
it means much more patches to your series, so if it turns out to be a lot
perhaps best left as a secondary effort for later. I don't think it clear
where this falls yet.
Luis
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [xfsprogs] Do we need so many data types for user input?
2017-04-06 12:40 ` Luis R. Rodriguez
@ 2017-04-06 13:03 ` Jan Tulak
0 siblings, 0 replies; 9+ messages in thread
From: Jan Tulak @ 2017-04-06 13:03 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: Eric Sandeen, linux-xfs
On Thu, Apr 6, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Wed, Apr 05, 2017 at 04:24:18PM +0200, Jan Tulak wrote:
>> On Wed, Apr 5, 2017 at 4:08 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> >
>> > On 4/5/17 3:26 AM, Jan Tulak wrote:
>> >> On Tue, Apr 4, 2017 at 10:17 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> >>> On 4/4/17 2:48 PM, Luis R. Rodriguez wrote:
>> >>>>
>> >>>> And then agsize is capped artificially later via validate_ag_geometry():
>> >>>>
>> >>>> if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>> >>>> ...
>> >>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>> >>>> ...
>> >>>> if (agsize > dblocks) {
>> >>>> ...
>> >>>> if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>> >>>> ...
>> >>>>
>> >>>> Its not clear if this is capped at UINT_MAX or if we need ULLONG_MAX here.
>> >>>
>> >>> .maxval = XFS_AG_MAX_BYTES,
>> >>>
>> >>> #define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
>> >>> #define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
>> >>>
>> >>> so the maximum is (long long)(512) << 31
>> >>>
>> >>> so, no - agsize won't fit in a 32 bit var, if that's the question...
>> >>>
>> >>
>> >> Yeah. I think that we can make it uint64 everywhere unless it is
>> >> causing an issue in a specific case with a bit shift or something, and
>> >> just limit the input value where appropriate explicitly.
>> >>
>> >>
>> >> And...
>> >>> And at the end of the day, we are converting all numbers into unsigned
>> >>
>> >> I take back this part, we are going through long long, not unsigned long long.
>> >> Everything else is still valid.
>> >
>> > Ok, for those who aren't immersed 24/7 in mkfs ;) can you help us understand
>> > where you're going with this? What's the change you're proposing, and how
>> > does it help?
>> >
>>
>> If you ask only about the last sentence - I was just taking back my
>> claim that we already convert all numbers into unsigned in getnum. We
>> convert them to long long right now.
>
> Yes but this is just general best practice -- typically strtoll() is used
> and then folks cast as needed, this is done to avoid loosing information.
> The casting is also where a lot of potential bugs can lurk.
>
> getnum() returns long long, we currently cast this mostly to int.
>
>> (It was too late evening for me,
>> apparently. :-) )
>>
>> If you ask more about summarising what I want to do, then:
>>
>> I'm already moving these standalone variables into an options table,
>> that holds all the information in one place. As I'm already touching
>> them, I can as well clean the differences and changes accumulated over
>> the years and change a lot of mkfs's internal variables from a mix of
>> signed and unsigned, 32bit and 64bit, to a single type, unsigned
>> 64bits long.
>>
>> That would simplify the code in some places and make it clear what
>> type numerical values are - less things like foo((long long) bar);
>
> Yes please ! Modulo -- as I noted I think in practice this may be:
>
> a) self contained work -- if the changes from say int to another unsigned
> type on xfs_mkfs.c only affect this file.
>
> b) a lot changes: if the type changes require much more changes to API calls
> outside of this xfs_mkfs.c file, say when the file calls to helpers.
>
> I think a) seems reasonable to do now. b) I think this should be welcomed but
> it means much more patches to your series, so if it turns out to be a lot
> perhaps best left as a secondary effort for later. I don't think it clear
> where this falls yet.
>
> Luis
I'm working on it right now and it *seems* that turning the important
variables from main (the ones declared on the top, at least) into
uint64 is rather straightforward and simple change.
I will make it a standalone patchset (change the variables + minimum
of other changes to make it work, then another patch to ditch some
type casting). I think I can send the patches today if it goes well.
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.
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-04-06 13:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 8:54 [xfsprogs] Do we need so many data types for user input? Jan Tulak
2017-04-04 19:48 ` Luis R. Rodriguez
2017-04-04 20:17 ` Eric Sandeen
2017-04-05 8:26 ` Jan Tulak
2017-04-05 14:08 ` Eric Sandeen
2017-04-05 14:24 ` Jan Tulak
2017-04-06 12:40 ` Luis R. Rodriguez
2017-04-06 13:03 ` Jan Tulak
2017-04-05 12:15 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.