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

[#18] Implement halt system call #43

Merged
merged 1 commit into from
Apr 9, 2017
Merged

[#18] Implement halt system call #43

merged 1 commit into from
Apr 9, 2017

Conversation

Lment
Copy link
Contributor

@Lment Lment commented Apr 9, 2017

Implementation of halt system call, using function defined in init.h.
shutdown_power_off() is unavailable in our pintos.

@Lment Lment self-assigned this Apr 9, 2017
@Lment Lment requested a review from hangpark April 9, 2017 10:37
@@ -91,7 +92,7 @@ syscall_handler (struct intr_frame *f UNUSED)
static void
syscall_halt (void)
{

power_off();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Use macro

Copy link
Owner

@hangpark hangpark Apr 9, 2017

Choose a reason for hiding this comment

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

See this.

@@ -91,7 +92,7 @@ syscall_handler (struct intr_frame *f UNUSED)
static void
syscall_halt (void)
{

power_off();
Copy link
Owner

Choose a reason for hiding this comment

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

Writing comment above this would be better.

@Lment Lment force-pushed the iss/18 branch 2 times, most recently from 11c3143 to dd9694e Compare April 9, 2017 12:21
@@ -88,10 +90,12 @@ syscall_handler (struct intr_frame *f UNUSED)
}
}

/* This syscall terminates pintos. */
Copy link
Owner

Choose a reason for hiding this comment

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

Please follow the format of other explanation comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault.

@Lment Lment force-pushed the iss/18 branch 2 times, most recently from 458f0d6 to 06746bf Compare April 9, 2017 13:17
@Lment Lment changed the title [#18] Implementation of halt() [#18] Implementation of halt system call Apr 9, 2017
@Lment Lment changed the title [#18] Implementation of halt system call [#18] Implementation of halt system call handler Apr 9, 2017
@Lment Lment changed the title [#18] Implementation of halt system call handler [#18] Implement of halt system call handler Apr 9, 2017
@Lment Lment changed the title [#18] Implement of halt system call handler [#18] Implement halt system call handler Apr 9, 2017
@@ -88,10 +90,12 @@ syscall_handler (struct intr_frame *f UNUSED)
}
}

/* Terminate pintos by calling built-in command. */
Copy link
Owner

Choose a reason for hiding this comment

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

whether calling built-in command or non-built-in command is not important. And other comments use terminates, not terminate I think. Check it.

Copy link
Contributor Author

@Lment Lment Apr 9, 2017

Choose a reason for hiding this comment

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

/* Free the current process's resources. */ 
/* Create a minimal stack by mapping a zeroed page at the top of user virtual memory. */
/* Waits for thread TID to die and returns its exit status. 
/* Starts a new thread running a user program loaded from

@hangpark
In process.c, both appears. I didn't think much of it.
So you prefer -s on comments..? Then I'll follow it.

Copy link
Owner

@hangpark hangpark Apr 9, 2017

Choose a reason for hiding this comment

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

Anything would be ok, then. You can choose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not use -s from now on then.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok. I'll follow too.

@hangpark hangpark changed the title [#18] Implement halt system call handler [#18] Implement halt system call Apr 9, 2017
Copy link
Owner

@hangpark hangpark left a comment

Choose a reason for hiding this comment

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

Changing commit title to implement halt system call would be better.

Copy link
Owner

@hangpark hangpark left a comment

Choose a reason for hiding this comment

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

Implementation of halt system call, using function defined in init.h
This is your commit message.

  • Where is comma?
  • You should wrap paths, function names, file names, etc., with `.
  • threads/init.h is better than init.h.
  • Which function? Specify it.

@@ -88,10 +90,12 @@ syscall_handler (struct intr_frame *f UNUSED)
}
}

/* Terminate pintos by calling built-in command. */
Copy link
Owner

Choose a reason for hiding this comment

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

You said you will use -s on verb. Why didn't this change yet? Also, by calling ~~~ is not needed I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote 'would not use' them...

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, okay.

Copy link
Owner

@hangpark hangpark left a comment

Choose a reason for hiding this comment

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

Update to develop please.

Implementation of `halt ()` system call, using `power_off ()` defined in `threads/init.h`.
@hangpark hangpark merged commit 9cda954 into develop Apr 9, 2017
@hangpark hangpark deleted the iss/18 branch April 9, 2017 19:53
@hangpark hangpark mentioned this pull request Apr 9, 2017
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.

2 participants