On Mon, Jul 18, 2022, Maxim Levitsky wrote:
> I honestly don't see much value in this change, but I don't mind it either.
Yeah, this particular instance isn't a significant improvement, but I really dislike
the pattern (if the target is a raw return) and want to discourage its use in KVM.
For longer functions, having to scroll down to see that the target is nothing more
than a raw return is quite annoying. And for more complex usage, the pattern sometimes
leads to setting the return value well ahead of the "goto", which combined with the
scrolling is very unfriendly to readers.
E.g. prior to commit 71a4c30bf0d3 ("KVM: Refactor error handling for setting memory
region"), the memslot code input validation was written as so. The "r = 0" in the
"Nothing to change" path was especially gross.
r = -EINVAL;
as_id = mem->slot >> 16;
id = (u16)mem->slot;
/* General sanity checks */
if (mem->memory_size & (PAGE_SIZE - 1))
goto out;
if (mem->guest_phys_addr & (PAGE_SIZE - 1))
goto out;
/* We can read the guest memory with __xxx_user() later on. */
if ((id < KVM_USER_MEM_SLOTS) &&
((mem->userspace_addr & (PAGE_SIZE - 1)) ||
!access_ok((void __user *)(unsigned long)mem->userspace_addr,
mem->memory_size)))
goto out;
if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_MEM_SLOTS_NUM)
goto out;
if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
goto out;
slot = id_to_memslot(__kvm_memslots(kvm, as_id), id);
base_gfn = mem->guest_phys_addr >> PAGE_SHIFT;
npages = mem->memory_size >> PAGE_SHIFT;
if (npages > KVM_MEM_MAX_NR_PAGES)
goto out;
new = old = *slot;
new.id = id;
new.base_gfn = base_gfn;
new.npages = npages;
new.flags = mem->flags;
new.userspace_addr = mem->userspace_addr;
if (npages) {
if (!old.npages)
change = KVM_MR_CREATE;
else { /* Modify an existing slot. */
if ((new.userspace_addr != old.userspace_addr) ||
(npages != old.npages) ||
((new.flags ^ old.flags) & KVM_MEM_READONLY))
goto out;
if (base_gfn != old.base_gfn)
change = KVM_MR_MOVE;
else if (new.flags != old.flags)
change = KVM_MR_FLAGS_ONLY;
else { /* Nothing to change. */
r = 0;
goto out;
}
}
} else {
if (!old.npages)
goto out;
change = KVM_MR_DELETE;
new.base_gfn = 0;
new.flags = 0;
}
