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

Added cwd configuration from cmdline to kernel #105

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Added cwd configuration from cmdline to kernel #105

merged 1 commit into from
Feb 11, 2021

Conversation

paulcallen
Copy link
Contributor

  • takes config from config.json
  • plumbs through kernel
  • fixed kernel to use cwd settings for each process
  • added parsing of config for linux target although not all settings are
    decoded yet
  • added unittests

/* process CWD. Can be set on differnt threads so need to protect it too
*/
char* cwd;
myst_spinlock_t cwd_lock;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we are stuffing more process-wise fields in myst_thread_t. At some point we have to separate out a myst_process_t to keep the size of myst_thread_t managable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is something that has concerned me. May want to create an issue on that

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use a union in struct thread (to overlap thread and process fields).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

processes also have thread items too

@@ -565,6 +560,12 @@ int myst_enter_kernel(myst_kernel_args_t* args)
thread->main.exec_crt_data = NULL;
thread->main.exec_crt_size = 0;
}
/* unmap any mapping made by the process */
myst_release_process_mappings(thread->pid);
Copy link
Contributor

Choose a reason for hiding this comment

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

The main process should never unmap itself. I think this clean up is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it causes a leak

assert(WIFEXITED(wstatus));
assert(WEXITSTATUS(wstatus) == 0);

// should still be /
Copy link
Contributor

Choose a reason for hiding this comment

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

Either the comment is wrong or the assertion below is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cut and paste bug! fixing

* takes config from config.json
* plumbs through kernel
* fixed kernel to use cwd settings for each process
* added parsing of config for linux target although not all settings are
decoded yet
Copy link
Contributor

@mikbras mikbras left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you for doing this!

kernel/exec.c Outdated
@@ -811,6 +811,7 @@ int myst_exec(
const char* argv[],
size_t envc,
const char* envp[],
MYST_UNUSED const char* current_working_directory,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually used below. Should we remove MYST_UNUSED?

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 have already removed that

@paulcallen paulcallen merged commit 1ba1432 into deislabs:main Feb 11, 2021
@paulcallen paulcallen deleted the paulcallen-cwd-config branch February 11, 2021 20:49
@paulcallen paulcallen mentioned this pull request Feb 11, 2021
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.

3 participants