Re: [Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [PATCH v3 5/5] mkelf32: Close those file 
descriptors in the error paths."):
> 
> 
> 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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Jan Beulich]

>>> On 18.02.16 at 22:12,  wrote:
> On Thu, Feb 18, 2016 at 09:45:30AM -0700, Jan Beulich wrote:
>> >>> On 18.02.16 at 17:39,  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,  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.
> 
> 
> 
> 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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Konrad Rzeszutek Wilk]

On Thu, Feb 18, 2016 at 09:45:30AM -0700, Jan Beulich wrote:
> >>> On 18.02.16 at 17:39,  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,  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.



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
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Jan Beulich]

>>> On 18.02.16 at 17:39,  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,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors 
in the error paths."):
> On 12.02.16 at 04:08,  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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Jan Beulich]

>>> On 12.02.16 at 04:08,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Konrad Rzeszutek Wilk]

While we are operating here we may as well fix some of the
file descriptor leaks.

Signed-off-by: Konrad Rzeszutek Wilk 
---
 xen/arch/x86/boot/mkelf32.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

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)
 u64final_exec_addr;
 u32loadbase, dat_siz, mem_siz;
 char  *inimage, *outimage;
-intinfd, outfd;
+intinfd, outfd = -1;
 char   buffer[1024];
-intbytes, todo, i;
+intbytes, 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, _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, _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(_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;
 }
 
 /*
-- 
2.4.3


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel