Skip to content

Changes to support ICS network on windows - #1755

Merged
mavenugo merged 1 commit into
moby:masterfrom
msabansal:msabansal/ics
Jul 7, 2017
Merged

mavenugo merged 1 commit into
moby:masterfrom
msabansal:msabansal/ics

Conversation

@msabansal

Copy link
Copy Markdown
Contributor

We were missing some basic properties. Added that.

Signed-off-by: Sandeep Bansal sabansal@microsoft.com

case MacPool:
config.MacPools = make([]hcsshim.MacPool, 0)
s := strings.Split(value, ",")
for i := 0; i < len(s)-1; i += 2 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't the last two elements be discarded because of the -1 in i < len(s)-1 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The mac pool range must be specified in pair. For odd number length I want to ignore the last value. If you want we can throw an error in case of odd numbers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, ok, I was assuming this will always come with an even number of elements.
As you suggested, I think it is better to explicitly fail in case of a malformed request, odd number of elements.

@aboch

aboch commented May 16, 2017

Copy link
Copy Markdown
Contributor

LGTM

Signed-off-by: Sandeep Bansal <sabansal@microsoft.com>
if len(s)%2 != 0 {
return nil, types.BadRequestErrorf("Invalid mac pool. You must specify both a start range and an end range")
}
for i := 0; i < len(s)-1; i += 2 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

with the <, you don't really need the -1 in the upper bound of the loop (also because you already checked that the number of elements is even).
if len(s) == 2 in the first loop i = 0, then i = 2 < 2 is already false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Accepted but not taking this change. Will address in some future iteration. Thanks.

@msabansal

Copy link
Copy Markdown
Contributor Author

@sanimej PTAL

@msabansal

Copy link
Copy Markdown
Contributor Author

@mrjana @mavenugo PTAL. We have moved to self hosting ICS networks and this is causing issues with docker startup

@lowenna

lowenna commented Jul 7, 2017

Copy link
Copy Markdown
Member

LGTM (not a maintainer on this repo).

@mavenugo

mavenugo commented Jul 7, 2017

Copy link
Copy Markdown
Contributor

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.

6 participants