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

Solaris 10 Fixes #1248

Merged
merged 6 commits into from
Mar 17, 2018
Merged

Solaris 10 Fixes #1248

merged 6 commits into from
Mar 17, 2018

Conversation

gsauthof
Copy link
Contributor

Fixes #1193 and #1194, a double free, some undefined behavior and some warnings.

Copy link
Owner

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

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

Nice work. Could you please update HISTORY file describing the fixes/improvements which occurred in this PR?

# Note #1: getpriority(3) doesn't work for realtime processes.
# Psinfo is what ps uses, see:
# https://github.com/giampaolo/psutil/issues/1194
return self._proc_basic_info()[proc_info_map['nice']]
Copy link
Owner

Choose a reason for hiding this comment

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

Nice that we got rid of that mess.

@@ -40,7 +40,7 @@ PyUnicode_DecodeFSDefaultAndSize(char *s, Py_ssize_t size) {
* If msg != "" the exception message will change in accordance.
*/
PyObject *
NoSuchProcess(char *msg) {
NoSuchProcess(const char *msg) {
Copy link
Owner

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const char * means pointer to const characters. The const modifier signals the caller that NoSuchProcess will not change the passed message. Otherwise, it's not clear and one has to inspect the function body. In case someone accidentally writes to msg using that const pointer it's a compile error. This change also remove compiler warnings since NoSuchProcess is called with string literals that effectively have the type const char *. Thus, this additional type safety protects against accidental writing into a const character array through a non-const pointer which would invoke undefined behavior (e.g. crash if the string literals are placed in a read-only data section).

@@ -4,6 +4,9 @@
* found in the LICENSE file.
*/

#ifndef PSUTIL_PSUTIL_COMMON_H
#define PSUTIL_PSUTIL_COMMON_H
Copy link
Owner

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This guards the header from being included multiple times into one translation unit (either directly/indirectly). In C, unless the header does something special, you always define such guards (or use the non-standardized once pragma).

@@ -394,7 +398,10 @@ def _proc_basic_info(self):

@memoize_when_activated
def _proc_cred(self):
return cext.proc_cred(self.pid, self._procfs_path)
@wrap_exceptions
Copy link
Owner

Choose a reason for hiding this comment

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

Why not decorate the method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried stacking the @wrap_exceptions on top of @memoize_when_activated of _proc_cred but it didn't work. Something about a missing attribute, IIRC.

except AccessDenied:
real = self._proc_basic_info()[proc_info_map['gid']]
effective = self._proc_basic_info()[proc_info_map['egid']]
saved = None
Copy link
Owner

Choose a reason for hiding this comment

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

PEP8 (please remove extra spaces)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will update (rebase) the commit.

except AccessDenied:
real = self._proc_basic_info()[proc_info_map['uid']]
effective = self._proc_basic_info()[proc_info_map['euid']]
saved = None
Copy link
Owner

Choose a reason for hiding this comment

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

PEP8 (please remove extra spaces)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will fix this.

with respect to strict aliasing rules and fix some warnings

For example, under strict aliasing rules of the C standard, casting a
char pointer to a struct pointer and accessing the character array
through that struct pointer yields undefined behavior.
@gsauthof
Copy link
Contributor Author

So I've updated the pull request and implemented the requested changes (HISTORY and PEP 8).

Btw, looking at the HISTORY.rst file I noticed:

- 1120_: [SunOS] cmdline() could be truncated at the 15th character when
  reading it from /proc. An extra effort is made by reading it from process
  address space first.

But #1120 doesn't really match the description. Probably a mixup with #694, right?

@giampaolo giampaolo merged commit 6276764 into giampaolo:master Mar 17, 2018
@giampaolo
Copy link
Owner

But #1120 doesn't really match the description. Probably a mixup with #694, right?

correct; thanks for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

psutil only returns uids of user-owned process on Solaris 10
2 participants