New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fd leak and error handling #33713

Merged
merged 1 commit into from Jul 6, 2017

Conversation

Projects
None yet
6 participants
@x1022as
Contributor

x1022as commented Jun 16, 2017

Signed-off-by: Deng Guangxing dengguangxing@huawei.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah changed the title from fd leak and error hanlding to fd leak and error handling Jun 16, 2017

Show outdated Hide outdated pkg/discovery/generator.go Outdated
@cpuguy83

LGTM

rdr.Seek(rdrOffset, os.SEEK_SET)
if _, err := rdr.Seek(rdrOffset, os.SEEK_SET); err != nil {
return -1, err
}
// make sure all following readers are at 0
for _, rdr := range r.readers[i+1:] {
rdr.Seek(0, os.SEEK_SET)

This comment has been minimized.

@aaronlehmann

aaronlehmann Jun 21, 2017

Contributor

Does this one need an error check as well?

@aaronlehmann

aaronlehmann Jun 21, 2017

Contributor

Does this one need an error check as well?

This comment has been minimized.

@x1022as

x1022as Jun 22, 2017

Contributor

I think errors should be ignored and continue the for loop, so maybe no need to check here.

@x1022as

x1022as Jun 22, 2017

Contributor

I think errors should be ignored and continue the for loop, so maybe no need to check here.

@x1022as

This comment has been minimized.

Show comment
Hide comment
@x1022as

x1022as Jun 23, 2017

Contributor

janky failed seems not related. will push again to re-run it.

Contributor

x1022as commented Jun 23, 2017

janky failed seems not related. will push again to re-run it.

fd leak and error handling
Signed-off-by: Deng Guangxing <dengguangxing@huawei.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Jun 29, 2017

ping @aaronlehmann PTAL

@@ -67,7 +69,9 @@ func (r *multiReadSeeker) Seek(offset int64, whence int) (int64, error) {
}
tmpOffset += s
}
r.Seek(tmpOffset+offset, os.SEEK_SET)
if _, err := r.Seek(tmpOffset+offset, os.SEEK_SET); err != nil {

This comment has been minimized.

@aaronlehmann

aaronlehmann Jun 29, 2017

Contributor

Rather than calling Seek recursively here, which seems to do a lot of redundant work, couldn't we just set r.pos directly?

@aaronlehmann

aaronlehmann Jun 29, 2017

Contributor

Rather than calling Seek recursively here, which seems to do a lot of redundant work, couldn't we just set r.pos directly?

This comment has been minimized.

@vieux

vieux Jul 4, 2017

Collaborator

ping @x1022as

@vieux

vieux Jul 4, 2017

Collaborator

ping @x1022as

This comment has been minimized.

@thaJeztah

thaJeztah Jul 5, 2017

Member

@aaronlehmann @vieux if this is an important fix, and we want it to be in 17.07, perhaps we should carry to make sure it gets merged in time

@thaJeztah

thaJeztah Jul 5, 2017

Member

@aaronlehmann @vieux if this is an important fix, and we want it to be in 17.07, perhaps we should carry to make sure it gets merged in time

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jul 6, 2017

Contributor

This LGTM as-is, however I suggested a simplification for the existing code. The simplification could happen in a separate PR.

Contributor

aaronlehmann commented Jul 6, 2017

This LGTM as-is, however I suggested a simplification for the existing code. The simplification could happen in a separate PR.

@aaronlehmann aaronlehmann merged commit 3be2273 into moby:master Jul 6, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35234 has succeeded
Details
janky Jenkins build Docker-PRs 43841 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4208 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15175 has succeeded
Details
z Jenkins build Docker-PRs-s390x 3936 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment