Re: [Qemu-devel] [PATCH v2] s390: use FILE instead of QEMUFile for creating text file

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Jan 15, 2016 at 04:29:32PM -0700, Eric Blake wrote:
> On 01/12/2016 05:59 AM, Daniel P. Berrange wrote:
> > The s390 skeys monitor command needs to write out a plain text
> > file. Currently it is using the QEMUFile class for this, but
> > work is ongoing to refactor QEMUFile and eliminate much code
> > related to it. The only feature qemu_fopen() gives over fopen()
> > is support for QEMU FD passing, but this can be achieved with
> > qemu_open() + fdopen() too. Switching to regular stdio FILE
> > APIs avoids the need to sprintf via an intermedia buffer which
> 
> s/intermedia/intermediate/
> 
> > slightly simplifies the code.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  hw/s390x/s390-skeys.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> 
> > @@ -124,8 +120,14 @@ void qmp_dump_skeys(const char *filename, Error **errp)
> >          return;
> >      }
> >  
> > -    f = qemu_fopen(filename, "wb");
> > +    fd = qemu_open(filename, O_WRONLY|O_CREAT|O_TRUNC, 0600);
> 
> A strict conversion should probably include O_BINARY for mingw (where
> "wb" turns on binary mode).  But maybe we should just make qemu_open()
> itself _always_ provide O_BINARY so that callers don't have to worry
> about it - do we really have a reason to open a file on mingw where we
> want \r\n munged into \n due to text mode?

We're writing text data to the file, so I figured that using binary
mode would be wrong.

> 
> > +    if (fd < 0) {
> > +        error_setg_file_open(errp, errno, filename);
> > +        return;
> > +    }
> > +    f = fdopen(fd, "wb");
> >      if (!f) {
> > +        close(fd);
> >          error_setg_file_open(errp, errno, filename);
> 
> close() may corrupt errno, resulting in a report of the wrong message.
> Swap these two lines.

Ok

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|