Re: [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/12/9 16:19, Jan Beulich wrote:
>>>> On 09.12.14 at 08:47, <tiejun.chen@intel.com> wrote:
>> On 2014/12/8 16:51, Jan Beulich wrote:
>>> The whole "if-copy-unlock-and-return-EFAULT-otherwise-increment"
>>> is identical and can be factored out pretty easily afaict.
>>
>> What about this?
>>
>> struct get_reserved_device_memory {
>>       struct xen_reserved_device_memory_map map;
>>       unsigned int used_entries;
>>       struct domain *domain;
>> };
>>
>> static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
>>                                         u32 id, void *ctxt)
>> {
>>       struct get_reserved_device_memory *grdm = ctxt;
>>       struct domain *d = grdm->domain;
>>       unsigned int i, hit_one = 0;
>>       u32 sbdf;
>>       struct xen_reserved_device_memory rdm = {
>>           .start_pfn = start, .nr_pages = nr
>>       };
>>
>>       if ( !d->arch.hvm_domain.pci_force )
>>       {
>>           for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
>>           {
>>               sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg,
>>                                d->arch.hvm_domain.pcidevs[i].bus,
>>                                d->arch.hvm_domain.pcidevs[i].devfn);
>>               if ( sbdf == id )
>>               {
>>                   hit_one = 1;
>>                   break;
>>               }
>>           }
>>
>>           if ( !hit_one )
>>               return 0;
>>       }
>
> Why do you always pick other than the simplest possible solution?

I don't intend it to be, but I may go a complicated way, even a wrong 
way, based on my understanding. But as one main maintainer, if you 
always say to me in such a reproachful word more than once, I have to 
consider you may hint constantly I'm not a suitable candidate to finish 
this. Its fair to me, I'd really like to quit this to ask my manager if 
it can deliver to other guy to make sure this can move forward.

> You don't need a separate variable here, you can simply check
> whether i reached d->arch.hvm_domain.num_pcidevs after the
> loop. And even if you added a variable, it would want to be a

Are you saying this?

	if ( i == d->arch.hvm_domain.num_pcidevs )
		return 0;

But if the last one happens to one hit, 'i' is equal to 
d->arch.hvm_domain.num_pcidevs.

Thanks
Tiejun

> bool_t one with the way you use it.
>
> Jan
>
>
>