Re: [Xen-devel] [V1 PATCH 3/3] P2M error code propogation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.14 at 03:47, <mukesh.rathor@xxxxxxxxxx> wrote:
> @@ -690,20 +689,22 @@ long arch_do_domctl(
>          }
>          else
>          {
> +            int tmp_rc;

Blank line after declarations please.

>              printk(XENLOG_G_INFO
>                     "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
>                     d->domain_id, gfn, mfn, nr_mfns);
>  
>              if ( paging_mode_translate(d) )
>                  for ( i = 0; i < nr_mfns; i++ )
> -                    add |= !clear_mmio_p2m_entry(d, gfn + i);
> -            ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> -            if ( !ret && add )
> -                ret = -EIO;
> +                    if ( (tmp_rc = clear_mmio_p2m_entry(d, gfn + i)) )
> +                        ret = tmp_rc;
> +            tmp_rc = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> +            if ( !ret && tmp_rc )
> +                ret = tmp_rc;
>              if ( ret && is_hardware_domain(current->domain) )
>                  printk(XENLOG_ERR
>                         "memory_map: error %ld %s dom%d access to 
> [%lx,%lx]\n",
> -                       ret, add ? "removing" : "denying", d->domain_id,
> +                       ret, tmp_rc ? "denying" : "removing", d->domain_id,

This isn't functionally identical to the old code.

> @@ -753,7 +750,7 @@ set_mmio_p2m_entry(struct domain *d, unsigned long gfn, 
> mfn_t mfn)
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>  
>      if ( !paging_mode_translate(d) )
> -        return 0;
> +        return -EPERM;

I think EPERM isn't quite right here, the (elsewhere) previously resulting
EIO would seem more appropriate. Same further down obviously.

> @@ -1401,11 +1398,8 @@ long p2m_set_mem_access(struct domain *d, unsigned 
> long pfn, uint32_t nr,
>      for ( pfn += start; nr > start; ++pfn )
>      {
>          mfn = p2m->get_entry(p2m, pfn, &t, &_a, 0, NULL);
> -        if ( p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a) == 0 )
> -        {
> -            rc = -ENOMEM;
> +        if ( (rc = p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a)) )
>              break;

I think constructs like this, namely to the somewhat odd "requirement"
of needing to add an extra pair of parentheses around the assignment
to avoid a compiler warning, would benefit from being split into
assignment and separate if().

Jan


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