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

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

[Daniel P. Berrange]

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
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(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 539ef6d..35758fc 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -45,15 +45,11 @@ void s390_skeys_init(void)
     qdev_init_nofail(DEVICE(obj));
 }
 
-static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
+static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
                        uint64_t count, Error **errp)
 {
     uint64_t curpage = startgfn;
     uint64_t maxpage = curpage + count - 1;
-    const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
-                      " ch=%d, reserved=%d\n";
-    char buf[128];
-    int len;
 
     for (; curpage <= maxpage; curpage++) {
         uint8_t acc = (*keys & 0xF0) >> 4;
@@ -62,10 +58,9 @@ static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
         int ch = (*keys & 0x02);
         int res = (*keys & 0x01);
 
-        len = snprintf(buf, sizeof(buf), fmt, curpage,
-                       *keys, acc, fp, ref, ch, res);
-        assert(len < sizeof(buf));
-        qemu_put_buffer(f, (uint8_t *)buf, len);
+        fprintf(f, "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
+                " ch=%d, reserved=%d\n",
+                curpage, *keys, acc, fp, ref, ch, res);
         keys++;
     }
 }
@@ -115,7 +110,8 @@ void qmp_dump_skeys(const char *filename, Error **errp)
     vaddr cur_gfn = 0;
     uint8_t *buf;
     int ret;
-    QEMUFile *f;
+    int fd;
+    FILE *f;
 
     /* Quick check to see if guest is using storage keys*/
     if (!skeyclass->skeys_enabled(ss)) {
@@ -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);
+    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);
         return;
     }
@@ -161,7 +163,7 @@ out_free:
     error_propagate(errp, lerr);
     g_free(buf);
 out:
-    qemu_fclose(f);
+    fclose(f);
 }
 
 static void qemu_s390_skeys_init(Object *obj)
-- 
2.5.0

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

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?

> +    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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Daniel P. Berrange]

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 :|

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

[Cornelia Huck]

On Mon, 18 Jan 2016 09:50:32 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Fri, Jan 15, 2016 at 04:29:32PM -0700, Eric Blake wrote:
> > On 01/12/2016 05:59 AM, Daniel P. Berrange wrote:

> > > @@ -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

Daniel, if you send a respin, could you please put Jason on cc: as
well? He can test this quicker than I can :)