GoogleCloudPlatform / iot-device-sdk-embedded-c

Cloud IoT Device SDK for Connectivity to IoT Core.
Other
246 stars 81 forks source link

Fix faulty io net error handling code in posix BSP #101

Open pnfisher opened 4 years ago

pnfisher commented 4 years ago

Basically, when the SDK allocates the structure in which it will store the socket descriptor it opens when connecting to the iotcore server, it initializes it to 0. If during the process of creating the socket, the posix BSP routine (iotc_bsp_io_net_socket_connect) that creates the socket can’t resolve the iotcore server name, it returns an error (leaving the socket descriptor field initialized to 0). In turn, the io net routine (iotc_io_net_layer_connect) that itself called iotc_bsp_io_net_socket_connect, on seeing the error, will then execute its error handling code that itself ends up calling the io net routine iotc_io_net_layer_close_externally that then calls the posix BSP iotc_bsp_io_net_close_socket routine. The posix BSP iotc_bsp_io_net_close_socket routine then blindly closes socket descriptor 0 (since this is what the data structure tracking the socket descriptor is still initialized to since no socket was ever successfully opened).

Files modified:

src/bsp/platform/posix/iotc_bsp_io_net_posix.c

98

galz10 commented 4 years ago

Hey @pnfisher can you make a no-op change to nudge the CI

pnfisher commented 4 years ago

Yes, will try to get to this today.

googlebot commented 4 years ago

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

pnfisher commented 4 years ago

@galz10 So I did a git commit --amend --no-edit and then force pushed that to my origin/posix_stdinclosefix branch, and that seems to have restarted the CI. Not sure if that's what you meant by doing a no-op change to nudge the CI. Hope I haven't messed things up too much.

Also, for some reason the google bot seems to have decided my CLAs are no longer signed, even though it was okay with them on May 2nd when I first submitted this PR. Not sure what the problem is. Can someone on your end resolve this?

galz10 commented 4 years ago

Yea that’s what I meant , I don’t know why the CLA bot did that , did you commit to the branch using another email that you didn’t sign the CLA with ? Also if you go to the link does it say you’ve already signed a CLA ?

pnfisher commented 4 years ago

No, I sued the email I always use. And the same I used for my original PR.

galz10 commented 4 years ago

I think you just need to comment @googlebot I consent. because your CLA is signed.

pnfisher commented 4 years ago

@googlebot I consent.

pnfisher commented 4 years ago

@galz10 Any ideas on how we can get this fix unblocked?

galz10 commented 4 years ago

@pnfisher I'm working with one of the engineers to figure out the problem with Travis CI, I'll ask around to see what the problem is with the CLA

googlebot commented 4 years ago

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot commented 4 years ago

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot commented 4 years ago

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

galz10 commented 4 years ago

Hey @pnfisher everything is correct on the CLA, i'm still trying to figure out what's wrong with the Travis CI test not initializing

gguuss commented 4 years ago

@googlebot I consent

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

pnfisher commented 4 years ago

@gguuss Just wondering if there are any updates concerning this PR. Is it likely ever to be merged, or are things so messed up that we should just kill this PR and start again from scratch?