Re: [Xen-devel] [PATCH v3 1/2] kexec: use hypercall_create_continuation to protect KEXEC ops

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.04.17 at 12:42, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Apr 20, 2017 at 03:34:21AM -0600, Jan Beulich wrote:
>> >>> On 19.04.17 at 19:16, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Wed, Apr 19, 2017 at 10:19:44AM -0600, Jan Beulich wrote:
>> >> >>> On 19.04.17 at 17:54, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > On Wed, Apr 19, 2017 at 10:47:15AM -0500, Eric DeVolder wrote:
>> >> >> @@ -1193,6 +1194,9 @@ static int do_kexec_op_internal(unsigned long op,
>> >> >>      if ( ret )
>> >> >>          return ret;
>> >> >>
>> >> >> +    if ( test_and_set_bit(KEXEC_FLAG_IN_HYPERCALL, &kexec_flags) )
>> >> >> +        return hypercall_create_continuation(__HYPERVISOR_kexec_op, 
>> >> >> "lh", op, uarg);
>> >> >> +
>> >> >
>> >> > I would suggest here:
>> >> >        ASSERT(test_bit(KEXEC_FLAG_IN_HYPERCALL, &kexec_flags));
>> >>
>> >> You're kidding? The flag was set just in the line above. Or do you
>> >> really mean we need to consider test_and_set_bit() not doing what
>> >> it is supposed to do?
>> >
>> > Yep, it looks ridiculous. However, ASSERT() in kexec_swap_images() looks 
>> > almost
>> > the same for me. So, TBH, I still do not understand need for it at all. 
>> > Could
>> > you enlighten me?
>>
>> Can't be that difficult to understand: There was a lock there before,
>> and the addition of the ASSERT() could help document that the
>> serialization requirements aren't being broken. I'm not saying there
> 
> Ahhh... OK, so, you treat this more as a comment than anything else.
> So, why not use regular comment here then? Hmmm... Do you treat an
> ASSERT() here as kind of fuse too?

Well, in a way - other than a plain comment, the ASSERT() serves
both documentation purposes _and_ enforces what the comment
would otherwise only state.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel