Re: [Xen-devel] [PATCH RFC 13/55] x86/mm: rewrite virt_to_xen_l3e

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.19 at 14:27, <wei.liu2@xxxxxxxxxx> wrote:
> On Mon, Apr 08, 2019 at 09:55:47AM -0600, Jan Beulich wrote:
>> >>> On 07.02.19 at 17:44, <wei.liu2@xxxxxxxxxx> wrote:
>> > @@ -4769,45 +4769,70 @@ void free_xen_pagetable_new(mfn_t mfn)
>> >  
>> >  static DEFINE_SPINLOCK(map_pgdir_lock);
>> >  
>> > +/*
>> > + * Given a virtual address, return a pointer to xen's L3 entry. Caller
>> > + * needs to unmap the pointer.
>> > + */
>> >  static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>> >  {
>> >      l4_pgentry_t *pl4e;
>> > +    l3_pgentry_t *pl3e = NULL;
>> >  
>> >      pl4e = &idle_pg_table[l4_table_offset(v)];
>> >      if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>> >      {
>> >          bool locking = system_state > SYS_STATE_boot;
>> > -        l3_pgentry_t *l3t = alloc_xen_pagetable();
>> > +        l3_pgentry_t *l3t;
>> > +        mfn_t mfn;
>> > +
>> > +        mfn = alloc_xen_pagetable_new();
>> > +        if ( mfn_eq(mfn, INVALID_MFN) )
>> > +            goto out;
>> 
>> Any reason not to use "return NULL" here, avoiding the goto
>> and label altogether in tis function?
> 
> This can be done, but I wanted to have all virt_to_xen_lXe have the same
> structure.

I sort of guessed this but for both this and ...

>> >  static l2_pgentry_t *virt_to_xen_l2e(unsigned long v)
>> >  {
>> >      l3_pgentry_t *pl3e;
>> > +    l2_pgentry_t *pl2e = NULL;
>> >  
>> >      pl3e = virt_to_xen_l3e(v);
>> >      if ( !pl3e )
>> > -        return NULL;
>> > +        goto out;
>> 
>> Why not keep this one the way it is?
> 
> Because there will be an out label anyway, why not use it to have a
> single exit path?

.. this: To me, the fewer goto-s the better, especially when - like
here - they can be replaced with just a single (non-block) statement.
But yes, this is personal taste ...

Jan



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