Re: [Xen-devel] [PATCH v1] x86/mm: Make change_type_range return error

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 25.04.2019 15:54, Jan Beulich wrote:
>>>> On 24.04.19 at 16:46, <roger.pau@xxxxxxxxxx> wrote:
>> On Wed, Apr 24, 2019 at 02:27:32PM +0000, Alexandru Stefan ISAILA wrote:
>>> @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain 
>>> *p2m,
>>>        * This should be revisited later, but for now post a warning.
>>>        */
>>>       if ( unlikely(end > host_max_pfn) )
>>> -    {
>>> -        printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to 
>>> max_mapped_pfn\n",
>>> -               d->domain_id);
>>> -        end = invalidate_end = host_max_pfn;
>>> -    }
>>> +        return -EINVAL;
>>>   
>>>       /* If the requested range is out of scope, return doing nothing. */
>>>       if ( start > end )
>>> -        return;
>>> +        return 0;
>>
>> Since you are already changing the behavior of the function above this
>> should also return EINVAL IMO.
> 
> I don't think I agree. Quite the other way around: In the latter
> case it's simply an empty range that gets requested, which is a
> no-op (and hence no reason to fail). Avoiding empty ranges in
> the callers may result in less readable code there.
> 
> Even in the former case I don't think returning an error is
> appropriate, the more that the comment there says this is
> probably not the right behavior. I think it would be better to
> leave this alone until we have settled on what the right
> behavior here is. 

I guess George may have a say here to clarify the right behavior.

> It is an issue anyway that a change is
> made without saying why the new behavior preferable over
> the current one.

So is there a way to continue with this?

> 
> In any event the comment there would become stale with the
> removal of the printk().

I will change the comment.

Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel