| Message ID | 1455246507-5589-6-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: > While we are operating here we may as well fix some of the > file descriptor leaks. I'm not convinced. The added goto-s make the code uglier to read, and this being a standalone utility there's not really any leak here. Jan
Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths."):
> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
> > While we are operating here we may as well fix some of the
> > file descriptor leaks.
>
> I'm not convinced. The added goto-s make the code uglier to read,
> and this being a standalone utility there's not really any leak here.
I don't buy this `uglier to read'. What `return 1' does is make me
think `is some resource being leaked' and `do I need to audit this
thing'.
Ian.
>>> On 18.02.16 at 17:39, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors > in the error paths."): >> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: >> > While we are operating here we may as well fix some of the >> > file descriptor leaks. >> >> I'm not convinced. The added goto-s make the code uglier to read, >> and this being a standalone utility there's not really any leak here. > > I don't buy this `uglier to read'. What `return 1' does is make me > think `is some resource being leaked' and `do I need to audit this > thing'. Certainly a matter of taste to some degree - goto-s are always ugly to read to my eyes. Irrespective of this I don't buy the leak aspect for a non-library like, short running build utility. The close() calls are just more code, with absolutely no added benefit to the system the code runs on. Jan
On Thu, Feb 18, 2016 at 09:45:30AM -0700, Jan Beulich wrote: > >>> On 18.02.16 at 17:39, <Ian.Jackson@eu.citrix.com> wrote: > > Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors > > in the error paths."): > >> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: > >> > While we are operating here we may as well fix some of the > >> > file descriptor leaks. > >> > >> I'm not convinced. The added goto-s make the code uglier to read, > >> and this being a standalone utility there's not really any leak here. > > > > I don't buy this `uglier to read'. What `return 1' does is make me > > think `is some resource being leaked' and `do I need to audit this > > thing'. > > Certainly a matter of taste to some degree - goto-s are always > ugly to read to my eyes. Irrespective of this I don't buy the leak > aspect for a non-library like, short running build utility. The close() > calls are just more code, with absolutely no added benefit to the > system the code runs on. <chuckles> If I turned them in: if (..blah..) { close(infd); return -1; } would that satisfy you? (Irrespective of the 'no added benefit to the system the code runs on.'). > > Jan >
>>> On 18.02.16 at 22:12, <konrad.wilk@oracle.com> wrote: > On Thu, Feb 18, 2016 at 09:45:30AM -0700, Jan Beulich wrote: >> >>> On 18.02.16 at 17:39, <Ian.Jackson@eu.citrix.com> wrote: >> > Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file > descriptors >> > in the error paths."): >> >> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote: >> >> > While we are operating here we may as well fix some of the >> >> > file descriptor leaks. >> >> >> >> I'm not convinced. The added goto-s make the code uglier to read, >> >> and this being a standalone utility there's not really any leak here. >> > >> > I don't buy this `uglier to read'. What `return 1' does is make me >> > think `is some resource being leaked' and `do I need to audit this >> > thing'. >> >> Certainly a matter of taste to some degree - goto-s are always >> ugly to read to my eyes. Irrespective of this I don't buy the leak >> aspect for a non-library like, short running build utility. The close() >> calls are just more code, with absolutely no added benefit to the >> system the code runs on. > > <chuckles> > > If I turned them in: > > if (..blah..) > { > close(infd); > return -1; > } > > would that satisfy you? Since presumably this would be on a number of error paths, I'm afraid ... > (Irrespective of the 'no added benefit to the system the code runs > on.'). ... this aspect would still be an relevant one. Jan
Konrad Rzeszutek Wilk writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths."):
> <chuckles>
>
> If I turned them in:
>
> if (..blah..)
> {
> close(infd);
> return -1;
> }
>
> would that satisfy you?
I would strongly resist that. That coding style is an error handling
antipattern.
Ian.
diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c index 993a7ee..890ae6d 100644 --- a/xen/arch/x86/boot/mkelf32.c +++ b/xen/arch/x86/boot/mkelf32.c @@ -230,9 +230,9 @@ int main(int argc, char **argv) u64 final_exec_addr; u32 loadbase, dat_siz, mem_siz; char *inimage, *outimage; - int infd, outfd; + int infd, outfd = -1; char buffer[1024]; - int bytes, todo, i; + int bytes, todo, i, rc = 1; Elf32_Ehdr in32_ehdr; @@ -256,7 +256,7 @@ int main(int argc, char **argv) { fprintf(stderr, "Failed to open input image '%s': %d (%s).\n", inimage, errno, strerror(errno)); - return 1; + goto out; } do_read(infd, &in32_ehdr, sizeof(in32_ehdr)); @@ -264,7 +264,7 @@ int main(int argc, char **argv) (in32_ehdr.e_ident[EI_DATA] != ELFDATA2LSB) ) { fprintf(stderr, "Input image must be a little-endian Elf image.\n"); - return 1; + goto out; } big_endian = (*(u16 *)in32_ehdr.e_ident == ((ELFMAG0 << 8) | ELFMAG1)); @@ -273,7 +273,7 @@ int main(int argc, char **argv) if ( in32_ehdr.e_ident[EI_CLASS] != ELFCLASS64 ) { fprintf(stderr, "Bad program header class - we only do 64-bit!.\n"); - return 1; + goto out; } (void)lseek(infd, 0, SEEK_SET); do_read(infd, &in64_ehdr, sizeof(in64_ehdr)); @@ -283,14 +283,14 @@ int main(int argc, char **argv) { fprintf(stderr, "Bad program header size (%d != %d).\n", (int)in64_ehdr.e_phentsize, (int)sizeof(in64_phdr)); - return 1; + goto out; } if ( in64_ehdr.e_phnum != 1 ) { fprintf(stderr, "Expect precisly 1 program header; found %d.\n", (int)in64_ehdr.e_phnum); - return 1; + goto out; } (void)lseek(infd, in64_ehdr.e_phoff, SEEK_SET); @@ -327,7 +327,7 @@ int main(int argc, char **argv) { fprintf(stderr, "Failed to open output image '%s': %d (%s).\n", outimage, errno, strerror(errno)); - return 1; + goto out; } endianadjust_ehdr32(&out_ehdr); @@ -339,7 +339,7 @@ int main(int argc, char **argv) if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr)) < 0 ) { fprintf(stderr, "Header overflow.\n"); - return 1; + goto out; } do_write(outfd, buffer, bytes); @@ -358,10 +358,14 @@ int main(int argc, char **argv) do_write(outfd, out_shstrtab, sizeof(out_shstrtab)); do_write(outfd, buffer, 4-((sizeof(out_shstrtab)+dat_siz)&3)); - close(infd); - close(outfd); + rc = 0; +out: + if ( infd != -1 ) + close(infd); + if ( outfd != -1 ) + close(outfd); - return 0; + return rc; } /*
While we are operating here we may as well fix some of the file descriptor leaks. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/x86/boot/mkelf32.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)