Hi,
On 1/30/20 2:51 PM, Andre Przywara wrote:
> On Thu, 23 Jan 2020 13:47:51 +0000
> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi,
>
>> An error returned by device__register, kvm__register_mmio and
>> ioport__register means that the device will
>> not be emulated properly. Annotate the functions with __must_check, so we
>> get a compiler warning when this error is ignored.
>>
>> And fix several instances where the caller returns 0 even if the
>> function failed.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> Looks alright, one minor nit below, with that fixed:
>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
[..]
>> diff --git a/ioport.c b/ioport.c
>> index a72e4035881a..d224819c6e43 100644
>> --- a/ioport.c
>> +++ b/ioport.c
>> @@ -91,16 +91,21 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
>> };
>>
>> r = ioport_insert(&ioport_tree, entry);
>> - if (r < 0) {
>> - free(entry);
>> - br_write_unlock(kvm);
>> - return r;
>> - }
>> -
>> - device__register(&entry->dev_hdr);
>> + if (r < 0)
>> + goto out_free;
>> + r = device__register(&entry->dev_hdr);
>> + if (r < 0)
>> + goto out_erase;
>> br_write_unlock(kvm);
>>
>> return port;
>> +
>> +out_erase:
>> + rb_int_erase(&ioport_tree, &entry->node);
> To keep the abstraction, shouldn't that rather be ioport_remove() instead?
ioport__register already uses rb_int_erase to remove a node (at the beginning, if
the requested port is already allocated). But you're right, it should use
ioport_remove in both cases, like ioport__unregister{,_all} does.
Thanks,
Alex
