#1272 PR merged: Patch for issue #1269 where mawk incorrectly calculated size of partition

Labels: enhancement, fixed / solved / done, minor bug

gozora opened issue at 2017-03-29 16:46:

This patch corrects problems with mawk, which allowed maximum integer 0x7FFFFFFF (2147483647) when "%d" conversion specification format was used.
This caused that maximum allowed partition size was limited to ~2GB.

jsmeix commented at 2017-03-30 08:23:

I wonder what "round starting size to next multiple of 4096"
actually means, in particular what the 'next' therein means.

Does it mean that e.g. 123 * 4096 should be rounded to 123 * 4096
or does the 'next' mean 123 * 4096 should be rounded to 124 * 4096

In the latter case the current calculation is actually right.

jsmeix commented at 2017-03-30 08:25:

@gozora
when we agreed on what that calculation actually means,
an additional explanatory comment in the code
could help to avoid confusion for future conributors ;-)

jsmeix commented at 2017-03-30 08:37:

@gozora
just an informal comment regarding coding style:
In bash arithmetic evaluation (( ... )) variables do not
need a leading '$' according to "man bash"

Within an expression, shell variables may also
be referenced by name without using the parameter
expansion syntax.  A shell variable that is null
or unset evaluates to 0 when referenced by name
without using the  parameter expansion syntax.

Therefore I wonder what is better coding style:

result=$(( ( $foo - $bar ) / $baz ))

or

result=$(( ( foo - bar ) / baz ))

jsmeix commented at 2017-03-30 13:57:

Furthermore I wonder why in 100_include_partition_code.sh
a variable block_size is calculated and used for stuff for
logical partition and in the next block there is a guess that
"4096 is a good match for most device's block size".

It seems I completely fail to understand the code
in 100_include_partition_code.sh :-(

Accordingly I dismiss my review because I think
a review from one who fails to understand the code
does not really help you.

I would very much appreciate it if @gozora or @gdha
could explain 100_include_partition_code.sh to me.

gozora commented at 2017-03-30 14:40:

@jsmeix, @gdha as I'm really bad in working on two things at once, I'd put this pull request on hold until I've finished XFS testing topic then I'll check 100_include_partition_code.sh more deeply.

Hope you don't mind.

V.

gozora commented at 2017-03-30 14:44:

Btw. original code seems to be created by @jhoekx so maybe he can bring bit of light into this?
(If I'm wrong just tell me and I'll read my Git book bought on Fosdem one more time :-) ).

gozora commented at 2017-03-30 14:50:

@jsmeix

just an informal comment regarding coding style:
In bash arithmetic evaluation (( ... )) variables do not
need a leading '$' according to "man bash"

Great so I'm not crazy after all! I've noticed syntax result=$(( ( foo - bar ) / baz )) but I ignored that :-).

If you ask me, I like result=$(( ( $foo - $bar ) / $baz )) more because $ somehow evokes variable for me ..

V.

jsmeix commented at 2017-03-30 15:18:

@gdha
I think we should for now simply merge this pull request
because it fixes the mawk issue without changing the
result of the calculations so that it cannot make things worse.

Later - as time permits - I may have a closer look
at the code in 100_include_partition_code.sh and
perhaps then I might be able to improve it.

[Export of Github issue for rear/rear.]