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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On May 23, 2016 9:41 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> 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?
> 

Make sense.

> > --- 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.
> 

I will also apply it to other patches.

> > +    {
> > +        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").
> 

To me, we are better not to remove 'gfn' / 'mfn', but I really need to add a 
'\n'.. then

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


Quan

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


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