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

Add support for s390[x]. #4425

Closed
wants to merge 3 commits into from
Closed

Add support for s390[x]. #4425

wants to merge 3 commits into from

Conversation

xnox
Copy link
Contributor

@xnox xnox commented Mar 16, 2016

Signed-off-by: Dimitri John Ledkov [email protected]

@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

Requires-spl: refs/pull/537/head

@behlendorf
Copy link
Contributor

@xnox sorry, I wasn't 100% clear. The Requires-spl comment is expected to be part of the git commit comment and not a github.com comment.

@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

let's try again.

@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

My style is wrong somehow? i thought i did do copy & pastes....

make checkstyle
 in dir /var/lib/buildbot/slaves/zfs/Amazon_2015_09_x86_64__BUILD_/build/zfs (timeout 1200 secs)
 watching logfiles {}
 argv: ['make', 'checkstyle']
 using PTY: False
./lib/libspl/include/sys/isa_defs.h: 161: extra space between function name and left paren
./lib/libspl/include/sys/isa_defs.h: 164: #define followed by space instead of tab
./lib/libspl/include/sys/isa_defs.h: 168: #define followed by space instead of tab
./lib/libspl/include/sys/isa_defs.h: 172: #define followed by space instead of tab
make: *** [cstyle] Error 1
program finished with exit code 2
elapsedTime=5.551528

@behlendorf
Copy link
Contributor

@xnox we follow the SunOS style guide for historical reasons and it's pretty pedantic about how it likes its whitespace. You can locally run the style checker with make cstyle before updating the patch. Specifically it really wants to have a tab after a #define instead of a space.

#define<tab>XYZ<tab>ABC

Requires-spl: refs/pull/537/head
Signed-off-by: Dimitri John Ledkov <[email protected]>
@ryao
Copy link
Contributor

ryao commented Mar 16, 2016

@xnox You can use ./scripts/cstyle.pl ./lib/libspl/include/sys/isa_defs.h to run the style check locally.

@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

@ryao

scripts/cstyle.pl ./lib/libspl/include/sys/isa_defs.h
Unescaped left brace in regex is deprecated, passed through in regex; marked by <-- HERE in m/\S{ <-- HERE / at scripts/cstyle.pl line 608.
Unescaped left brace in regex is deprecated, passed through in regex; marked by <-- HERE in m/{{ <-- HERE / at scripts/cstyle.pl line 608.

i think it's good, albeit "old" perl.

@ryao
Copy link
Contributor

ryao commented Mar 16, 2016

@xnox Have you run any regression tests on this to validate that everything is working on the z/architecture? For example, the ZoL fork of the XFS tests:

https://github.com/zfsonlinux/xfstests/tree/zfs-upstream

That being said, this looks good to me.

@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

@ryao even if nothing works, this is not a regression, but a compile fix. Cause clearly nobody even tried to build it yet. I was mostly interested in just spl to be honest. If this gets zfs module to build, it is a pleasant side-effect. Maybe somebody else will try to test it, et.al.

@behlendorf
Copy link
Contributor

@xnox indeed, you're the first person to come around these parts looking for a s390[x] build. If it compiles that's at least progress. If you have access to hardware to test on it would be interesting to run the ZFS Test Suite, see the README in the tests directory for instructions.

@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

I am having a suspecion that isa_defs.h header should also have (a) check that either/or _SUNOS_VTOC_[8|16] are defined (b) define _SUNOS_VTOC_[8|16] for s390/s390x. What should it be? _8 for s390, and _16 for s390x?

Signed-off-by: Dimitri John Ledkov <[email protected]>
@xnox
Copy link
Contributor Author

xnox commented Mar 16, 2016

i think _SUNOS_VTOC_16 is correct here for this repo, but shouldn't be defined in the spl repo.

@xnox
Copy link
Contributor Author

xnox commented Mar 17, 2016

can someone explain the failures above to me? I somehow doubt I have caused them....

@behlendorf
Copy link
Contributor

@xnox SUNOS_VTOC_16 is what you want. You're also going to need to add __s390__ and __s390x__ to this block of code. It looks to me like you've added one but not the other.

We're still working on updating some of the test cases for Linux so the test failure was definitely not your fault.

@xnox
Copy link
Contributor Author

xnox commented Mar 17, 2016

@behlendorf only one define is needed __s390__ is defined on both s390 and s390x. the _16 vtoc is added too, so i should be all good then. Please merge.

@behlendorf
Copy link
Contributor

@xbox OK, then that makes sense. This LGTM I'll get it and the SPL patch merged.

@behlendorf
Copy link
Contributor

@xnox thanks. Squashed and merged as:

a1bc34c Add support for s390[x].

nedbass pushed a commit that referenced this pull request Mar 23, 2016
Signed-off-by: Dimitri John Ledkov <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #4425
@behlendorf behlendorf added this to the 0.6.5.6 milestone Mar 23, 2016
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.

3 participants