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

(Partial) Fix https://github.com/giampaolo/psutil/issues/694 #1084

Closed
wants to merge 1 commit into from

Conversation

alxchk
Copy link
Contributor

@alxchk alxchk commented May 19, 2017

I'm not really sure about your exception processing, so this code needs review. Also I may forget about some memory cleanup, who knows.

This works for me for Intel 32/64 on SunOS 10, and behavior is pretty much like pargs. Also code adds support for environment block. I'm not sure about your API, so there is some kind of issue with exception handling there.

@giampaolo
Copy link
Owner

Hello and thanks for this. I'm glad there's a SunOS internals expert out there as SunOS it's the least tested platform in psutil. With that said, to ease up the review process and trackability I would prefer two separate PRs. Do you think that is possible? Also the coding style needs some adjustments. I'm gonna start pointing them out here and now but it's totally OK to make a PR first and then I'll review that in details.

  1. better to use 4 spaces indentation all over the place

  2. do:

if (! something) {
     ...
}
else {
    ...
}

...rather than:

if (!something) {
     ...
} else {
    ...
}

...or other variants.

  1. declare all variables at the top of the function (instead of later)

  2. 1 variable declaration per line, e.g.

bad:

int i, x; 

good:

int i;
int x;
  1. always check malloc(), calloc() etc. return values and raise PyErr_NoMemory(); if needed

  2. check all function return values (e.g. for Py_BuildValue() etc)

  3. remember to free any allocated resource before returning or raising; tend to use gotos in order to do that (do goto error in case of error and do all your cleanup in the error block)

@giampaolo giampaolo closed this May 19, 2017
@alxchk
Copy link
Contributor Author

alxchk commented May 19, 2017

I'm glad there's a SunOS internals expert

Well... I see SunOS for.. 1.5 days? Something like that. Sorry about that! :)

I would prefer two separate PRs

You mean separate for env and cmdline?

Also the coding style needs some adjustments

Sure. I did that in vim in that SunOS box. Horrible experience.

@giampaolo
Copy link
Owner

giampaolo commented May 19, 2017

Well... I see SunOS for.. 1.5 days? Something like that. Sorry about that! :)

=)

You mean separate for env and cmdline?

Yes please

Sure. I did that in vim in that SunOS box. Horrible experience.

I can feel your pain. I've been doing the same for many years by emulating VMs with VirtualBox, setting up the VM (vim, git etc.) and switch between windows (horrible). Then I discovered Vagrant. With it I can start the VM, SSH into it and mount the local psutil directory into the VM. This way I can edit files locally (with my configured local editor) and run tests on the VM via SSH. Like this I switch between the (local) shell and the (local) editor windows, without using the mouse, and that's it.
If you want to try to do the same you can try this:

@giampaolo
Copy link
Owner

I realized and forgot to say that SunOS is the only Vagrant platform I don't use because I can't compile psutil with it: it uses CC compiler by default, I installed GCC, but I can't manage to compile properly, so maybe what I said above is not worth the effort, at least in case of SunOS.

@alxchk
Copy link
Contributor Author

alxchk commented May 19, 2017

Well, I'm using sshfs now, like in old good times :)
I almost refactored code. Is it ok to make 1 PR with 2 commits? Or you prefer 2 PRs (+merge)?

@giampaolo
Copy link
Owner

2 separate PRs with as many commits as you want please. For simplicity you may want to push one, wait for review + merge and then start with the next one though.

giampaolo added a commit that referenced this pull request Nov 30, 2017
giampaolo added a commit that referenced this pull request Dec 1, 2017
* update HISTORY

* update doc

* #1183: speedup Process.children() by 2.2x

* fix windows err

* #1083 / #1084: implement linux-specific ppid_map() function speending things up from 2x to 2.4x

* add ESRCH to err handling

* update doc
@giampaolo giampaolo added the bug label Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants