Patchwork [RFC,07/20] Introduce qemu_put_vector() and qemu_put_vector_prepare() to use put_vector() in QEMUFile.

login
register
mail settings
Submitter Yoshiaki Tamura
Date April 21, 2010, 5:57 a.m.
Message ID <1271829445-5328-8-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/50633/
State New
Headers show

Comments

Yoshiaki Tamura - April 21, 2010, 5:57 a.m.
For fool proof purpose, qemu_put_vector_parepare should be called
before qemu_put_vector.  Then, if qemu_put_* functions except this is
called after qemu_put_vector_prepare, program will abort().

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 hw/hw.h  |    2 ++
 savevm.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)
Anthony Liguori - April 22, 2010, 7:29 p.m.
On 04/21/2010 12:57 AM, Yoshiaki Tamura wrote:
> For fool proof purpose, qemu_put_vector_parepare should be called
> before qemu_put_vector.  Then, if qemu_put_* functions except this is
> called after qemu_put_vector_prepare, program will abort().
>
> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>    

I don't get it.  What's this protecting against?

Regards,

Anthony Liguori

> ---
>   hw/hw.h  |    2 ++
>   savevm.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 55 insertions(+), 0 deletions(-)
>
> diff --git a/hw/hw.h b/hw/hw.h
> index 921cf90..10e6dda 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -77,6 +77,8 @@ void qemu_fflush(QEMUFile *f);
>   int qemu_fclose(QEMUFile *f);
>   void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
>   void qemu_put_byte(QEMUFile *f, int v);
> +void qemu_put_vector(QEMUFile *f, QEMUIOVector *qiov);
> +void qemu_put_vector_prepare(QEMUFile *f);
>   void *qemu_realloc_buffer(QEMUFile *f, int size);
>   void qemu_clear_buffer(QEMUFile *f);
>
> diff --git a/savevm.c b/savevm.c
> index 944e788..22d928c 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -180,6 +180,7 @@ struct QEMUFile {
>       uint8_t *buf;
>
>       int has_error;
> +    int prepares_vector;
>   };
>
>   typedef struct QEMUFileStdio
> @@ -557,6 +558,58 @@ void qemu_put_byte(QEMUFile *f, int v)
>           qemu_fflush(f);
>   }
>
> +void qemu_put_vector(QEMUFile *f, QEMUIOVector *v)
> +{
> +    struct iovec *iov;
> +    int cnt;
> +    size_t bufsize;
> +    uint8_t *buf;
> +
> +    if (qemu_file_get_rate_limit(f) != 0) {
> +        fprintf(stderr,
> +                "Attempted to write vector while bandwidth limit is not zero.\n");
> +        abort();
> +    }
> +
> +    /* checks prepares vector.
> +     * For fool proof purpose, qemu_put_vector_parepare should be called
> +     * before qemu_put_vector.  Then, if qemu_put_* functions except this
> +     * is called after qemu_put_vector_prepare, program will abort().
> +     */
> +    if (!f->prepares_vector) {
> +        fprintf(stderr,
> +            "You should prepare with qemu_put_vector_prepare.\n");
> +        abort();
> +    } else if (f->prepares_vector&&  f->buf_index != 0) {
> +        fprintf(stderr, "Wrote data after qemu_put_vector_prepare.\n");
> +        abort();
> +    }
> +    f->prepares_vector = 0;
> +
> +    if (f->put_vector) {
> +        qemu_iovec_to_vector(v,&iov,&cnt);
> +        f->put_vector(f->opaque, iov, 0, cnt);
> +    } else {
> +        qemu_iovec_to_size(v,&bufsize);
> +        buf = qemu_malloc(bufsize + 1 /* for '\0' */);
> +        qemu_iovec_to_buffer(v, buf);
> +        qemu_put_buffer(f, buf, bufsize);
> +        qemu_free(buf);
> +    }
> +
> +}
> +
> +void qemu_put_vector_prepare(QEMUFile *f)
> +{
> +    if (f->prepares_vector) {
> +        /* prepare vector */
> +        fprintf(stderr, "Attempted to prepare vector twice\n");
> +        abort();
> +    }
> +    f->prepares_vector = 1;
> +    qemu_fflush(f);
> +}
> +
>   int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
>   {
>       int size, l;
>
Yoshiaki Tamura - April 23, 2010, 4:02 a.m.
Anthony Liguori wrote:
> On 04/21/2010 12:57 AM, Yoshiaki Tamura wrote:
>> For fool proof purpose, qemu_put_vector_parepare should be called
>> before qemu_put_vector. Then, if qemu_put_* functions except this is
>> called after qemu_put_vector_prepare, program will abort().
>>
>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>
> I don't get it. What's this protecting against?

This was introduced to prevent mixing the order of normal write and vector 
write, and flush QEMUFile buffer before handling vectors.
While qemu_put_buffer copies data to QEMUFile buffer, qemu_put_vector() will 
bypass that buffer.

It's just fool proof purpose for what we encountered at beginning, and if the 
user of qemu_put_vector() is careful enough, we can remove 
qemu_put_vectore_prepare().  While writing this message, I started to think that 
just calling qemu_fflush() in qemu_put_vector() would be enough...

>
> Regards,
>
> Anthony Liguori
>
>> ---
>> hw/hw.h | 2 ++
>> savevm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 55 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/hw.h b/hw/hw.h
>> index 921cf90..10e6dda 100644
>> --- a/hw/hw.h
>> +++ b/hw/hw.h
>> @@ -77,6 +77,8 @@ void qemu_fflush(QEMUFile *f);
>> int qemu_fclose(QEMUFile *f);
>> void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
>> void qemu_put_byte(QEMUFile *f, int v);
>> +void qemu_put_vector(QEMUFile *f, QEMUIOVector *qiov);
>> +void qemu_put_vector_prepare(QEMUFile *f);
>> void *qemu_realloc_buffer(QEMUFile *f, int size);
>> void qemu_clear_buffer(QEMUFile *f);
>>
>> diff --git a/savevm.c b/savevm.c
>> index 944e788..22d928c 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -180,6 +180,7 @@ struct QEMUFile {
>> uint8_t *buf;
>>
>> int has_error;
>> + int prepares_vector;
>> };
>>
>> typedef struct QEMUFileStdio
>> @@ -557,6 +558,58 @@ void qemu_put_byte(QEMUFile *f, int v)
>> qemu_fflush(f);
>> }
>>
>> +void qemu_put_vector(QEMUFile *f, QEMUIOVector *v)
>> +{
>> + struct iovec *iov;
>> + int cnt;
>> + size_t bufsize;
>> + uint8_t *buf;
>> +
>> + if (qemu_file_get_rate_limit(f) != 0) {
>> + fprintf(stderr,
>> + "Attempted to write vector while bandwidth limit is not zero.\n");
>> + abort();
>> + }
>> +
>> + /* checks prepares vector.
>> + * For fool proof purpose, qemu_put_vector_parepare should be called
>> + * before qemu_put_vector. Then, if qemu_put_* functions except this
>> + * is called after qemu_put_vector_prepare, program will abort().
>> + */
>> + if (!f->prepares_vector) {
>> + fprintf(stderr,
>> + "You should prepare with qemu_put_vector_prepare.\n");
>> + abort();
>> + } else if (f->prepares_vector&& f->buf_index != 0) {
>> + fprintf(stderr, "Wrote data after qemu_put_vector_prepare.\n");
>> + abort();
>> + }
>> + f->prepares_vector = 0;
>> +
>> + if (f->put_vector) {
>> + qemu_iovec_to_vector(v,&iov,&cnt);
>> + f->put_vector(f->opaque, iov, 0, cnt);
>> + } else {
>> + qemu_iovec_to_size(v,&bufsize);
>> + buf = qemu_malloc(bufsize + 1 /* for '\0' */);
>> + qemu_iovec_to_buffer(v, buf);
>> + qemu_put_buffer(f, buf, bufsize);
>> + qemu_free(buf);
>> + }
>> +
>> +}
>> +
>> +void qemu_put_vector_prepare(QEMUFile *f)
>> +{
>> + if (f->prepares_vector) {
>> + /* prepare vector */
>> + fprintf(stderr, "Attempted to prepare vector twice\n");
>> + abort();
>> + }
>> + f->prepares_vector = 1;
>> + qemu_fflush(f);
>> +}
>> +
>> int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
>> {
>> int size, l;
>
>
>
>
Anthony Liguori - April 23, 2010, 1:23 p.m.
On 04/22/2010 11:02 PM, Yoshiaki Tamura wrote:
> Anthony Liguori wrote:
>> On 04/21/2010 12:57 AM, Yoshiaki Tamura wrote:
>>> For fool proof purpose, qemu_put_vector_parepare should be called
>>> before qemu_put_vector. Then, if qemu_put_* functions except this is
>>> called after qemu_put_vector_prepare, program will abort().
>>>
>>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>>
>> I don't get it. What's this protecting against?
>
> This was introduced to prevent mixing the order of normal write and 
> vector write, and flush QEMUFile buffer before handling vectors.
> While qemu_put_buffer copies data to QEMUFile buffer, 
> qemu_put_vector() will bypass that buffer.
>
> It's just fool proof purpose for what we encountered at beginning, and 
> if the user of qemu_put_vector() is careful enough, we can remove 
> qemu_put_vectore_prepare().  While writing this message, I started to 
> think that just calling qemu_fflush() in qemu_put_vector() would be 
> enough...

I definitely think removing the vector stuff in the first version would 
simplify the process of getting everything merged.  I'd prefer not to 
have two apis so if vector operations were important from a performance 
perspective, I'd want to see everything converted to a vector API.

Regards,

Anthony Liguori
Yoshiaki Tamura - April 26, 2010, 10:43 a.m.
Anthony Liguori wrote:
> On 04/22/2010 11:02 PM, Yoshiaki Tamura wrote:
>> Anthony Liguori wrote:
>>> On 04/21/2010 12:57 AM, Yoshiaki Tamura wrote:
>>>> For fool proof purpose, qemu_put_vector_parepare should be called
>>>> before qemu_put_vector. Then, if qemu_put_* functions except this is
>>>> called after qemu_put_vector_prepare, program will abort().
>>>>
>>>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>>>
>>> I don't get it. What's this protecting against?
>>
>> This was introduced to prevent mixing the order of normal write and
>> vector write, and flush QEMUFile buffer before handling vectors.
>> While qemu_put_buffer copies data to QEMUFile buffer,
>> qemu_put_vector() will bypass that buffer.
>>
>> It's just fool proof purpose for what we encountered at beginning, and
>> if the user of qemu_put_vector() is careful enough, we can remove
>> qemu_put_vectore_prepare(). While writing this message, I started to
>> think that just calling qemu_fflush() in qemu_put_vector() would be
>> enough...
>
> I definitely think removing the vector stuff in the first version would
> simplify the process of getting everything merged. I'd prefer not to
> have two apis so if vector operations were important from a performance
> perspective, I'd want to see everything converted to a vector API.

I agree with your opinion.
I will measure the effect of introducing vector stuff, and post the data later.

Patch

diff --git a/hw/hw.h b/hw/hw.h
index 921cf90..10e6dda 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -77,6 +77,8 @@  void qemu_fflush(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
 void qemu_put_byte(QEMUFile *f, int v);
+void qemu_put_vector(QEMUFile *f, QEMUIOVector *qiov);
+void qemu_put_vector_prepare(QEMUFile *f);
 void *qemu_realloc_buffer(QEMUFile *f, int size);
 void qemu_clear_buffer(QEMUFile *f);
 
diff --git a/savevm.c b/savevm.c
index 944e788..22d928c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -180,6 +180,7 @@  struct QEMUFile {
     uint8_t *buf;
 
     int has_error;
+    int prepares_vector;
 };
 
 typedef struct QEMUFileStdio
@@ -557,6 +558,58 @@  void qemu_put_byte(QEMUFile *f, int v)
         qemu_fflush(f);
 }
 
+void qemu_put_vector(QEMUFile *f, QEMUIOVector *v)
+{
+    struct iovec *iov;
+    int cnt;
+    size_t bufsize;
+    uint8_t *buf;
+
+    if (qemu_file_get_rate_limit(f) != 0) {
+        fprintf(stderr,
+                "Attempted to write vector while bandwidth limit is not zero.\n");
+        abort();
+    }
+
+    /* checks prepares vector.
+     * For fool proof purpose, qemu_put_vector_parepare should be called
+     * before qemu_put_vector.  Then, if qemu_put_* functions except this
+     * is called after qemu_put_vector_prepare, program will abort().
+     */
+    if (!f->prepares_vector) {
+        fprintf(stderr,
+            "You should prepare with qemu_put_vector_prepare.\n");
+        abort();
+    } else if (f->prepares_vector && f->buf_index != 0) {
+        fprintf(stderr, "Wrote data after qemu_put_vector_prepare.\n");
+        abort();
+    }
+    f->prepares_vector = 0;
+
+    if (f->put_vector) {
+        qemu_iovec_to_vector(v, &iov, &cnt);
+        f->put_vector(f->opaque, iov, 0, cnt);
+    } else {
+        qemu_iovec_to_size(v, &bufsize);
+        buf = qemu_malloc(bufsize + 1 /* for '\0' */);
+        qemu_iovec_to_buffer(v, buf);
+        qemu_put_buffer(f, buf, bufsize);
+        qemu_free(buf);
+    }
+
+}
+
+void qemu_put_vector_prepare(QEMUFile *f)
+{
+    if (f->prepares_vector) {
+        /* prepare vector */
+        fprintf(stderr, "Attempted to prepare vector twice\n");
+        abort();
+    }
+    f->prepares_vector = 1;
+    qemu_fflush(f);
+}
+
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
 {
     int size, l;