Hi,
On 3/30/20 10:30 AM, André Przywara wrote:
> Hi,
>
> On 26/03/2020 15:24, Alexandru Elisei wrote:
>> Don't ignore an error in the bus specific initialization function in
>> virtio_init; don't ignore the result of virtio_init; and don't return 0
>> in virtio_blk__init and virtio_scsi__init when we encounter an error.
>> Hopefully this will save some developer's time debugging faulty virtio
>> devices in a guest.
>>
>> To take advantage of the cleanup function virtio_blk__exit, we have
>> moved appending the new device to the list before the call to
>> virtio_init.
>>
>> To safeguard against this in the future, virtio_init has been annoted
>> with the compiler attribute warn_unused_result.
> This is a good idea, but actually triggers an unrelated, long standing
> bug in vesa.c (on x86):
> hw/vesa.c: In function ‘vesa__init’:
> hw/vesa.c:77:3: error: ignoring return value of ‘ERR_PTR’, declared with
> attribute warn_unused_result [-Werror=unused-result]
> ERR_PTR(-errno);
> ^
> cc1: all warnings being treated as errors
>
> So could you add the missing "return" statement in that line, to fix
> that bug?
> I see that this gets rectified two patches later, but for the sake of
> bisect-ability it would be good to keep this compilable all the way through.
You're right, I'll fix it here.
>
>> diff --git a/virtio/net.c b/virtio/net.c
>> index 091406912a24..425c13ba1136 100644
>> --- a/virtio/net.c
>> +++ b/virtio/net.c
>> @@ -910,7 +910,7 @@ done:
>>
>> static int virtio_net__init_one(struct virtio_net_params *params)
>> {
>> - int i, err;
>> + int i, r;
>> struct net_dev *ndev;
>> struct virtio_ops *ops;
>> enum virtio_trans trans = VIRTIO_DEFAULT_TRANS(params->kvm);
>> @@ -920,10 +920,8 @@ static int virtio_net__init_one(struct virtio_net_params *params)
>> return -ENOMEM;
>>
>> ops = malloc(sizeof(*ops));
>> - if (ops == NULL) {
>> - err = -ENOMEM;
>> - goto err_free_ndev;
>> - }
>> + if (ops == NULL)
>> + return -ENOMEM;
>>
>> list_add_tail(&ndev->list, &ndevs);
> As mentioned in the reply to the v2 version, I think this is still
> leaking memory here.
Yes, the leak is there. I didn't realize that virtio_net__exit doesn't do any
free'ing at all.
I find it a bit strange that we copy net_dev_virtio_ops for each device instance.
I suspect that passing the pointer to the static struct to virtio_init would have
been enough (this is what blk does), but I might be missing something. I don't
want to introduce a regression, so I'll keep the memory allocation and free it on
error.
Thanks,
Alex
