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

Check pointer fields of g.spec #144

Merged
merged 1 commit into from
Jul 26, 2016

Conversation

haiyanmeng
Copy link
Contributor

The PR tries to check whether a pointer field is nil before modifying the field. This avoids modifying the field of a nil pointer.

Signed-off-by: Haiyan Meng hmeng@redhat.com

@haiyanmeng
Copy link
Contributor Author

@mrunalp , @wking , PTAL.
This PR is not ready to be merged directly, and should only be merged after #140.
I created the PR here, so you can see what I am up to.

@wking
Copy link
Contributor

wking commented Jul 25, 2016

On Sun, Jul 24, 2016 at 01:25:15PM -0700, hmeng-19 wrote:

The PR tries to check whether a pointer field is nil before
modifying the field.

I think we want ‘initialize_’ instead of ‘check_’, but the helpers are
more compact than the direct entries I'm currently using in #112. Do
we have some threshold number of calls under which a helper function
is more mental overhead than its worth, or do we want to use
initializers for all of this sort of thing for consistency?

@haiyanmeng
Copy link
Contributor Author

@wking , you are right. initSpec* is a better name. Fixed it.

Signed-off-by: Haiyan Meng <hmeng@redhat.com>
@haiyanmeng
Copy link
Contributor Author

@mrunalp , @wking , PTAL.
Since #140 has been merged, I think it is time to consider this PR.

@wking
Copy link
Contributor

wking commented Jul 26, 2016

Travis isn't happy with 889f775, possibly because of #140 landing 1:

go build -tags "" -o ocitools ./cmd/ocitools

github.com/opencontainers/ocitools/generate

generate/generate.go:444: cannot convert nil to type specs.Linux

I have some nil-Linux guards in #112, but they don't cover #137's
single-field setters like SetLinuxResourcesCPUShares. We can add
guards to those methods in either this PR or #112, but I'd rather make
the config parameter public and drop the setters 2.

@haiyanmeng
Copy link
Contributor Author

@wking , I rebased this PR on top of the current master, the head of the PR should be 60df8fc . On my side, github shows that All checks have passed.

@wking
Copy link
Contributor

wking commented Jul 26, 2016

On Tue, Jul 26, 2016 at 08:18:19AM -0700, hmeng-19 wrote:

@wking , I rebased this PR on top of the current master, the head of
the PR should be
60df8fc
. On my side, github shows that All checks have passed.

Ah, looks like you pushed that while I was working on my comment.
60df8fc looks good to me, and with that this PR replaces #112.

@wking
Copy link
Contributor

wking commented Jul 26, 2016

On Tue, Jul 26, 2016 at 10:00:48AM -0700, W. Trevor King wrote:

Ah, looks like you pushed that while I was working on my comment.
60df8fc looks good to me, and with that this PR replaces #112.

Actually, #112 still has the empty-Linux JSON serialization change, so
I'll keep it open. After this PR lands, I'll rebase #112 on top of
the new master.

@haiyanmeng
Copy link
Contributor Author

@wking , sounds good.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 26, 2016

LGTM

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