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

Fix for utimensat01 test #67

Open
wants to merge 1 commit into
base: sgx-lkl
Choose a base branch
from

Conversation

rsrinivas01
Copy link

Modified the utimensat01.c file to check for different error and success conditions. Instead of using the utimensat_test.sh script the test will be run directly from the c file.

Copy link
Author

@rsrinivas01 rsrinivas01 left a comment

Choose a reason for hiding this comment

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

@hukoyu @shaikshavali1.. Please review the changes

@hukoyu
Copy link
Collaborator

hukoyu commented Aug 12, 2020

@shaikshavali1 can you do a peer review? I will review after you approve this PR.

Copy link

@shaikshavali1 shaikshavali1 left a comment

Choose a reason for hiding this comment

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

@rsrinivas01, Could you please rename the original main function, invoke it from your new main with test parameters, instead of commenting out the original main.

@rsrinivas01 rsrinivas01 force-pushed the utimensat01 branch 27 times, most recently from 91aec6f to 2b38158 Compare August 17, 2020 06:30
@rsrinivas01 rsrinivas01 force-pushed the utimensat01 branch 6 times, most recently from 995efb8 to bce9dd1 Compare August 17, 2020 07:12
Copy link
Author

@rsrinivas01 rsrinivas01 left a comment

Choose a reason for hiding this comment

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

@shaikshavali1 @hukoyu ..

Addressed review comments. Please check and let me know

Copy link

@shaikshavali1 shaikshavali1 left a comment

Choose a reason for hiding this comment

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

Please fix the below minor comments. Others are fine. Approved.

  1. some empty lines are removed example Line-144. Please, keep these lines
  2. use "tst_resm" in place of printf

Copy link
Author

@rsrinivas01 rsrinivas01 left a comment

Choose a reason for hiding this comment

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

@shaikshavali1 @hukoyu

Updated as per latest comments. Please approve

@davidchisnall
Copy link

I think the code looks like it might be correct, but the complete lack of explanatory comments means that I can't review this.

@SeanTAllen
Copy link

There's no comments here for why any of the changes were made. It looks to be extensive. At minimum, this needs a good amount of commenting for what is being tested and why. No one is likely to be able to come along and support this later (or now).

@hukoyu
Copy link
Collaborator

hukoyu commented Aug 18, 2020

@rsrinivas01 can you add comments to this PR explaining code addition and removals. Please add lots of comments.
cc @shaikshavali1

@rsrinivas01
Copy link
Author

I think the code looks like it might be correct, but the complete lack of explanatory comments means that I can't review this.

Added comments in the file explaining the changes. Please check.

@rsrinivas01
Copy link
Author

There's no comments here for why any of the changes were made. It looks to be extensive. At minimum, this needs a good amount of commenting for what is being tested and why. No one is likely to be able to come along and support this later (or now).

Comments added in the file explaining the changes. Please check and let me know.

Copy link
Author

@rsrinivas01 rsrinivas01 left a comment

Choose a reason for hiding this comment

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

@davidchisnall @SeanTAllen @hukoyu

Added comments in the file explaining the changes. Please check and let me know your views.

@SeanTAllen
Copy link

I've reviewed a couple times today. Both times I ended up confused by what is going on. I'll give it another try tomorrow.

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.

5 participants