Check errors #431

mcardosos · 2016-10-29T02:33:29Z

Fixes issue 428
Please take a look @vishrutshah @jhendrixMSFT

jhendrixMSFT · 2016-11-01T17:41:55Z

management/vmutils/extensions.go

@@ -58,7 +58,9 @@ func AddAzureDockerVMExtensionConfiguration(role *vm.Role, dockerPort int, versi
 		return fmt.Errorf(errParamNotSpecified, "role")
 	}

-	ConfigureWithExternalPort(role, "docker", dockerPort, dockerPort, vm.InputEndpointProtocolTCP)
+	if err := ConfigureWithExternalPort(role, "docker", dockerPort, dockerPort, vm.InputEndpointProtocolTCP); err != nil {
+		panic(err)


Why not return err here like the other branches in this function?

Ohh, true that. Will change :D

jhendrixMSFT · 2016-11-01T17:47:05Z

storage/blob.go

 	return checkRespCode(resp.statusCode, []int{http.StatusCreated})
 }

 // CreateContainerIfNotExists creates a blob container if it does not exist. Returns
 // true if container is newly created or false if container already exists.
-func (b BlobStorageClient) CreateContainerIfNotExists(name string, access ContainerAccessType) (bool, error) {
+func (b BlobStorageClient) CreateContainerIfNotExists(name string, access ContainerAccessType) (notExists bool, err error) {


Why was the bool return valued named? I don't see the name referenced in the method body. I see this same pattern in other methods too.

I named the bool because if I name the error, all returning stuff should be named.
The error is named so errors from defer can be returned and checked.

I see, makes sense.

jhendrixMSFT · 2016-11-01T18:21:41Z

storage/blob.go

-		defer resp.body.Close()
+		defer func() {
+			err = resp.body.Close()
+		}()
 		if resp.statusCode == http.StatusCreated || resp.statusCode == http.StatusConflict {
 			return resp.statusCode == http.StatusCreated, nil
 		}


The function after this one, createContainer, needs to have the same fix applied.

OK after re-reading the implementation I don't think it does.

jhendrixMSFT · 2016-11-01T18:28:05Z

Changes look good.

ahmetb · 2016-12-08T19:55:02Z

.travis.yml

@@ -31,3 +32,10 @@ script:
  - go vet ./management/...
  - test -z "$(golint ./Gododir/... | tee /dev/stderr)"
  - go vet ./Gododir/...
+  - errcheck -ignoretests ./arm/... 


can be shortened: set -e; for v in ./arm... ./datalake-store/... ./Gododir/...; do errcheck -ignoretests "$v"; done

ahmetb · 2016-12-08T19:56:18Z

management/util.go

-func getResponseBody(response *http.Response) ([]byte, error) {
-	defer response.Body.Close()
+func getResponseBody(response *http.Response) (out []byte, err error) {
+	defer func() {


I'm not super ok with this one 1) response.Body can be nil 2) returning Close() error is a behavior change. also why do you add named returns?

I'm returning named results so the errors inside the defer function can actually be returned.
Question, will Close() return an error on an empty / nil response.Body? I that is the case then I agree it is not as useful to return an error from close, because ReadAll would just return an empty []byte.
Will fix

Question, will Close() return an error on an empty / nil response.Body?

it will panic with nil dereference error.

ahmetb · 2016-12-08T23:24:15Z

.travis.yml

@@ -31,3 +32,6 @@ script:
  - go vet ./management/...
  - test -z "$(golint ./Gododir/... | tee /dev/stderr)"
  - go vet ./Gododir/...
+  - test -z "$(set -e; for v in ./arm... ./datalake-store/... ./Gododir/...; do (errcheck -ignoretests "$v" | grep -v 'resp.body.Close()' | tee /dev/stderr); done)""


if you will have test -z, set -e is not necessary.

instead of this grep hack, perhaps you can do:

defer func(){ _ = resp.Body.Close() }()

can you please check all callers of getRequestBody() make sure that error from the request is nil and then go on reading the body? (if error from http.Do/Get/... is non-nil, then resp.Body will be nil, causing the panic in defer resp.Body.Close()

You have grep -v 'resp.body.Close()' but I think body should be Body?

mcardosos · 2016-12-09T19:28:10Z

Travis is passing. How does this look @ahmetalpbalkan ?

ahmetb · 2016-12-09T19:29:11Z

.travis.yml

@@ -30,3 +31,6 @@ script:
  - go vet ./management/...
  - test -z "$(golint ./Gododir/... | tee /dev/stderr)"
  - go vet ./Gododir/...
+  - set -e; for v in ./arm/... ./datalake-store/... ./Gododir/... ./management/... ./storage/...; do errcheck -ignoretests "$v"; done


are you sure errcheck returns with non-zero exitcode when it finds issues? if so you may still need the test -z.

Travis fails with errcheck, take a look at this build. And from errcheck documentation.

That's fair. I think I tried integrating errcheck long time ago manually and it just came with tons of changes which were not necessary (see my other comment). I just started to recall that...

ahmetb · 2016-12-09T19:30:34Z

storage/blob.go

@@ -439,7 +439,9 @@ func (b BlobStorageClient) ListContainers(params ListContainersParameters) (Cont
 	if err != nil {
 		return out, err
 	}
-	defer resp.body.Close()
+	defer func() {


@mcardosos I strongly suggest turning off errcheck for this package. It makes the code really ugly with this change. It's idiomatic go to call defer resp.body.Close() and discard the output implicitly.

I am sorry that I did not see this coming. I thought you were just going to change 1-2 places in management package. But this results in quite ugly code for this package.

We must use static analysis tools like this occasionally to catch bugs but for cases like these, they are deliberate decisions and we should not be using hacks like this all over the place to suppress the static analysis tool. I think errcheck is a bit extreme. Perhaps just run errcheck | grep -v Close or something again. :(

ahmetb · 2016-12-09T20:48:32Z

storage/util.go

 	h := hmac.New(sha256.New, c.accountKey)
-	h.Write([]byte(message))
-	return base64.StdEncoding.EncodeToString(h.Sum(nil))
+	if _, err := h.Write([]byte(message)); err != nil {


Kinda mixed feelings about changing this. the hmac.inner == sha256 type's Write() will never return an error as everything is in-memory. 😕

yeah this change causes many changes in the upstream callers whereas the error can be just discarded. wdyt?

errcheck doesn't like it. In the other hand, gas doesn't really care. Looking at all this again, I actually think gas is more reasonable. I think it would be better if instead of gas -skip=*/arm/*/models.go -skip=*/management/examples/*.go -skip=*vendor* -skip=*/Gododir/* ./..., I rewrote that line so for all this skipped files I just exclude whatever rule they break. For example, I'm skipping all the models.go files because they "break" rule G101.

And we can then get rid of errcheck :D

ahmetb · 2016-12-09T20:48:48Z

storage/queue.go

@@ -319,7 +319,7 @@ func (c QueueServiceClient) DeleteMessage(queue, messageID, popReceipt string) e
 	if err != nil {
 		return err
 	}
-	defer resp.body.Close()
+	deferresp.body.Close()


ahmetb · 2016-12-09T20:49:33Z

storage/util.go

 	h := hmac.New(sha256.New, c.accountKey)
-	h.Write([]byte(message))
-	return base64.StdEncoding.EncodeToString(h.Sum(nil))
+	if _, err := h.Write([]byte(message)); err != nil {


yeah this change causes many changes in the upstream callers whereas the error can be just discarded. wdyt?

mcardosos added 7 commits October 10, 2016 18:05

Added rakefile (original in github.com/Azure/autorest)

8285212

Rakefile translated to golang

6eb4fe0

Updated travis file

00a0145

Removed rakefile

40fb143

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-go

7478058

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-go

e01c89c

Included errcheck

f177877

msftclas added the cla-already-signed label Oct 29, 2016

Fix metadata

a7de7ad

jhendrixMSFT reviewed Nov 1, 2016

View reviewed changes

jhendrixMSFT mentioned this pull request Nov 1, 2016

Containeracl #422

Merged

Removed panic

1c94249

ahmetb reviewed Dec 8, 2016

View reviewed changes

Ignoring resp.body.Close() errors

9ca7400

mcardosos force-pushed the check-err branch from 57509c5 to 9ca7400 Compare December 8, 2016 23:20

ahmetb reviewed Dec 8, 2016

View reviewed changes

mcardosos added 5 commits December 8, 2016 16:18

Discarding defer errors

9048767

Merge from master

ef112da

Fixed merged stuff

bf23f32

Fix management errcheck

ef160cf

Exclude errors rule in gas scanning

f7e4e42

ahmetb suggested changes Dec 9, 2016

View reviewed changes

Ignore checkerr issues involving Close()

3fa71a0

mcardosos force-pushed the check-err branch from 857df19 to 3fa71a0 Compare December 9, 2016 20:13

ahmetb reviewed Dec 9, 2016

View reviewed changes

ahmetb approved these changes Dec 9, 2016

View reviewed changes

mcardosos force-pushed the check-err branch 3 times, most recently from 50791d3 to 9a1b963 Compare December 12, 2016 23:35

Get rid of gocheck, and add specific gas tests

3b3aa5e

mcardosos force-pushed the check-err branch from 9a1b963 to 3b3aa5e Compare December 12, 2016 23:40

marstr approved these changes Dec 13, 2016

View reviewed changes

marstr merged commit 2fb0ef4 into Azure:master Dec 13, 2016

mcardosos deleted the check-err branch January 12, 2017 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check errors #431

Check errors #431

mcardosos commented Oct 29, 2016

jhendrixMSFT Nov 1, 2016

mcardosos Nov 1, 2016

jhendrixMSFT Nov 1, 2016

mcardosos Nov 1, 2016

jhendrixMSFT Nov 1, 2016

jhendrixMSFT Nov 1, 2016

jhendrixMSFT Nov 1, 2016

jhendrixMSFT commented Nov 1, 2016

ahmetb Dec 8, 2016

ahmetb Dec 8, 2016

mcardosos Dec 8, 2016

ahmetb Dec 8, 2016

ahmetb Dec 8, 2016

mcardosos commented Dec 9, 2016

ahmetb Dec 9, 2016

mcardosos Dec 9, 2016

ahmetb Dec 9, 2016

ahmetb Dec 9, 2016

ahmetb Dec 9, 2016

ahmetb Dec 9, 2016

mcardosos Dec 9, 2016

mcardosos Dec 9, 2016

ahmetb Dec 9, 2016

mcardosos Dec 9, 2016

ahmetb Dec 9, 2016

Check errors #431

Check errors #431

Conversation

mcardosos commented Oct 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhendrixMSFT commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcardosos commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment