Skip to content

Conversation

@crosbymichael
Copy link
Member

Marshall the raw objects for the sync pipes so that no new line chars
are left behind in the pipe causing errors.

Signed-off-by: Michael Crosby [email protected]

Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to have the returned length and expected value here, something like: fmt.Errorf("object not written, got(%d), expected(%d)", n, len(data))

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 26, 2016

I think having a separate pipe for sync would significantly simplify the code. But for now, this code is fine.

@crosbymichael
Copy link
Member Author

@LK4D4 @dqminh fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

%s --> %d

Copy link
Member Author

Choose a reason for hiding this comment

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

it was a typo!

Copy link
Contributor

Choose a reason for hiding this comment

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

s and d are so close :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dqminh I'm pretty sure that Write will return io.ShortWrite before this line :) but it's ok I think

Copy link
Contributor

Choose a reason for hiding this comment

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

yah thats true ( Write must return a non-nil error if it returns n < len(p)), so i dont think we need this check

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 26, 2016

LGTM

Marshall the raw objects for the sync pipes so that no new line chars
are left behind in the pipe causing errors.

Signed-off-by: Michael Crosby <[email protected]>
@mrunalp
Copy link
Contributor

mrunalp commented Jan 26, 2016

LGTM. @crosbymichael Not sure if we want to wait for the docker testing results to come back or not before merging.

@csfrancis
Copy link

Tested the patch provided in #508 (comment) against docker 1.9.1 in production and it resolves our issue. Thanks!

@crosbymichael
Copy link
Member Author

@csfrancis awesome! thanks for your help

crosbymichael added a commit that referenced this pull request Jan 26, 2016
Do not use stream encoders for pipe communication
@crosbymichael crosbymichael merged commit 7cd384c into opencontainers:master Jan 26, 2016
@crosbymichael crosbymichael deleted the readall branch January 26, 2016 22:37
@mrunalp
Copy link
Contributor

mrunalp commented Jan 26, 2016

Nice!

@mrunalp
Copy link
Contributor

mrunalp commented Jan 26, 2016

I have created moby/moby#19752 to bump the version in docker

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017
…number

README: Update conference-call phone number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants