Skip to content

Added TIOCSWINSZ to struct_info and added corner case stub #6468

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

Merged
merged 5 commits into from
Apr 30, 2018

Conversation

DataKinds
Copy link
Contributor

@DataKinds DataKinds commented Apr 21, 2018

This fixes issue #6463 by adding the incorrectly missing TIOCSWINSZ (which is defined in system/lib/libc/musl/arch/emscripten/bits/ioctl.h) to the struct_info.json file and handling it in the ioctl() syscall.

@kripken
Copy link
Member

kripken commented Apr 23, 2018

Thanks! Looks good. Please just add yourself to AUTHORS, and add a test (best thing is to grep in tests/ for similar things, like another ioctl call very much like that one, and add a line in there to test this, and confirm it fails before your fix).

@DataKinds
Copy link
Contributor Author

Done & tests passed.

ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws);
ioctl(STDOUT_FILENO, TIOCSWINSZ, &ws);
return 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! For the test, you also need to add code to the test suite so it runs, with something like this in tests/test_other.py:

  def test_ioctl_window_size(self):
    self.do_other_test(os.path.join('other', 'ioctl_window_size'))

That expects the source file to be in tests/other/ and end with suffix cpp, and there should be .out file alongside it with the expected output (which here could be that you add a printf after doing those ioctl calls, to say everything went ok - might be worth printing the return codes from ioctl). See e.g. c8d9b35 for a recently added test along those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, done, and done. Sorry about all the back & forth! I'm new to contributing to OSS.

@@ -8229,3 +8229,9 @@ def test_autotools_shared_check(self):
except OSError:
# Ignore missing python aliases.
pass

def test_ioctl_window_size(self):
run_process([PYTHON, EMCC, path_from_root('tests', 'ioctl', 'ioctl_window_size.c')])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 4 lines can be replaced with

    self.do_other_test(os.path.join('other', 'ioctl'))

, but you'll need to rename the c file to test.cpp and the output file to test.out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, would be better to do self.do_other_test(os.path.join('other', 'ioctl', 'window_size')) (add a subdir window_size).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it. My local copy didn't have self.do_other_test so I pulled it from upstream -- you should be able to remove any changes this made while merging.

@kripken
Copy link
Member

kripken commented Apr 29, 2018

Looks like you need to merge upstream incoming again, as there are conflicts.

@DataKinds
Copy link
Contributor Author

@kripken Merged upstream.

@kripken
Copy link
Member

kripken commented Apr 30, 2018

Thanks, perfect!

@kripken kripken merged commit 893a8d8 into emscripten-core:incoming Apr 30, 2018
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.

2 participants