| Message ID | 1271829445-5328-8-git-send-email-tamura.yoshiaki@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
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; >
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; > > > >
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
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.
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;
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(-)