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

Change parameter type #28

Merged
merged 3 commits into from
May 25, 2017
Merged

Conversation

amshinde
Copy link
Contributor

No description provided.

@amshinde amshinde changed the title Change param type Change parameter type May 24, 2017
do {
ret = recv(fd, buf, (size_t)(size-offset), 0);
while(offset < size) {
ret = recv(fd, buf+offset, size-offset, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's actually a behaviour change no? I believe this is indeed a needed fix, but that kind of things shouldn't be buried in a commit that should have no functional changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least you should details how you refactored the loop in the commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two things. The refactoring (including the loop refactor) is one commit. Then there's the buf+offset, which is not part of the refactoring as far as I can see (but could be wrong). That's a second commit. But I could be wrong of course.

src/shim.c Outdated

if ( fd < 0 || ! buf ) {
if ( fd < 0 || ! buf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a spurious white space changes. The rule is no such thing sharing the same commit with other changes. The indentation in the shim is a bit all over the place, seems like we have hard time decided what should be used:

if(foo)
if (foo)
if ( foo )

and now:

if ( foo)

Life may well be too short for this, but opened #31 anyway. This isn't high priority.

@dlespiau
Copy link
Contributor

Just need an answer to the buf+offset change and if indeed a bug fix, split it in a separate commit.

@amshinde
Copy link
Contributor Author

yes, that indeed is a bug fix that I noticed while refactoring a bit, i'll split it up

@dlespiau
Copy link
Contributor

The point was that you could add a const to the first parameter of serialize_frame:

uint8_t*
serialize_frame(const struct frame *fr, ssize_t *total_size)
...

But doesn't matter that much.

@dlespiau
Copy link
Contributor

dlespiau commented May 25, 2017

lgtm

Approved with PullApprove Approved with PullApprove

@dlespiau
Copy link
Contributor

Sorry, lgtm'ed while there's still a change pending.

@amshinde amshinde force-pushed the change-param-type branch from e945706 to be0135e Compare May 25, 2017 16:30
@amshinde
Copy link
Contributor Author

@dlespiau Changes pushed

@@ -391,11 +391,16 @@ read_frame(struct cc_shim *shim)
abort();
}

if (! read_wire_data(shim->proxy_sock_fd, buf, (ssize_t)size)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this line removal on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shuffled between rewriting commits. fixed

amshinde added 3 commits May 25, 2017 10:28
Check that the header length received from proxy is at least
the minimum length defined by the proxy protocol.
We need to define an upper limit on this in the future.

Signed-off-by: Archana Shinde <[email protected]>
In case of the incomplete reads, the data needs to be
stored at the right place by incrementing the buffer address
by offset, number of bytes read in the last operation.

Signed-off-by: Archana Shinde <[email protected]>
Change the type of parameter "size" from size_t to ssize_t as this
really needs to be an unsigned value. Change the loop
to a while loop. Get rid of the casts in some of the calls to
read_wire_data.

Signed-off-by: Archana Shinde <[email protected]>
@amshinde amshinde force-pushed the change-param-type branch from be0135e to e1e5c05 Compare May 25, 2017 17:37
@dlespiau dlespiau merged commit 9137c3b into clearcontainers:master May 25, 2017
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.

3 participants