lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 11 Jun 2020 08:35:57 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Vivek Goyal <vgoyal@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf()

On Thu, Jun 11, 2020 at 10:31:08AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@...el.com> writes:
> 
> >
> > I'd also be in favor of changing the return type to a boolean.  I think
> > you alluded to it earlier, the current semantics are quite confusing as they
> > invert the normal "return 0 on success".
> 
> Yes, will do a follow-up.
> 
> KVM/x86 code has an intertwined mix of:
> - normal 'int' functions ('0 on success')
> - bool functions ('true'/'1' on success)
> - 'int' exit handlers ('1'/'0' on success depending if exit to userspace
>   was required)
> - ...
> 
> I think we can try to standardize this to:
> - 'int' when error is propagated outside of KVM (userspace, other kernel
>   subsystem,...)
> - 'bool' when the function is internal to KVM and the result is binary
>  ('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()',
>  ...)
> - 'enum' for the rest.
> And, if there's a good reason for making an exception, require a
> comment. (leaving aside everything returning a pointer, of course as
> these are self-explanatory -- unless it's 'void *' :-))

Agreed for 'bool' case, but 'int' versus 'enum' is less straightforward as
there are a huge number of functions that _may_ propagate an error outside
of KVM, including all of the exit handlers.  As Paolo point out[*], it'd
be quite easy to end up with a mixture of enum/#define and 0/1 code, which
would be an even bigger mess than what we have today.  There are
undoubtedly cases that could be converted to an enum, but they're probably
few and far between as it requires total encapsulation, e.g. the emulator.

[*] https://lkml.kernel.org/r/3d827e8b-a04e-0a93-4bb4-e0e9d59036da@redhat.com

Powered by blists - more mailing lists