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

Add comments to 9pfs.h file #794

Closed
wants to merge 1 commit into from

Conversation

DeliaPavel
Copy link
Contributor

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.pl on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): [e.g. x86_64 or N/A]
  • Platform(s): [e.g. kvm, xen or N/A]
  • Application(s): [e.g. app-python3 or N/A]

Additional configuration

Description of changes

@DeliaPavel DeliaPavel requested a review from a team as a code owner March 11, 2023 14:20
@DeliaPavel DeliaPavel requested review from razvand and StefanJum and removed request for a team March 11, 2023 14:20
@razvand razvand added area/docs Documentation lib/9pfs 9p client labels Mar 13, 2023
@razvand razvand added this to the v0.13.0 (Atlas) milestone Mar 13, 2023
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Hi, @DeliaPavel. This looks good. See my comments.

lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
@unikraft-bot unikraft-bot added the area/lib Internal Unikraft Microlibrary label Mar 17, 2023
@DeliaPavel DeliaPavel requested a review from razvand April 2, 2023 17:42
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thanks @DeliaPavel, see the few comments.

lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks good @DeliaPavel, see how you can solve the comments from @razvand (I left some additions there) and then I think we can approve this.

lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Just few very minor comments @DeliaPavel, thanks.

lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
lib/9pfs/9pfs.h Outdated Show resolved Hide resolved
@unikraft-bot
Copy link
Member

Checkpatch failed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered errors:

SHA commit checkpatch
91a01ee Add comments to 9pfs.h file

Truncated logs starting from first error 91a01ee:

ERROR:UNIKRAFT_SUBJECT_FORMAT: Patch subject line does not follow Unikraft scheme: '[Selector]/[Component]: [Short message]'
#4: 
Subject: [PATCH] Add comments to 9pfs.h file

total: 1 errors, 0 warnings, 167 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0001-Add-comments-to-9pfs.h-file.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Change the commit message to match the guidelines (lib/9pfs: Add comments to 9pfs.h file).
I'll approve it after that.

Signed-off-by: Delia-Maria Pavel <delia_maria.pavel@stud.acs.upb.ro>
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good now, thanks a lot @DeliaPavel.
I'll leave the review tag after @razvand takes another look.

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks, @DeliaPavel.

Reviewed-by: Razvan Deaconescu razvand@unikraft.io
Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Apr 24, 2023
StefanJum pushed a commit to unikraft-upb/unikraft that referenced this pull request Jun 5, 2023
Signed-off-by: Delia-Maria Pavel <delia_maria.pavel@stud.acs.upb.ro>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Documentation area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lib/9pfs 9p client
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

4 participants