Skip to content
This repository was archived by the owner on May 6, 2020. It is now read-only.

agent: read full data from ctl and tty channels#202

Merged
amshinde merged 1 commit intoclearcontainers:masterfrom
devimc:fix201
Jan 27, 2018
Merged

agent: read full data from ctl and tty channels#202
amshinde merged 1 commit intoclearcontainers:masterfrom
devimc:fix201

Conversation

@devimc
Copy link
Copy Markdown

@devimc devimc commented Jan 23, 2018

this patch is to read all the data sent by the proxy and to avoid
error reading more than 4096 bytes

fixes #201

Signed-off-by: Julio Montes julio.montes@intel.com

@jodh-intel
Copy link
Copy Markdown

Hi @devimc - looks fine, but please can you add some tests for readAll() (and ideally for readCtl() and readTty() too). It would be good if you could test a variety of length including -1 (for readAll()), 0, 1, 4095, 4096, 4097' and maybe a huge read of a few MB.

@devimc
Copy link
Copy Markdown
Author

devimc commented Jan 24, 2018

@jodh-intel I always forget to write unit tests 😄 , lemme write them
thanks

@grahamwhaley
Copy link
Copy Markdown
Contributor

heh heh - one way around that @devimc is to write the tests before the code ;-) TDD

@jodh-intel
Copy link
Copy Markdown

... which is odd because you are really good at writing them! 😄 Thanks.

@devimc
Copy link
Copy Markdown
Author

devimc commented Jan 24, 2018

@jodh-intel changes applied, thanks

agent_test.go Outdated
t.Error(err)
}
if uint32(n) != uint32(ctlHeaderSize) {
t.Error("expected: %d, actual: %d", n, ctlHeaderSize)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Travis is failing as the linters have detected this and a few other calls should be t.Errorf().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oops!, fixed, thanks

this patch is to read all the data sent by the proxy and to avoid
error reading more than 4096 bytes

fixes clearcontainers#201

Signed-off-by: Julio Montes <julio.montes@intel.com>
Copy link
Copy Markdown
Contributor

@amshinde amshinde left a comment

Choose a reason for hiding this comment

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

lgtm

@amshinde amshinde merged commit 817485a into clearcontainers:master Jan 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

failed to read more than 4096 bytes from ctl

4 participants