| Message ID | 926db899bebc1f9ca575bfd29d140f7658050a82.1706542958.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix resource freeing bugs in virtio-gpu-rutabaga | expand |
On Mon, Jan 29, 2024 at 7:46 AM Manos Pitsidianakis < manos.pitsidianakis@linaro.org> wrote: > When the Rutabaga GPU device frees resources, it calls > rutabaga_resource_unref for that resource_id. However, when the generic > VirtIOGPU functions destroys resources, it only removes the > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. > The rutabaga resource associated with that resource_id is then leaked. > > This commit overrides the resource_destroy class method introduced in > the previous commit to fix this. > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 12 deletions(-) > > diff --git a/hw/display/virtio-gpu-rutabaga.c > b/hw/display/virtio-gpu-rutabaga.c > index 9e67f9bd51..6ac0776005 100644 > --- a/hw/display/virtio-gpu-rutabaga.c > +++ b/hw/display/virtio-gpu-rutabaga.c > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, > } > > static void > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res, > + Error **errp) > +{ > + int32_t result; > + const char *strerror = NULL; > + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > + > + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); > + if (result) { > + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 > + " for resource_id = %"PRIu32, __func__, result, > + res->resource_id); > + strerror = strerrorname_np((int)result); > + if (strerror != NULL) { > + error_append_hint(errp, "%s: %s\n", > + strerror, strerrordesc_np((int)result) ? : > ""); > + } > Can't we rely on virtio_gpu_rutabaga_debug_cb(..) to report an error when the resource ID is not found? > + } > + > + if (res->image) { > + pixman_image_unref(res->image); > + } > + > + QTAILQ_REMOVE(&g->reslist, res, next); > + g_free(res); > +} > + > +static void > rutabaga_cmd_resource_unref(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > - int32_t result; > + int32_t result = 0; > struct virtio_gpu_simple_resource *res; > struct virtio_gpu_resource_unref unref; > - > - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > + Error *local_err = NULL; > > VIRTIO_GPU_FILL_CMD(unref); > > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, > res = virtio_gpu_find_resource(g, unref.resource_id); > CHECK(res, cmd); > > - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); > - CHECK(!result, cmd); > - > - if (res->image) { > - pixman_image_unref(res->image); > + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); > + if (local_err) { > + error_report_err(local_err); > + /* local_err was freed, do not reuse it. */ > + local_err = NULL; > + result = 1; > } > - > - QTAILQ_REMOVE(&g->reslist, res, next); > - g_free(res); > + CHECK(!result, cmd); > } > > static void > @@ -1099,7 +1126,7 @@ static void > virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; > - > + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; > vdc->realize = virtio_gpu_rutabaga_realize; > device_class_set_props(dc, virtio_gpu_rutabaga_properties); > } > -- > γαῖα πυρί μιχθήτω > >
On Tue, 30 Jan 2024 03:26, Gurchetan Singh <gurchetansingh@chromium.org> wrote: >On Mon, Jan 29, 2024 at 7:46 AM Manos Pitsidianakis < >manos.pitsidianakis@linaro.org> wrote: > >> When the Rutabaga GPU device frees resources, it calls >> rutabaga_resource_unref for that resource_id. However, when the generic >> VirtIOGPU functions destroys resources, it only removes the >> virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. >> The rutabaga resource associated with that resource_id is then leaked. >> >> This commit overrides the resource_destroy class method introduced in >> the previous commit to fix this. >> >> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >> --- >> hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- >> 1 file changed, 39 insertions(+), 12 deletions(-) >> >> diff --git a/hw/display/virtio-gpu-rutabaga.c >> b/hw/display/virtio-gpu-rutabaga.c >> index 9e67f9bd51..6ac0776005 100644 >> --- a/hw/display/virtio-gpu-rutabaga.c >> +++ b/hw/display/virtio-gpu-rutabaga.c >> @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, >> } >> >> static void >> +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, >> + struct virtio_gpu_simple_resource *res, >> + Error **errp) >> +{ >> + int32_t result; >> + const char *strerror = NULL; >> + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); >> + >> + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); >> + if (result) { >> + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 >> + " for resource_id = %"PRIu32, __func__, result, >> + res->resource_id); >> + strerror = strerrorname_np((int)result); >> + if (strerror != NULL) { >> + error_append_hint(errp, "%s: %s\n", >> + strerror, strerrordesc_np((int)result) ? : >> ""); >> + } >> > >Can't we rely on virtio_gpu_rutabaga_debug_cb(..) to report an error when >the resource ID is not found? IIUC that callback is called from the external library, and uses its own type (const struct rutabaga *debug) which is not how the interface works here. rutabaga_resource_unref() returns an int32_t. Besides this isn't a debug print but an error print. > >> + } >> + >> + if (res->image) { >> + pixman_image_unref(res->image); >> + } >> + >> + QTAILQ_REMOVE(&g->reslist, res, next); >> + g_free(res); >> +} >> + >> +static void >> rutabaga_cmd_resource_unref(VirtIOGPU *g, >> struct virtio_gpu_ctrl_command *cmd) >> { >> - int32_t result; >> + int32_t result = 0; >> struct virtio_gpu_simple_resource *res; >> struct virtio_gpu_resource_unref unref; >> - >> - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); >> + Error *local_err = NULL; >> >> VIRTIO_GPU_FILL_CMD(unref); >> >> @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, >> res = virtio_gpu_find_resource(g, unref.resource_id); >> CHECK(res, cmd); >> >> - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); >> - CHECK(!result, cmd); >> - >> - if (res->image) { >> - pixman_image_unref(res->image); >> + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + /* local_err was freed, do not reuse it. */ >> + local_err = NULL; >> + result = 1; >> } >> - >> - QTAILQ_REMOVE(&g->reslist, res, next); >> - g_free(res); >> + CHECK(!result, cmd); >> } >> >> static void >> @@ -1099,7 +1126,7 @@ static void >> virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) >> vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; >> vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; >> vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; >> - >> + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; >> vdc->realize = virtio_gpu_rutabaga_realize; >> device_class_set_props(dc, virtio_gpu_rutabaga_properties); >> } >> -- >> γαῖα πυρί μιχθήτω >> >>
Hi On Mon, Jan 29, 2024 at 7:46 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > When the Rutabaga GPU device frees resources, it calls > rutabaga_resource_unref for that resource_id. However, when the generic > VirtIOGPU functions destroys resources, it only removes the > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. > The rutabaga resource associated with that resource_id is then leaked. > > This commit overrides the resource_destroy class method introduced in > the previous commit to fix this. > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 12 deletions(-) > > diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c > index 9e67f9bd51..6ac0776005 100644 > --- a/hw/display/virtio-gpu-rutabaga.c > +++ b/hw/display/virtio-gpu-rutabaga.c > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, > } > > static void > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res, > + Error **errp) > +{ > + int32_t result; > + const char *strerror = NULL; > + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > + > + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); > + if (result) { > + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 > + " for resource_id = %"PRIu32, __func__, result, > + res->resource_id); > + strerror = strerrorname_np((int)result); > + if (strerror != NULL) { > + error_append_hint(errp, "%s: %s\n", > + strerror, strerrordesc_np((int)result) ? : ""); > + } > + } There is error_setg_errno() > + > + if (res->image) { > + pixman_image_unref(res->image); > + } > + > + QTAILQ_REMOVE(&g->reslist, res, next); > + g_free(res); > +} > + > +static void > rutabaga_cmd_resource_unref(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > - int32_t result; > + int32_t result = 0; > struct virtio_gpu_simple_resource *res; > struct virtio_gpu_resource_unref unref; > - > - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > + Error *local_err = NULL; > > VIRTIO_GPU_FILL_CMD(unref); > > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, > res = virtio_gpu_find_resource(g, unref.resource_id); > CHECK(res, cmd); > > - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); > - CHECK(!result, cmd); > - > - if (res->image) { > - pixman_image_unref(res->image); > + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); > + if (local_err) { > + error_report_err(local_err); > + /* local_err was freed, do not reuse it. */ > + local_err = NULL; > + result = 1; > } > - > - QTAILQ_REMOVE(&g->reslist, res, next); > - g_free(res); > + CHECK(!result, cmd); > } > > static void > @@ -1099,7 +1126,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; > - > + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; > vdc->realize = virtio_gpu_rutabaga_realize; > device_class_set_props(dc, virtio_gpu_rutabaga_properties); > } > -- > γαῖα πυρί μιχθήτω >
On Tue, 30 Jan 2024 at 14:50, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Mon, Jan 29, 2024 at 7:46 PM Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> wrote: > > > > When the Rutabaga GPU device frees resources, it calls > > rutabaga_resource_unref for that resource_id. However, when the generic > > VirtIOGPU functions destroys resources, it only removes the > > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. > > The rutabaga resource associated with that resource_id is then leaked. > > > > This commit overrides the resource_destroy class method introduced in > > the previous commit to fix this. > > > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > --- > > hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- > > 1 file changed, 39 insertions(+), 12 deletions(-) > > > > diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c > > index 9e67f9bd51..6ac0776005 100644 > > --- a/hw/display/virtio-gpu-rutabaga.c > > +++ b/hw/display/virtio-gpu-rutabaga.c > > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, > > } > > > > static void > > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, > > + struct virtio_gpu_simple_resource *res, > > + Error **errp) > > +{ > > + int32_t result; > > + const char *strerror = NULL; > > + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > > + > > + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); > > + if (result) { > > + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 > > + " for resource_id = %"PRIu32, __func__, result, > > + res->resource_id); > > + strerror = strerrorname_np((int)result); > > + if (strerror != NULL) { > > + error_append_hint(errp, "%s: %s\n", > > + strerror, strerrordesc_np((int)result) ? : ""); > > + } > > + } > > There is error_setg_errno() I did not use it on purpose because I was not certain if rutabaga_gfx starts using other error numbers in the future. I don't like my approach but I don't like assuming it's an errno either to be honest... Which one seems better to you? Thanks, Manos > > + > > + if (res->image) { > > + pixman_image_unref(res->image); > > + } > > + > > + QTAILQ_REMOVE(&g->reslist, res, next); > > + g_free(res); > > +} > > + > > +static void > > rutabaga_cmd_resource_unref(VirtIOGPU *g, > > struct virtio_gpu_ctrl_command *cmd) > > { > > - int32_t result; > > + int32_t result = 0; > > struct virtio_gpu_simple_resource *res; > > struct virtio_gpu_resource_unref unref; > > - > > - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > > + Error *local_err = NULL; > > > > VIRTIO_GPU_FILL_CMD(unref); > > > > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, > > res = virtio_gpu_find_resource(g, unref.resource_id); > > CHECK(res, cmd); > > > > - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); > > - CHECK(!result, cmd); > > - > > - if (res->image) { > > - pixman_image_unref(res->image); > > + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); > > + if (local_err) { > > + error_report_err(local_err); > > + /* local_err was freed, do not reuse it. */ > > + local_err = NULL; > > + result = 1; > > } > > - > > - QTAILQ_REMOVE(&g->reslist, res, next); > > - g_free(res); > > + CHECK(!result, cmd); > > } > > > > static void > > @@ -1099,7 +1126,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) > > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; > > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; > > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; > > - > > + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; > > vdc->realize = virtio_gpu_rutabaga_realize; > > device_class_set_props(dc, virtio_gpu_rutabaga_properties); > > } > > -- > > γαῖα πυρί μιχθήτω > > > > > -- > Marc-André Lureau
Hi On Tue, Jan 30, 2024 at 5:01 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > On Tue, 30 Jan 2024 at 14:50, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > Hi > > > > On Mon, Jan 29, 2024 at 7:46 PM Manos Pitsidianakis > > <manos.pitsidianakis@linaro.org> wrote: > > > > > > When the Rutabaga GPU device frees resources, it calls > > > rutabaga_resource_unref for that resource_id. However, when the generic > > > VirtIOGPU functions destroys resources, it only removes the > > > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. > > > The rutabaga resource associated with that resource_id is then leaked. > > > > > > This commit overrides the resource_destroy class method introduced in > > > the previous commit to fix this. > > > > > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > > --- > > > hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- > > > 1 file changed, 39 insertions(+), 12 deletions(-) > > > > > > diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c > > > index 9e67f9bd51..6ac0776005 100644 > > > --- a/hw/display/virtio-gpu-rutabaga.c > > > +++ b/hw/display/virtio-gpu-rutabaga.c > > > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, > > > } > > > > > > static void > > > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, > > > + struct virtio_gpu_simple_resource *res, > > > + Error **errp) > > > +{ > > > + int32_t result; > > > + const char *strerror = NULL; > > > + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > > > + > > > + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); > > > + if (result) { > > > + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 > > > + " for resource_id = %"PRIu32, __func__, result, > > > + res->resource_id); > > > + strerror = strerrorname_np((int)result); > > > + if (strerror != NULL) { > > > + error_append_hint(errp, "%s: %s\n", > > > + strerror, strerrordesc_np((int)result) ? : ""); > > > + } > > > + } > > > > There is error_setg_errno() > > I did not use it on purpose because I was not certain if rutabaga_gfx > starts using other error numbers in the future. I don't like my > approach but I don't like assuming it's an errno either to be > honest... Which one seems better to you? > In that case, don't use strerrordesc_np() either :) I think we can assume they will keep using errno though, unless they clearly communicate this and break API, which seems unlikely to me. > Thanks, > Manos > > > > + > > > + if (res->image) { > > > + pixman_image_unref(res->image); > > > + } > > > + > > > + QTAILQ_REMOVE(&g->reslist, res, next); > > > + g_free(res); > > > +} > > > + > > > +static void > > > rutabaga_cmd_resource_unref(VirtIOGPU *g, > > > struct virtio_gpu_ctrl_command *cmd) > > > { > > > - int32_t result; > > > + int32_t result = 0; > > > struct virtio_gpu_simple_resource *res; > > > struct virtio_gpu_resource_unref unref; > > > - > > > - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > > > + Error *local_err = NULL; > > > > > > VIRTIO_GPU_FILL_CMD(unref); > > > > > > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, > > > res = virtio_gpu_find_resource(g, unref.resource_id); > > > CHECK(res, cmd); > > > > > > - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); > > > - CHECK(!result, cmd); > > > - > > > - if (res->image) { > > > - pixman_image_unref(res->image); > > > + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); > > > + if (local_err) { > > > + error_report_err(local_err); > > > + /* local_err was freed, do not reuse it. */ > > > + local_err = NULL; > > > + result = 1; > > > } > > > - > > > - QTAILQ_REMOVE(&g->reslist, res, next); > > > - g_free(res); > > > + CHECK(!result, cmd); > > > } > > > > > > static void > > > @@ -1099,7 +1126,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) > > > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; > > > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; > > > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; > > > - > > > + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; > > > vdc->realize = virtio_gpu_rutabaga_realize; > > > device_class_set_props(dc, virtio_gpu_rutabaga_properties); > > > } > > > -- > > > γαῖα πυρί μιχθήτω > > > > > > > > > -- > > Marc-André Lureau
On Tue, 30 Jan 2024 at 15:10, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Tue, Jan 30, 2024 at 5:01 PM Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> wrote: > > > > On Tue, 30 Jan 2024 at 14:50, Marc-André Lureau > > <marcandre.lureau@gmail.com> wrote: > > > > > > Hi > > > > > > On Mon, Jan 29, 2024 at 7:46 PM Manos Pitsidianakis > > > <manos.pitsidianakis@linaro.org> wrote: > > > > > > > > When the Rutabaga GPU device frees resources, it calls > > > > rutabaga_resource_unref for that resource_id. However, when the generic > > > > VirtIOGPU functions destroys resources, it only removes the > > > > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. > > > > The rutabaga resource associated with that resource_id is then leaked. > > > > > > > > This commit overrides the resource_destroy class method introduced in > > > > the previous commit to fix this. > > > > > > > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > > > --- > > > > hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- > > > > 1 file changed, 39 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c > > > > index 9e67f9bd51..6ac0776005 100644 > > > > --- a/hw/display/virtio-gpu-rutabaga.c > > > > +++ b/hw/display/virtio-gpu-rutabaga.c > > > > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, > > > > } > > > > > > > > static void > > > > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, > > > > + struct virtio_gpu_simple_resource *res, > > > > + Error **errp) > > > > +{ > > > > + int32_t result; > > > > + const char *strerror = NULL; > > > > + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > > > > + > > > > + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); > > > > + if (result) { > > > > + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 > > > > + " for resource_id = %"PRIu32, __func__, result, > > > > + res->resource_id); > > > > + strerror = strerrorname_np((int)result); > > > > + if (strerror != NULL) { > > > > + error_append_hint(errp, "%s: %s\n", > > > > + strerror, strerrordesc_np((int)result) ? : ""); > > > > + } > > > > + } > > > > > > There is error_setg_errno() > > > > I did not use it on purpose because I was not certain if rutabaga_gfx > > starts using other error numbers in the future. I don't like my > > approach but I don't like assuming it's an errno either to be > > honest... Which one seems better to you? > > > > In that case, don't use strerrordesc_np() either :) strerrordesc_np will be valid if strerrorname_np() does not return NULL. > I think we can assume they will keep using errno though, unless they > clearly communicate this and break API, which seems unlikely to me. I will use error_setg_errno then, thanks! Manos > > Thanks, > > Manos > > > > > > + > > > > + if (res->image) { > > > > + pixman_image_unref(res->image); > > > > + } > > > > + > > > > + QTAILQ_REMOVE(&g->reslist, res, next); > > > > + g_free(res); > > > > +} > > > > + > > > > +static void > > > > rutabaga_cmd_resource_unref(VirtIOGPU *g, > > > > struct virtio_gpu_ctrl_command *cmd) > > > > { > > > > - int32_t result; > > > > + int32_t result = 0; > > > > struct virtio_gpu_simple_resource *res; > > > > struct virtio_gpu_resource_unref unref; > > > > - > > > > - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > > > > + Error *local_err = NULL; > > > > > > > > VIRTIO_GPU_FILL_CMD(unref); > > > > > > > > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, > > > > res = virtio_gpu_find_resource(g, unref.resource_id); > > > > CHECK(res, cmd); > > > > > > > > - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); > > > > - CHECK(!result, cmd); > > > > - > > > > - if (res->image) { > > > > - pixman_image_unref(res->image); > > > > + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); > > > > + if (local_err) { > > > > + error_report_err(local_err); > > > > + /* local_err was freed, do not reuse it. */ > > > > + local_err = NULL; > > > > + result = 1; > > > > } > > > > - > > > > - QTAILQ_REMOVE(&g->reslist, res, next); > > > > - g_free(res); > > > > + CHECK(!result, cmd); > > > > } > > > > > > > > static void > > > > @@ -1099,7 +1126,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) > > > > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; > > > > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; > > > > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; > > > > - > > > > + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; > > > > vdc->realize = virtio_gpu_rutabaga_realize; > > > > device_class_set_props(dc, virtio_gpu_rutabaga_properties); > > > > } > > > > -- > > > > γαῖα πυρί μιχθήτω > > > > > > > > > > > > > -- > > > Marc-André Lureau > > > > -- > Marc-André Lureau
diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c index 9e67f9bd51..6ac0776005 100644 --- a/hw/display/virtio-gpu-rutabaga.c +++ b/hw/display/virtio-gpu-rutabaga.c @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, } static void +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, + struct virtio_gpu_simple_resource *res, + Error **errp) +{ + int32_t result; + const char *strerror = NULL; + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); + + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); + if (result) { + error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32 + " for resource_id = %"PRIu32, __func__, result, + res->resource_id); + strerror = strerrorname_np((int)result); + if (strerror != NULL) { + error_append_hint(errp, "%s: %s\n", + strerror, strerrordesc_np((int)result) ? : ""); + } + } + + if (res->image) { + pixman_image_unref(res->image); + } + + QTAILQ_REMOVE(&g->reslist, res, next); + g_free(res); +} + +static void rutabaga_cmd_resource_unref(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { - int32_t result; + int32_t result = 0; struct virtio_gpu_simple_resource *res; struct virtio_gpu_resource_unref unref; - - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); + Error *local_err = NULL; VIRTIO_GPU_FILL_CMD(unref); @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, res = virtio_gpu_find_resource(g, unref.resource_id); CHECK(res, cmd); - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); - CHECK(!result, cmd); - - if (res->image) { - pixman_image_unref(res->image); + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); + if (local_err) { + error_report_err(local_err); + /* local_err was freed, do not reuse it. */ + local_err = NULL; + result = 1; } - - QTAILQ_REMOVE(&g->reslist, res, next); - g_free(res); + CHECK(!result, cmd); } static void @@ -1099,7 +1126,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; - + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; vdc->realize = virtio_gpu_rutabaga_realize; device_class_set_props(dc, virtio_gpu_rutabaga_properties); }
When the Rutabaga GPU device frees resources, it calls rutabaga_resource_unref for that resource_id. However, when the generic VirtIOGPU functions destroys resources, it only removes the virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. The rutabaga resource associated with that resource_id is then leaked. This commit overrides the resource_destroy class method introduced in the previous commit to fix this. Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- hw/display/virtio-gpu-rutabaga.c | 51 ++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-)