Re: [Xen-devel] [PATCH v5 02/10] IOMMU: handle IOMMU mapping and unmapping failures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
> No spamming can occur.

May I suggest "No spamming of the log can occur", to set some
context for what follows?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -240,21 +240,49 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
> unsigned long mfn,
>                     unsigned int flags)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
> +    int rc;
>  
>      if ( !iommu_enabled || !hd->platform_ops )
>          return 0;
>  
> -    return hd->platform_ops->map_page(d, gfn, mfn, flags);
> +    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> +
> +    if ( unlikely(rc) )

A more general word on the use of blank lines: I think their use is
well advised to separate logically (mostly) independent steps. In
cases like this, where you check the return value of a function,
the two things really belong together imo. Using too little blank
lines negatively affects readability, but using too many easily
leads to not seeing enough context anymore when looking at
some code fragment.

> +    {
> +        if ( !d->is_shutting_down && printk_ratelimit() )
> +            printk(XENLOG_ERR
> +                   "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",

I really dislike having to repeat this yet another time: No stops in
log messages please. Also to the reader of the log it would be
unclear what the number at the end represents. May I suggest

            printk(XENLOG_ERR
                   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d",
                   d->domain_id, gfn, mfn, rc);

(arguably one might then also remove the words "gfn" and "mfn").

Apart from these cosmetic issues I think this is fine now.

Jan


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