[Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[Juan Quintela]

Hi

QEMUFile is intended to be used only for migration.  Change the other
three users to use FILE * operations directly.  gcc on Fedora 15
complains about fread/write not checking its return value, so I added
checks.  But in several places only print an error message (there is
no error handly that I can hook into).  Notice that this is not worse
than it is today.

Later, Juan.

Juan Quintela (3):
  vawaudio: port to FILE*
  wavcapture: port to FILE*
  ds1225y: port to FILE*

 audio/wavaudio.c   |   28 +++++++++++++++++++---------
 audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
 hw/ds1225y.c       |   28 ++++++++++++++++------------
 3 files changed, 60 insertions(+), 34 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/3] vawaudio: port to FILE*

[Juan Quintela]

QEMUFile * is only intended for Migration.  Using it for anything else
just adds pain and a layer of buffers for no good reason.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 audio/wavaudio.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/audio/wavaudio.c b/audio/wavaudio.c
index aed1817..837b86d 100644
--- a/audio/wavaudio.c
+++ b/audio/wavaudio.c
@@ -30,7 +30,7 @@

 typedef struct WAVVoiceOut {
     HWVoiceOut hw;
-    QEMUFile *f;
+    FILE *f;
     int64_t old_ticks;
     void *pcm_buf;
     int total_samples;
@@ -76,7 +76,10 @@ static int wav_run_out (HWVoiceOut *hw, int live)
         dst = advance (wav->pcm_buf, rpos << hw->info.shift);

         hw->clip (dst, src, convert_samples);
-        qemu_put_buffer (wav->f, dst, convert_samples << hw->info.shift);
+        if (fwrite (dst, convert_samples << hw->info.shift, 1, wav->f) !=
+            convert_samples << hw->info.shift) {
+            printf("wav_run_out: short write\n");
+        }

         rpos = (rpos + convert_samples) % hw->samples;
         samples -= convert_samples;
@@ -152,7 +155,7 @@ static int wav_init_out (HWVoiceOut *hw, struct audsettings *as)
     le_store (hdr + 28, hw->info.freq << (bits16 + stereo), 4);
     le_store (hdr + 32, 1 << (bits16 + stereo), 2);

-    wav->f = qemu_fopen (conf.wav_path, "wb");
+    wav->f = fopen (conf.wav_path, "wb");
     if (!wav->f) {
         dolog ("Failed to open wave file `%s'\nReason: %s\n",
                conf.wav_path, strerror (errno));
@@ -161,7 +164,10 @@ static int wav_init_out (HWVoiceOut *hw, struct audsettings *as)
         return -1;
     }

-    qemu_put_buffer (wav->f, hdr, sizeof (hdr));
+    if (fwrite (hdr, sizeof (hdr), 1, wav->f) != sizeof (hdr)) {
+        printf("wav_init_out: short write\n");
+        return -1;
+    }
     return 0;
 }

@@ -180,13 +186,17 @@ static void wav_fini_out (HWVoiceOut *hw)
     le_store (rlen, rifflen, 4);
     le_store (dlen, datalen, 4);

-    qemu_fseek (wav->f, 4, SEEK_SET);
-    qemu_put_buffer (wav->f, rlen, 4);
+    fseek (wav->f, 4, SEEK_SET);
+    if (fwrite (rlen, 1, 4, wav->f) != 4) {
+        printf("wav_fini_out: short write\n");
+    }

-    qemu_fseek (wav->f, 32, SEEK_CUR);
-    qemu_put_buffer (wav->f, dlen, 4);
+    fseek (wav->f, 32, SEEK_CUR);
+    if (fwrite (dlen, 1, 4, wav->f) != 4) {
+        printf("wav_fini_out: short write\n");
+    }

-    qemu_fclose (wav->f);
+    fclose (wav->f);
     wav->f = NULL;

     g_free (wav->pcm_buf);
-- 
1.7.6

[Qemu-devel] [PATCH 2/3] wavcapture: port to FILE*

[Juan Quintela]

QEMUFile * is only intended for Migration.  Using it for anything else
just adds pain and a layer of buffers for no good reason.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index c64f0ef..ecdb9ec 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -3,7 +3,7 @@
 #include "audio.h"

 typedef struct {
-    QEMUFile *f;
+    FILE *f;
     int bytes;
     char *path;
     int freq;
@@ -40,12 +40,16 @@ static void wav_destroy (void *opaque)
         le_store (rlen, rifflen, 4);
         le_store (dlen, datalen, 4);

-        qemu_fseek (wav->f, 4, SEEK_SET);
-        qemu_put_buffer (wav->f, rlen, 4);
+        fseek (wav->f, 4, SEEK_SET);
+        if (fwrite (rlen, 1, 4, wav->f) != 4) {
+            printf("wav_destroy: short write\n");
+        }

-        qemu_fseek (wav->f, 32, SEEK_CUR);
-        qemu_put_buffer (wav->f, dlen, 4);
-        qemu_fclose (wav->f);
+        fseek (wav->f, 32, SEEK_CUR);
+        if (fwrite (dlen, 1, 4, wav->f) != 4) {
+            printf("wav_destroy: short write\n");
+        }
+        fclose (wav->f);
     }

     g_free (wav->path);
@@ -55,7 +59,9 @@ static void wav_capture (void *opaque, void *buf, int size)
 {
     WAVState *wav = opaque;

-    qemu_put_buffer (wav->f, buf, size);
+    if (fwrite (buf, size, 1, wav->f) != size) {
+        printf("wav_capture: short write\n");
+    }
     wav->bytes += size;
 }

@@ -130,7 +136,7 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
     le_store (hdr + 28, freq << shift, 4);
     le_store (hdr + 32, 1 << shift, 2);

-    wav->f = qemu_fopen (path, "wb");
+    wav->f = fopen (path, "wb");
     if (!wav->f) {
         monitor_printf(mon, "Failed to open wave file `%s'\nReason: %s\n",
                        path, strerror (errno));
@@ -143,19 +149,25 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
     wav->nchannels = nchannels;
     wav->freq = freq;

-    qemu_put_buffer (wav->f, hdr, sizeof (hdr));
+    if (fwrite (hdr, sizeof (hdr), 1, wav->f) != sizeof (hdr)) {
+        monitor_printf(mon, "wav_start_capture: short write\n");
+        goto error_free;
+    }

     cap = AUD_add_capture (&as, &ops, wav);
     if (!cap) {
         monitor_printf(mon, "Failed to add audio capture\n");
-        g_free (wav->path);
-        qemu_fclose (wav->f);
-        g_free (wav);
-        return -1;
+        goto error_free;
     }

     wav->cap = cap;
     s->opaque = wav;
     s->ops = wav_capture_ops;
     return 0;
+
+error_free:
+    g_free (wav->path);
+    fclose (wav->f);
+    g_free (wav);
+    return -1;
 }
-- 
1.7.6

[Qemu-devel] [PATCH 3/3] ds1225y: port to FILE*

[Juan Quintela]

QEMUFile * is only intended for Migration.  Using it for anything else
just adds pain and a layer of buffers for no good reason.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ds1225y.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/ds1225y.c b/hw/ds1225y.c
index 9875c44..cd23668 100644
--- a/hw/ds1225y.c
+++ b/hw/ds1225y.c
@@ -29,7 +29,7 @@ typedef struct {
     DeviceState qdev;
     uint32_t chip_size;
     char *filename;
-    QEMUFile *file;
+    FILE *file;
     uint8_t *contents;
 } NvRamState;

@@ -70,9 +70,9 @@ static void nvram_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)

     s->contents[addr] = val;
     if (s->file) {
-        qemu_fseek(s->file, addr, SEEK_SET);
-        qemu_put_byte(s->file, (int)val);
-        qemu_fflush(s->file);
+        fseek(s->file, addr, SEEK_SET);
+        fputc(val, s->file);
+        fflush(s->file);
     }
 }

@@ -108,15 +108,17 @@ static int nvram_post_load(void *opaque, int version_id)

     /* Close file, as filename may has changed in load/store process */
     if (s->file) {
-        qemu_fclose(s->file);
+        fclose(s->file);
     }

     /* Write back nvram contents */
-    s->file = qemu_fopen(s->filename, "wb");
+    s->file = fopen(s->filename, "wb");
     if (s->file) {
         /* Write back contents, as 'wb' mode cleaned the file */
-        qemu_put_buffer(s->file, s->contents, s->chip_size);
-        qemu_fflush(s->file);
+        if (fwrite(s->contents, s->chip_size, 1, s->file) != s->chip_size) {
+            printf("nvram_post_load: short write\n");
+        }
+        fflush(s->file);
     }

     return 0;
@@ -143,7 +145,7 @@ typedef struct {
 static int nvram_sysbus_initfn(SysBusDevice *dev)
 {
     NvRamState *s = &FROM_SYSBUS(SysBusNvRamState, dev)->nvram;
-    QEMUFile *file;
+    FILE *file;
     int s_io;

     s->contents = g_malloc0(s->chip_size);
@@ -153,11 +155,13 @@ static int nvram_sysbus_initfn(SysBusDevice *dev)
     sysbus_init_mmio(dev, s->chip_size, s_io);

     /* Read current file */
-    file = qemu_fopen(s->filename, "rb");
+    file = fopen(s->filename, "rb");
     if (file) {
         /* Read nvram contents */
-        qemu_get_buffer(file, s->contents, s->chip_size);
-        qemu_fclose(file);
+        if (fread(s->contents, s->chip_size, 1, file) != s->chip_size) {
+            printf("nvram_sysbus_initfn: short read\n");
+        }
+        fclose(file);
     }
     nvram_post_load(s, 0);

-- 
1.7.6

Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[Anthony Liguori]

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

malc, please Ack.

Regards,

Anthony Liguori


On 09/13/2011 07:52 AM, Juan Quintela wrote:
> Hi
>
> QEMUFile is intended to be used only for migration.  Change the other
> three users to use FILE * operations directly.  gcc on Fedora 15
> complains about fread/write not checking its return value, so I added
> checks.  But in several places only print an error message (there is
> no error handly that I can hook into).  Notice that this is not worse
> than it is today.
>
> Later, Juan.
>
> Juan Quintela (3):
>    vawaudio: port to FILE*
>    wavcapture: port to FILE*
>    ds1225y: port to FILE*
>
>   audio/wavaudio.c   |   28 +++++++++++++++++++---------
>   audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
>   hw/ds1225y.c       |   28 ++++++++++++++++------------
>   3 files changed, 60 insertions(+), 34 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[malc]

On Fri, 16 Sep 2011, Anthony Liguori wrote:

> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> malc, please Ack.
> 

I don't like the commit message.

> 
> 
> On 09/13/2011 07:52 AM, Juan Quintela wrote:
> > Hi
> > 
> > QEMUFile is intended to be used only for migration.  Change the other
> > three users to use FILE * operations directly.  gcc on Fedora 15
> > complains about fread/write not checking its return value, so I added
> > checks.  But in several places only print an error message (there is
> > no error handly that I can hook into).  Notice that this is not worse
> > than it is today.
> > 
> > Later, Juan.
> > 
> > Juan Quintela (3):
> >    vawaudio: port to FILE*
> >    wavcapture: port to FILE*
> >    ds1225y: port to FILE*
> > 
> >   audio/wavaudio.c   |   28 +++++++++++++++++++---------
> >   audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
> >   hw/ds1225y.c       |   28 ++++++++++++++++------------
> >   3 files changed, 60 insertions(+), 34 deletions(-)
> > 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[Juan Quintela]

malc <av1474@comtv.ru> wrote:
> On Fri, 16 Sep 2011, Anthony Liguori wrote:
>
>> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>> 
>> malc, please Ack.
>> 
>
> I don't like the commit message.

Can you be more specific?

Can you say what you will preffer?


>> On 09/13/2011 07:52 AM, Juan Quintela wrote:
>> > Hi
>> > 
>> > QEMUFile is intended to be used only for migration.  Change the other
>> > three users to use FILE * operations directly.  gcc on Fedora 15
>> > complains about fread/write not checking its return value, so I added
>> > checks.  But in several places only print an error message (there is
>> > no error handly that I can hook into).  Notice that this is not worse
>> > than it is today.
>> > 
>> > Later, Juan.
>> > 
>> > Juan Quintela (3):
>> >    vawaudio: port to FILE*
>> >    wavcapture: port to FILE*
>> >    ds1225y: port to FILE*
>> > 
>> >   audio/wavaudio.c   |   28 +++++++++++++++++++---------
>> >   audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
>> >   hw/ds1225y.c       |   28 ++++++++++++++++------------
>> >   3 files changed, 60 insertions(+), 34 deletions(-)
>> > 
>> 

Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[malc]

On Sun, 18 Sep 2011, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Fri, 16 Sep 2011, Anthony Liguori wrote:
> >
> >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> >> 
> >> malc, please Ack.
> >> 
> >
> > I don't like the commit message.
> 
> Can you be more specific?

QEMUFile predates migration by a few years so could have never been
inteneded to be used for it (leave alone only). There's no such thing
as "vawaudio" (i.e. v vs w).

Commentary aside: fcalls (seek/tell/read/close) can fail and the code 
in the patch doesn't handle it, error path for fwrite does not supply 
information on why the call has failed and furthermore does it via printf, 
also, i believe i mentioned this once before, fwrite (p, 1, n, f) should
really be (p, n, 1, f).

> 
> Can you say what you will preffer?
> 

"Use stdio instead of QEMUFile"

> 
> >> On 09/13/2011 07:52 AM, Juan Quintela wrote:
> >> > Hi
> >> > 
> >> > QEMUFile is intended to be used only for migration.  Change the other
> >> > three users to use FILE * operations directly.  gcc on Fedora 15
> >> > complains about fread/write not checking its return value, so I added
> >> > checks.  But in several places only print an error message (there is
> >> > no error handly that I can hook into).  Notice that this is not worse
> >> > than it is today.
> >> > 
> >> > Later, Juan.
> >> > 
> >> > Juan Quintela (3):
> >> >    vawaudio: port to FILE*
> >> >    wavcapture: port to FILE*
> >> >    ds1225y: port to FILE*
> >> > 
> >> >   audio/wavaudio.c   |   28 +++++++++++++++++++---------
> >> >   audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
> >> >   hw/ds1225y.c       |   28 ++++++++++++++++------------
> >> >   3 files changed, 60 insertions(+), 34 deletions(-)
> >> > 
> >> 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[Juan Quintela]

malc <av1474@comtv.ru> wrote:
> On Sun, 18 Sep 2011, Juan Quintela wrote:
>
>> malc <av1474@comtv.ru> wrote:
>> > On Fri, 16 Sep 2011, Anthony Liguori wrote:
>> >
>> >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>> >> 
>> >> malc, please Ack.
>> >> 
>> >
>> > I don't like the commit message.
>> 
>> Can you be more specific?
>
> QEMUFile predates migration by a few years so could have never been
> inteneded to be used for it (leave alone only).

See, I feel young again O:-).  Nowadays it is true, though.
Improved comment.

>  There's no such thing
> as "vawaudio" (i.e. v vs w).

Fixed.

> Commentary aside: fcalls (seek/tell/read/close) can fail and the code 

It is the same code that it is today.  There were no handling of errors
before, and adding that means changing infrastructure.

> in the patch doesn't handle it, error path for fwrite does not supply 
> information on why the call has failed

It prints: "name_of_function: short write"

Man page on my fedora linux puts:

fread()  and  fwrite()  return the number of items successfully read or
       written (i.e., not the number of characters).  If an error  occurs,  or
       the  end-of-file is reached, the return value is a short item count (or
       zero).

Not a lot that I can do :-(
Several of the functions return void, so there is not easy to add error
handling.  In the functions that handle errors, error code was added and
handled.

>  and furthermore does it via printf, 
> also, i believe i mentioned this once before, fwrite (p, 1, n, f) should
> really be (p, n, 1, f).

I don't care one way or another.  But qemu source code uses it the other
way around.

[ Removed the ones that I introduced on my patch ]

../hw/vga.c:2370:        ret = fwrite(linebuf, 1, pbuf - linebuf, f);

char *linebuf;

../qga/guest-agent-commands.c:259:    write_count = fwrite(buf, 1, count, fh);

guchar *buf;

../trace/simple.c:134:            unused = fwrite(&record, sizeof(record), 1, trace_fp);

TraceRecord record;

../trace/simple.c:141:            unused = fwrite(&record, sizeof(record), 1, trace_fp);

same

../trace/simple.c:239:        if (fwrite(&header, sizeof header, 1, trace_fp) != 1) {

TraceRecord header;

../monitor.c:1609:        if (fwrite(buf, 1, l, f) != l) {

uint8_t buf[];

../monitor.c:1645:        if (fwrite(buf, 1, l, f) != l) {

uint8_t buf[];

../savevm.c:216:    return fwrite(buf, 1, size, s->stdio_file);

uint8_t *buf;

../savevm.c:340:    return fwrite(buf, 1, size, s->stdio_file);

same.

BTW, what is the reason that "size of element" for a char string is not
1, and number of elements is not number of elements of array?  I have to
admit that I have always used fwrite/read the other way around that you
suggest.


>> 
>> Can you say what you will preffer?
>> 
>
> "Use stdio instead of QEMUFile"

Done.

Thanks for the review, Juan.

Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[malc]

On Mon, 19 Sep 2011, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Sun, 18 Sep 2011, Juan Quintela wrote:
> >
> >> malc <av1474@comtv.ru> wrote:
> >> > On Fri, 16 Sep 2011, Anthony Liguori wrote:
> >> >
> >> >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> >> >> 
> >> >> malc, please Ack.
> >> >> 
> >> >
> >> > I don't like the commit message.
> >> 
> >> Can you be more specific?
> >
> > QEMUFile predates migration by a few years so could have never been
> > inteneded to be used for it (leave alone only).
> 
> See, I feel young again O:-).  Nowadays it is true, though.
> Improved comment.
> 
> >  There's no such thing
> > as "vawaudio" (i.e. v vs w).
> 
> Fixed.
> 
> > Commentary aside: fcalls (seek/tell/read/close) can fail and the code 
> 
> It is the same code that it is today.  There were no handling of errors
> before, and adding that means changing infrastructure.
> 

The code was using QEMUFile abstraction which didn't have an option to
return errors, stdio does, so it must be used (look at how file open
was handled before, it could fail and when it does old code tries to
notify the user, new code for all replaced QEMUFile function must
follow suit)

> > in the patch doesn't handle it, error path for fwrite does not supply 
> > information on why the call has failed
> 
> It prints: "name_of_function: short write"
> 
> Man page on my fedora linux puts:
> 
> fread()  and  fwrite()  return the number of items successfully read or
>        written (i.e., not the number of characters).  If an error  occurs,  or
>        the  end-of-file is reached, the return value is a short item count (or
>        zero).
> 
> Not a lot that I can do :-(
> Several of the functions return void, so there is not easy to add error
> handling.  In the functions that handle errors, error code was added and
> handled.

errno

> 
> >  and furthermore does it via printf, 
> > also, i believe i mentioned this once before, fwrite (p, 1, n, f) should
> > really be (p, n, 1, f).
> 
> I don't care one way or another.  But qemu source code uses it the other
> way around.
> 
> [ Removed the ones that I introduced on my patch ]
> 
> ../hw/vga.c:2370:        ret = fwrite(linebuf, 1, pbuf - linebuf, f);
> 
> char *linebuf;
> 
> ../qga/guest-agent-commands.c:259:    write_count = fwrite(buf, 1, count, fh);
> 
> guchar *buf;
> 
> ../trace/simple.c:134:            unused = fwrite(&record, sizeof(record), 1, trace_fp);
> 
> TraceRecord record;
> 
> ../trace/simple.c:141:            unused = fwrite(&record, sizeof(record), 1, trace_fp);
> 
> same
> 
> ../trace/simple.c:239:        if (fwrite(&header, sizeof header, 1, trace_fp) != 1) {
> 
> TraceRecord header;
> 
> ../monitor.c:1609:        if (fwrite(buf, 1, l, f) != l) {
> 
> uint8_t buf[];
> 
> ../monitor.c:1645:        if (fwrite(buf, 1, l, f) != l) {
> 
> uint8_t buf[];
> 
> ../savevm.c:216:    return fwrite(buf, 1, size, s->stdio_file);
> 
> uint8_t *buf;
> 
> ../savevm.c:340:    return fwrite(buf, 1, size, s->stdio_file);
> 
> same.

And i don't much care what the code i do not maintain/wrote does things
should be done properly.

> 
> BTW, what is the reason that "size of element" for a char string is not
> 1, and number of elements is not number of elements of array?  I have to
> admit that I have always used fwrite/read the other way around that you
> suggest.

For one thing it greatly simplifies error checking, i.e. you _always_
compare the returned value with 1 instead of some random parameters.

> 
> 
> >> 
> >> Can you say what you will preffer?
> >> 
> >
> > "Use stdio instead of QEMUFile"
> 
> Done.
> 
> Thanks for the review, Juan.
> 

-- 
mailto:av1474@comtv.ru