Skip to content

Correctly set silo field when opening job object#1437

Merged
dcantah merged 2 commits intomicrosoft:masterfrom
dcantah:jobobj-opensilo
Jun 29, 2022
Merged

Correctly set silo field when opening job object#1437
dcantah merged 2 commits intomicrosoft:masterfrom
dcantah:jobobj-opensilo

Conversation

@dcantah
Copy link
Copy Markdown
Contributor

@dcantah dcantah commented Jun 22, 2022

We don't set the silo field on Open of an existing job object today.
This is useful if once opening a job we want to bind a file that only
that silo can see as it relies on atomically checking the silo u32
field to determine if we can carry out the operation.

The manner in which we check if the job is a silo is by using a new
jobobject information class with QueryInformationJobObject that fails
unless the job is a silo.

@dcantah dcantah requested a review from a team as a code owner June 22, 2022 12:31
Comment thread internal/jobobject/jobobject.go
Comment thread internal/jobobject/jobobject.go Outdated
Comment thread internal/jobobject/jobobject.go
@dcantah dcantah force-pushed the jobobj-opensilo branch 2 times, most recently from a6700d3 to 66618c5 Compare June 24, 2022 14:58
@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Jun 24, 2022

Rebased to pickup CI fix for mingw

@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Jun 24, 2022

@kevpar ptal when you get a chance again

Comment thread internal/jobobject/jobobject.go Outdated
dcantah added 2 commits June 29, 2022 11:26
We don't set the silo field on Open of an existing job object today.
This is useful if once opening a job we want to bind a file that only
that silo can see as it relies on atomically checking the `silo` u32
field to determine if we can carry out the operation.

The manner in which we check if the job is a silo is by using a new
jobobject information class with QueryInformationJobObject that fails
unless the job is a silo.

Signed-off-by: Daniel Canter <[email protected]>
Change to isJobSilo from pointer receiver method as calling a method
on an object we made in the same function is a bit odd.

Signed-off-by: Daniel Canter <[email protected]>
@kevpar
Copy link
Copy Markdown
Member

kevpar commented Jun 29, 2022

As a side note, it seems weird to have Silo as a field on Options, given this struct is shared between Open and Create. This is the kind of thing we would want to fix before moving this package out of internal.

Copy link
Copy Markdown
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Jun 29, 2022

As a side note, it seems weird to have Silo as a field on Options, given this struct is shared between Open and Create. This is the kind of thing we would want to fix before moving this package out of internal.

Agree, especially as it does nothing for Open

@dcantah dcantah merged commit 7049997 into microsoft:master Jun 29, 2022
kiashok pushed a commit to kiashok/hcsshim that referenced this pull request Jul 11, 2022
* Correctly set silo field when opening job object

We don't set the silo field on Open of an existing job object today.
This is useful if once opening a job we want to bind a file that only
that silo can see as it relies on atomically checking the `silo` u32
field to determine if we can carry out the operation.

The manner in which we check if the job is a silo is by using a new
jobobject information class with QueryInformationJobObject that fails
unless the job is a silo.

Signed-off-by: Daniel Canter <[email protected]>
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
* Correctly set silo field when opening job object

We don't set the silo field on Open of an existing job object today.
This is useful if once opening a job we want to bind a file that only
that silo can see as it relies on atomically checking the `silo` u32
field to determine if we can carry out the operation.

The manner in which we check if the job is a silo is by using a new
jobobject information class with QueryInformationJobObject that fails
unless the job is a silo.

Signed-off-by: Daniel Canter <[email protected]>
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