On Wed, May 11, 2011 at 12:28:29PM +0200, Michel Hermier <[email protected]> wrote: > I just saw something *big* > case PM_OP_PS: ret = pspkg(pm_targets); break; > and then the declaration is: > int pspkg(); > You should add the void or the pm_targets type to the call.
Fixed. > (maybe all our > () prototype should also be changed to (void) ?) That's possible (kernel coding style requeres that, for example), but is not a must. > Smaller issues: > * ps_parse lacks a static in the declaration. Fixed. > * lsof is executed using execvp which might be problematic since it > exports all the environment, leading to potentioal locale issues at least. That's not a problem, lsof is not localized and we parse a machine output, which won't even contain English strings. > * on execvp failure child process should die not return Fixed. > * The parent should also check the fdopen return, just in case the child > was faster, and execvp failed (leading to an appempt to read a pipe > without writer, which is an error) Fxied. > * end_lsof should be input values guarded (checking NULL file and 0 pid) That's not necessary, start_lsof() guarantees these constrains as we have checks for these there. > * struct __ps_t don't have to be library public, and pid should be of > pid_t type. __ps_t is in the frontend at the moment, so it makes no sense to make it private. We can do that if we move it to libpacman. I fixed the int vs pid_t issue. > I wait review for now, maybe some more on the next round ;) Thanks a lot, pushed out. :)
pgprIauIsjaPp.pgp
Description: PGP signature
_______________________________________________ Frugalware-devel mailing list [email protected] http://frugalware.org/mailman/listinfo/frugalware-devel
