Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reservation test cases for large disks #5715

Merged
merged 1 commit into from
Feb 2, 2017

Conversation

behlendorf
Copy link
Contributor

@behlendorf behlendorf commented Jan 31, 2017

Convert explicit typeset -i declarations to typeset -l in order
to prevent 32-bit overflow from occurs with disks >2G.

@mention-bot
Copy link

@behlendorf, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tuxoko, @liaoyuxiangqin and @ChaoyuZhang to be potential reviewers.

@behlendorf
Copy link
Contributor Author

@gmelikov increasing the default volume size to 4G causes these test cases to fail due to 32-bit arithmatic limitations of ksh. This patch disables these test case until they can be updated. @jwk404 is this an issue for OpenZFS too? If so how are you handling it?

@jwk404
Copy link
Contributor

jwk404 commented Feb 1, 2017

At first glance, I'd say this is a problem I introduced by using typeset -i here. The shell isn't limited to 32 bit math if you use typeset -l foo or even just typeset foo by itself. We're not seeing this in OpenZFS because these tests (only these) use bash instead of ksh. When the tests first integrated, I'd switched to bash but was asked in review to keep using ksh. Somehow this set of tests must have been overlooked.

@behlendorf behlendorf changed the title Refresh Linux test suite runfile Fix reservation test cases for large disks Feb 1, 2017
@behlendorf
Copy link
Contributor Author

@jwk404 spot on! Thanks for the pointer to typeset -l. I've updated the patch to make the naive fix here and converted the typeset -i instances which could overflow to typeset -l. This resolved the issue.

I didn't notice you were using bash for theses scripts either. Do you intend to switch them back to ksh? If so what's your preferred fix for this?

@jwk404
Copy link
Contributor

jwk404 commented Feb 1, 2017

I'll convert our tests to use ksh, and at the same time, pull in your fix and we'll be back in sync again.

@tuxoko
Copy link
Contributor

tuxoko commented Feb 1, 2017

But why typeset -l?

$ help typeset
-l	to convert NAMEs to lower case on assignment

Why not just remove the typeset?

@gmelikov
Copy link
Member

gmelikov commented Feb 1, 2017

@tuxoko i've read that typeset makes variables local too.

@behlendorf thanks, i'll rebase #5679 just after this PR will be merged.

@jwk404
Copy link
Contributor

jwk404 commented Feb 1, 2017

@tuxoko @gmelikov The -l suggestion was a misreading of the manpage on my part. The typeset should be kept though, to keep the variables local.

@behlendorf
Copy link
Contributor Author

Right, so it looks like adding -l worked for the wrong reasons. I've update the patch to drop -l and fixed a few other places which -l was used incorrectly.

Convert explicit `typeset -i` and `typeset -l` declarations to
`typeset` in order to prevent 32-bit overflow from occurs with
disks >2G.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#5714

TEST_ZFSTESTS_DISKSIZE=4G
@behlendorf behlendorf merged commit 8eecd4a into openzfs:master Feb 2, 2017
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
Convert explicit `typeset -i` and `typeset -l` declarations to
`typeset` in order to prevent 32-bit overflow from occurs with
disks >2G.

TEST_ZFSTESTS_DISKSIZE=4G

Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#5715 
Closes openzfs#5714
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
Convert explicit `typeset -i` and `typeset -l` declarations to
`typeset` in order to prevent 32-bit overflow from occurs with
disks >2G.

TEST_ZFSTESTS_DISKSIZE=4G

Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#5715
Closes openzfs#5714
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants