Re: [PATCH v2 kvmtool 15/30] virtio: Don't ignore initialization failures

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Andre Przywara <andre.przywara@arm.com>
Cc: kvm@vger.kernel.org, will@kernel.org,
	julien.thierry.kdev@gmail.com, sami.mujawar@arm.com,
	lorenzo.pieralisi@arm.com, maz@kernel.org
Subject: Re: [PATCH v2 kvmtool 15/30] virtio: Don't ignore initialization failures
Date: Fri, 6 Mar 2020 11:20:00 +0000	[thread overview]
Message-ID: <ad0199a8-bd18-4031-3489-eca9865b68fb@arm.com> (raw)
In-Reply-To: <20200130145120.0cad4a14@donnerap.cambridge.arm.com>

Hi,

On 1/30/20 2:51 PM, Andre Przywara wrote:
> On Thu, 23 Jan 2020 13:47:50 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> 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