Ticket #3747 (closed defect: fixed)

Opened 4 months ago

Last modified 4 months ago

extfs: hp48: fix float truncation

Reported by: mooffie Owned by: zaytsev
Priority: major Milestone: 4.8.19
Component: mc-vfs Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

[This is forked from #3729.]

The hp48 helper uses the following code to convert floats (of which filesizes are) to integers:

hp48_retsize()
{
    printf "%d" "$2" 2>/dev/null
}
# Note: The number is in $2, not in $1.

(This results in truncation, not rounding.)

While this trick is acceptable according to the a standard ("If an argument operand cannot be completely converted [...] the utility shall [...] write the value accumulated at the time the error was detected to standard output"), it seems that some shells don't support it. @Andrew gives as example a certain version of Bash.

So we'd better change that code. One possibility:

hp48_retsize()
{
    echo ${2%.*}
}

(We shouldn't do "printf %.0f NUMBER" as this would round the number, not truncate it. Not that rounding is a sin, but we should better stick to the current behavior.)

Change History

comment:1 Changed 4 months ago by mooffie

(We shouldn't do "printf %.0f NUMBER" as this would round the number, not truncate it. Not that rounding is a sin, but we should better stick to the current behavior.)


(While I haven't seen anything other than ".5" used as the fraction in the two samples I found on the net, these samples were very short and I don't know the meaning of this fraction.)

I'm attaching a patch.

Changed 4 months ago by mooffie

comment:2 follow-up: ↓ 4 Changed 4 months ago by zaytsev

To be honest, I feel bad about the replacement, because it will obviously not handle all kinds of badness like .2, 1.2e3 and such. Of course, this doesn't make too much sense, but you said it yourself that we don't have a lot of valid inputs to test against.

How about using awk instead? It is used in the script anyways, and has a handy int() function. I'm not an awk expert, but I would imagine that something like awk '{ print int($0) }' would do the trick.

Last edited 4 months ago by zaytsev (previous) (diff)

comment:3 Changed 4 months ago by zaytsev

  • Owner set to zaytsev
  • Status changed from new to accepted
  • Milestone changed from Future Releases to 4.8.19

comment:4 in reply to: ↑ 2 Changed 4 months ago by mooffie

Replying to zaytsev:

To be honest, I feel bad about the replacement, because it will obviously not handle all kinds of badness like .2, 1.2e3 [...] something like awk '{ print int($0) }' would do the trick.


I tried your suggestion, but I couldn't bring myself to upload the patch (I had two versions of it). Here's why:

The current code is ridiculous as it is. Making hp48_retsize() use awk will make it worthy of appearing in the "Software" section of Wikipedia's "Farce" article. I'm not trying to be "clever" or "witty": I've been struggling with uploading that patch for the last 24 hours.

So I see two possibilities (beside someone else uploading that patch):

  • Use my previous patch (which matches the original behavior, btw).
  • Use the following patch, which makes the inside of the loop sane. I intentionally left the silly $INPUT and "EOF" things in (instead of simply doing "while read obj_name obj_size obj_type") because I felt changing this wouldn't be appreciated considering the circumstances (lack of exhaustive tests). I know it's not what you were expecting. To me this looks like the obvious right choice, to get rid of silly code as we go along, but it's fine with me if you reject it.

comment:5 Changed 4 months ago by mooffie

And next is a follow up patch to make the code more readable (only if you choose the "--ALTERNATIVE" patch.)

(BTW, I tested the helper with a "Directory" line to make sure the corresponding printf still works.)

comment:6 Changed 4 months ago by zaytsev

On an unrelated note: could you please enable automatic sign-off for your patches?

git config [--global] format.signoff true

I've just realized that I've recently committed some of them without amending with my sign-off (so, in the end, they haven't got any), and now I feel bad about it. It's not the end of the world, but still, if we are doing it at all, we should be doing this consistently. Thanks!

comment:7 Changed 4 months ago by zaytsev

  • Branch state changed from no branch to on review

The current code is ridiculous as it is.

I agree that this old code is a joke and yes, I'd still be wary of making changes to it due to the level of test coverage, but whatever, first, there is a basic test now, so outrageous brokenness will hopefully fail it, and then it didn't even work before you fixed it, so...

Branch: 3747_hp48_extfs
Initial changeset: 6ecb6d12e120b692c93d39ff610cb26728da8867

comment:8 Changed 4 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:9 Changed 4 months ago by zaytsev

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:10 Changed 4 months ago by zaytsev

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.