Remove QEMUFile abuse

PATCH Superseded Juan Quintela 3 participants 10 messages Sep 13, 2011 – Sep 20, 2011

lore

Share & tools

lore

Juan Quintela

author

2011-09-13 12:53 UTC #2 lore raw

PATCH 2/3 wavcapture: port to FILE*

In reply to 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 <[email protected]>

---
 audio/wavcapture.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)

audio/wavcapture.c +25 ~~-13~~

index c64f0ef..ecdb9ec 100644
@@ -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;
 }

Signature · 1 line

1.7.6

Juan Quintela

author

2011-09-13 12:53 UTC #0 lore raw

cover Remove QEMUFile abuse

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

Signature · 1 line

1.7.6

Juan Quintela

author

2011-09-13 12:53 UTC #1 lore raw

PATCH 1/3 vawaudio: port to FILE*

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 <[email protected]>

---
 audio/wavaudio.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

audio/wavaudio.c +19 -9

index aed1817..837b86d 100644
@@ -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);

Signature · 1 line

1.7.6

Juan Quintela

author

2011-09-13 12:53 UTC #3 lore raw

PATCH 3/3 ds1225y: port to FILE*

In reply to 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 <[email protected]>

---
 hw/ds1225y.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

hw/ds1225y.c +16 ~~-12~~

index 9875c44..cd23668 100644
@@ -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);

Signature · 1 line

1.7.6

Anthony Liguori

Reviewed-by on the v1 series

2011-09-16 13:12 UTC #4 lore raw

In reply to Juan Quintela

Reviewed-by Anthony Liguori <[email protected]>

malc, please Ack.

Regards,

Anthony Liguori

Quoted 21 lines · 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(-)

malc

2011-09-16 22:52 UTC #5 lore raw

Quoted 4 lines · Anthony Liguori

Reviewed-by Anthony Liguori <[email protected]>
malc, please Ack.

I don't like the commit message.

Quoted 25 lines · Juan Quintela

Quoted 21 lines
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(-)

Signature · 1 line

mailto:[email protected]

Juan Quintela

author

2011-09-18 20:08 UTC #6 lore raw

Quoted 8 lines · malc

Quoted 4 lines · Anthony Liguori
Reviewed-by Anthony Liguori <[email protected]>
malc, please Ack.
I don't like the commit message.

Can you be more specific?

Can you say what you will preffer?

Quoted 23 lines

Quoted 23 lines · Juan Quintela
Quoted 21 lines
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(-)

malc

2011-09-18 20:30 UTC #7 lore raw

Quoted 11 lines · Juan Quintela

Quoted 8 lines · malc
Quoted 4 lines · Anthony Liguori
Reviewed-by Anthony Liguori <[email protected]>
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).

Quoted 3 lines

Can you say what you will preffer?

"Use stdio instead of QEMUFile"

Quoted 25 lines

Quoted 24 lines
Quoted 24 lines · Juan Quintela
Quoted 21 lines
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(-)

Signature · 1 line

mailto:[email protected]

Juan Quintela

author

2011-09-19 21:03 UTC #8 lore raw

Quoted 10 lines · malc

Quoted 8 lines · Juan Quintela
Quoted 7 lines · malc
Quoted 4 lines · Anthony Liguori
Reviewed-by Anthony Liguori <[email protected]>
malc, please Ack.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
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.

Quoted 2 lines

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

Fixed.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 3 lines
Can you say what you will preffer?
"Use stdio instead of QEMUFile"

Done.

Thanks for the review, Juan.

malc

2011-09-20 05:53 UTC #9 lore raw

Quoted 24 lines · Juan Quintela

Quoted 10 lines · malc
Quoted 8 lines · Juan Quintela
Quoted 7 lines · malc
Quoted 4 lines · Anthony Liguori
Reviewed-by Anthony Liguori <[email protected]>
malc, please Ack.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 2 lines
There's no such thing
as "vawaudio" (i.e. v vs w).

Quoted 7 lines

Fixed.
Quoted 1 line
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)

Quoted 12 lines

Quoted 2 lines
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).

Quoted 4 lines

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

Quoted 38 lines

Quoted 4 lines
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[];

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 12 lines

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

Signature · 1 line

mailto:[email protected]