From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:56394)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1T734R-00058D-HB
	for qemu-devel@nongnu.org; Thu, 30 Aug 2012 07:38:00 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1T734N-0004dn-Dp
	for qemu-devel@nongnu.org; Thu, 30 Aug 2012 07:37:59 -0400
Received: from mx1.redhat.com ([209.132.183.28]:52867)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1T734N-0004dg-5k
	for qemu-devel@nongnu.org; Thu, 30 Aug 2012 07:37:55 -0400
Date: Thu, 30 Aug 2012 08:38:37 -0300
From: Luiz Capitulino <lcapitulino@redhat.com>
Message-ID: <20120830083837.2f550c0f@doriath.home>
In-Reply-To: <CAFEAcA-w_PX2nUKH4_vxST-6AzOKCE=hyoMAOacaZ4gZmNGqDw@mail.gmail.com>
References: <1346269986-13539-1-git-send-email-lcapitulino@redhat.com>
	<1346269986-13539-7-git-send-email-lcapitulino@redhat.com>
	<CAFEAcA-w_PX2nUKH4_vxST-6AzOKCE=hyoMAOacaZ4gZmNGqDw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 6/9] omap_lcdc: omap_ppm_save(): add error
 handling
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: alevy@redhat.com, qemu-devel@nongnu.org

On Wed, 29 Aug 2012 22:28:38 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 29 August 2012 20:53, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hw/omap_lcdc.c | 59 ++++++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 45 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> > index 3d6328f..e2ba108 100644
> > --- a/hw/omap_lcdc.c
> > +++ b/hw/omap_lcdc.c
> > @@ -224,18 +224,24 @@ static void omap_update_display(void *opaque)
> >      omap_lcd->invalidate = 0;
> >  }
> >
> > -static int omap_ppm_save(const char *filename, uint8_t *data,
> > -                    int w, int h, int linesize)
> > +static void omap_ppm_save(const char *filename, uint8_t *data,
> > +                    int w, int h, int linesize, Error **errp)
> >  {
> >      FILE *f;
> >      uint8_t *d, *d1;
> >      unsigned int v;
> > -    int y, x, bpp;
> > +    int ret, y, x, bpp;
> >
> >      f = fopen(filename, "wb");
> > -    if (!f)
> > -        return -1;
> > -    fprintf(f, "P6\n%d %d\n%d\n", w, h, 255);
> > +    if (!f) {
> > +        error_setg(errp, "failed to open file '%s': %s", filename,
> > +                   strerror(errno));
> > +        return;
> > +    }
> > +    ret = fprintf(f, "P6\n%d %d\n%d\n", w, h, 255);
> > +    if (ret < 0) {
> > +        goto write_err;
> > +    }
> 
> We don't use 'ret' in write_err, so why not just
>       if (fprintf(f....)  < 0) {
>           goto write_err;
>       }
> 
> here and similarly below and drop the variable altogether?

For clarity. This is probably a matter of taste, but I much more prefer
separate statements (vs. saving 4 bytes during the function call).