Hi,
On 1/30/20 2:51 PM, Andre Przywara wrote:
> On Thu, 23 Jan 2020 13:47:50 +0000
> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi,
>
>> 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.
> Seems like the right thing to do, but I was wondering how you triggered this? AFAICS virtio_init only fails when calloc() fails or you pass an illegal transport, with the latter looking like being hard coded to one of the two supported.
I haven't triggered it. I found it by inspection. The transport-specific
initialization functions can fail for various reasons (ioport_register or
kvm__register_mmio can fail because some device emulation claimed all the MMIO
space or the MMIO space was configured incorrectly in the kvm-arch.h header file;
or memory allocation failed, etc) and this is the reason they return an int.
Because of this, virtio_init can fail and this is the reason it too returns an
int. It makes sense to check that the protocol that your device uses is actually
working.
>
> One minor thing below ...
[..]
>> 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;
> Doesn't that leave struct net_dev allocated? I am happy with removing the goto, but we should free(ndev) before we return, I think.
Nope, the cleanup routine in virtio_net__exit takes care of deallocating it (you
get there from virtio_net__init if virtio_net__init_one fails).
Thanks,
Alex
