Re: [PATCH v13 11/13] virtio-gpu: Handle resource blob commands

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/06/03 14:32, Dmitry Osipenko wrote:
> On 6/2/24 08:45, Akihiko Odaki wrote:
> ...
>>> +    case HOSTMEM_MR_FINISH_UNMAPPING:
>>> +        ret = virgl_renderer_resource_unmap(res->base.resource_id);
>>> +        if (ret) {
>>> +            qemu_log_mask(LOG_GUEST_ERROR,
>>> +                          "%s: failed to unmap virgl resource: %s\n",
>>> +                          __func__, strerror(-ret));
>>> +            return ret;
>>> +        }
>>> +        res->mr = NULL;
>>> +        g_free(vmr);
>>> +        break;
>>> +    case HOSTMEM_MR_UNMAPPING:
>>> +        *cmd_suspended = true;
>>
>> This code path should be unreachable since the command processing is
>> blocked while unmapping.
> 
> Will change to abort()

I don't think abort() call is needed here. You can just do what you do 
for HOSTMEM_MR_MAPPED; the reference counter will automatically catch 
the double-free condition and abort.

> 
>>> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
>>> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
>>> sizeof(cblob),
>>> +                                            cmd, &res->base.addrs,
>>> +                                            &res->base.iov,
>>> &res->base.iov_cnt);
>>> +        if (!ret) {
>>> +            g_free(res);
>>
>> As noted for an earlier version:
>>> Use g_autofree instead of writing duplicate g_free() calls. See
>>> docs/devel/style.rst for details.
> 
> The g_autofree isn't appropriate for this code. It's intended to be used
> if you allocate a tmp variable that should be freed in all code paths.
> This is not the case here, the res variable isn't temporal and shall not
> be freed on success.

You can assign NULL to res after QTAILQ_INSERT_HEAD(). See 
rutabaga_cmd_resource_create_blob() for example.

Usually g_steal_pointer() should be used in such a situation but 
unfortunately it is not possible in this case due to how 
QTAILQ_INSERT_HEAD() macro is implemented.