| Message ID | 1409213061-15562-2-git-send-email-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
The Thursday 28 Aug 2014 à 09:04:21 (+0100), Richard W.M. Jones wrote : > In commit 63f0f45f2e89b60ff8245fec81328ddfde42a303 the following > mechanical change was made: > > if (!state) { > - qemu_aio_wait(); > + aio_poll(state->s->aio_context, true); > } > > The new code now checks if state is NULL and then dereferences it > ('state->s') which is obviously incorrect. > > This commit replaces state->s->aio_context with > bdrv_get_aio_context(bs), fixing this problem. The two other hunks > are concerned with getting the BlockDriverState pointer bs to where it > is needed. > > The original bug causes a segfault when using libguestfs to access a > VMware vCenter Server and doing any kind of complex read-heavy > operations. With this commit the segfault goes away. > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/curl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index d4b85d2..f59615d 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > #endif > } > > -static CURLState *curl_init_state(BDRVCURLState *s) > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > { > CURLState *state = NULL; > int i, j; > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > break; > } > if (!state) { > - aio_poll(state->s->aio_context, true); > + aio_poll(bdrv_get_aio_context(bs), true); > } > } while(!state); > > @@ -541,7 +541,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, > DPRINTF("CURL: Opening %s\n", file); > s->aio_context = bdrv_get_aio_context(bs); > s->url = g_strdup(file); > - state = curl_init_state(s); > + state = curl_init_state(bs, s); > if (!state) > goto out_noclean; > > @@ -625,7 +625,7 @@ static void curl_readv_bh_cb(void *p) > } > > // No cache found, so let's start a new request > - state = curl_init_state(s); > + state = curl_init_state(acb->common.bs, s); > if (!state) { > acb->common.cb(acb->common.opaque, -EIO); > qemu_aio_release(acb); > -- > 2.0.4 > > Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
On Thu, Aug 28, 2014 at 09:04:21AM +0100, Richard W.M. Jones wrote: > diff --git a/block/curl.c b/block/curl.c > index d4b85d2..f59615d 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > #endif > } > > -static CURLState *curl_init_state(BDRVCURLState *s) > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > { > CURLState *state = NULL; > int i, j; Why add the BDRVCURLState *s argument... > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > break; > } > if (!state) { > - aio_poll(state->s->aio_context, true); > + aio_poll(bdrv_get_aio_context(bs), true); > } > } while(!state); > ...if it is not used?
On Fri, Aug 29, 2014 at 10:03:59AM +0100, Stefan Hajnoczi wrote: > On Thu, Aug 28, 2014 at 09:04:21AM +0100, Richard W.M. Jones wrote: > > diff --git a/block/curl.c b/block/curl.c > > index d4b85d2..f59615d 100644 > > --- a/block/curl.c > > +++ b/block/curl.c > > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > > #endif > > } > > > > -static CURLState *curl_init_state(BDRVCURLState *s) > > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > > { > > CURLState *state = NULL; > > int i, j; > > Why add the BDRVCURLState *s argument... > > > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > > break; > > } > > if (!state) { > > - aio_poll(state->s->aio_context, true); > > + aio_poll(bdrv_get_aio_context(bs), true); > > } > > } while(!state); > > > > ...if it is not used? I may be misunderstanding this, but BDRVCURLState *s is used elsewhere in the function. However there is a potential to derive `BDRVCURLState *s = bs->opaque;' if that's what you meant? Rich.
On Fri, Aug 29, 2014 at 10:19:04AM +0100, Richard W.M. Jones wrote: > On Fri, Aug 29, 2014 at 10:03:59AM +0100, Stefan Hajnoczi wrote: > > On Thu, Aug 28, 2014 at 09:04:21AM +0100, Richard W.M. Jones wrote: > > > diff --git a/block/curl.c b/block/curl.c > > > index d4b85d2..f59615d 100644 > > > --- a/block/curl.c > > > +++ b/block/curl.c > > > @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) > > > #endif > > > } > > > > > > -static CURLState *curl_init_state(BDRVCURLState *s) > > > +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) > > > { > > > CURLState *state = NULL; > > > int i, j; > > > > Why add the BDRVCURLState *s argument... > > > > > @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) > > > break; > > > } > > > if (!state) { > > > - aio_poll(state->s->aio_context, true); > > > + aio_poll(bdrv_get_aio_context(bs), true); > > > } > > > } while(!state); > > > > > > > ...if it is not used? > > I may be misunderstanding this, but BDRVCURLState *s is used elsewhere > in the function. > > However there is a potential to derive `BDRVCURLState *s = bs->opaque;' > if that's what you meant? Sorry, I misread the code. I thought you added s but you really added bs. That's fine and it makes sense now :). Stefan
diff --git a/block/curl.c b/block/curl.c index d4b85d2..f59615d 100644 --- a/block/curl.c +++ b/block/curl.c @@ -352,7 +352,7 @@ static void curl_multi_timeout_do(void *arg) #endif } -static CURLState *curl_init_state(BDRVCURLState *s) +static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s) { CURLState *state = NULL; int i, j; @@ -370,7 +370,7 @@ static CURLState *curl_init_state(BDRVCURLState *s) break; } if (!state) { - aio_poll(state->s->aio_context, true); + aio_poll(bdrv_get_aio_context(bs), true); } } while(!state); @@ -541,7 +541,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, DPRINTF("CURL: Opening %s\n", file); s->aio_context = bdrv_get_aio_context(bs); s->url = g_strdup(file); - state = curl_init_state(s); + state = curl_init_state(bs, s); if (!state) goto out_noclean; @@ -625,7 +625,7 @@ static void curl_readv_bh_cb(void *p) } // No cache found, so let's start a new request - state = curl_init_state(s); + state = curl_init_state(acb->common.bs, s); if (!state) { acb->common.cb(acb->common.opaque, -EIO); qemu_aio_release(acb);
In commit 63f0f45f2e89b60ff8245fec81328ddfde42a303 the following mechanical change was made: if (!state) { - qemu_aio_wait(); + aio_poll(state->s->aio_context, true); } The new code now checks if state is NULL and then dereferences it ('state->s') which is obviously incorrect. This commit replaces state->s->aio_context with bdrv_get_aio_context(bs), fixing this problem. The two other hunks are concerned with getting the BlockDriverState pointer bs to where it is needed. The original bug causes a segfault when using libguestfs to access a VMware vCenter Server and doing any kind of complex read-heavy operations. With this commit the segfault goes away. Signed-off-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- block/curl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)