| Message ID | 1452603559-13685-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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 :)
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)
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(-)