Attention is currently required from: Thomas Heijligen, Angel Pons.
14 comments:
Patchset:
Ooof, sorry lots of comments /o\
File hwaccess_x86_io.c:
Patch Set #3, Line 22: therefor
AFAIK, this term is used rather specific, in legal documents and such. Maybe
just start the sentence with "For this purpose, ...".
Patch Set #3, Line 23: those
No platforms were named to refer to, AFAICT. So I would rather say
"on the respective platform".
Patch Set #3, Line 23: static int platform_get_io_perms(void)
No need to mention the whole function signature (you too actually didn't
on the next line), I would just say platform_get_io_perms() and
platform_release_io_perms().
please separate multiple platforms with a comma
Patch Set #3, Line 49: return sysi86(SI86V86, V86SC_IOPL, PS_IOPL);
This needs <sys/sysi86.h> (our header file already includes it
but we shouldn't rely on that).
Patch Set #3, Line 65: return (io_fd = open("/dev/io", O_RDWR) >= 0 ? 0 : 1);
Please do the assignment on a separate line :)
Other functions, e.g. iopl(), ioperm(), return -1 on error.
Patch Set #3, Line 92: return iopl(3);
<sys/io.h>
Patch Set #3, Line 114: if (ret == 0)
Please add braces on all paths if one path needs them. Or `return 0`
on the positive path and drop the `else`. Would save indentation below.
Patch Set #3, Line 115: register_shutdown(platform_release_io_perms, NULL);
Could be a separate patch as this wasn't handled before:
```
if (register_shutdown(...)) {
platform_release_io_perms(...);
return 1;
}
return 0;
```
In theory, this could be a return parameter of platform_get_io_perms().
Then we wouldn't need the global `io_fd`, for instance.
Patch Set #3, Line 121: msg_perr("On NetBSD please reboot into single user mode or make sure\n"
Can we make the output changes a separate patch?
Patch Set #3, Line 124: return ret;
Better use an explicit `return 0`/`return 1` as before. Currently this
could also return a -1 from iopl()/ioperm().
To view, visit change 61275. To unsubscribe, or for help writing mail filters, visit settings.