Skip to content

Conversation

@mateusrodrigues
Copy link

PR Summary

As cited in #7207, these are the changes I've made to libpsl-native to fix all tests on FreeBSD. The only two files that needed changes were src/getuserfrompid.cpp and src/isfile.cpp:

  • In isfile.cpp, I've changed the logic to verify whether something is a file or not by using a macro available in sys/stat.h. This changed allowed IsFileTest.RootIsFile to pass.
  • In getuserfrompid.cpp, a FreeBSD-specific logic wasn't defined, so NULL was being returned every time. Therefore, I've added a logic similar to the macOS one with a subtle difference in the return statement. This allowed GetUserFromPid.Success to pass.

PR Checklist

@msftclas
Copy link

msftclas commented Jun 29, 2018

CLA assistant check
All CLA requirements met.

}

// TODO: Real of effective user ID?
return GetPwUid(oldp.ki_uid);
Copy link
Author

Choose a reason for hiding this comment

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

ki_uid returns effective user id, where as ki_ruid returns real user id and ki_svuid returns saved effective user id. Which one should we look for in this case?

if (lstat(path, &buf) == -1)
{
// TODO: throw error on path doesn't exist?
return false;
Copy link
Author

Choose a reason for hiding this comment

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

If path does not exist, is returning false enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

The unit test test-isfile.cpp currently expects false for a file that does not exit. See https://github.com/PowerShell/PowerShell/blob/master/src/libpsl-native/test/test-isfile.cpp

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I would suggest testing for path before using it; something like the following:

if (path == NULL || lstat(path, &buf) == -1)

@daxian-dbw daxian-dbw self-assigned this Jun 29, 2018
@iSazonov
Copy link
Collaborator

Did we migrate the psl to https://github.com/PowerShell/PowerShell-Native?

@iSazonov
Copy link
Collaborator

After merging #7129 we will no longer use src/isfile.cpp.

@TravisEz13
Copy link
Member

@adityapatwardhan Can we answer @iSazonov 's question? When should we stop taking PRs here?

@adityapatwardhan
Copy link
Member

@mateusrodrigues Could you submit this PR to https://github.com/PowerShell/PowerShell-Native. We are moving the native code to a separate repo. Thanks for your contribution.

@mateusrodrigues
Copy link
Author

@adityapatwardhan just moved the PR over

@adityapatwardhan
Copy link
Member

@mateusrodrigues Thanks! I will close this PR.

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.

7 participants