Re: [Xen-devel] [RFC v3 5/6] xen/arm: Add log_dirty support for ARM

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-05-09 at 00:46 +0100, Andrew Cooper wrote:
> > +/* Allocate dirty bitmap resource */
> > +static int bitmap_init(struct domain *d)
> 
> This function name is far too generic.

It might be OK if it were in p2m.c rather than mm.c, or even better
would be to create logdirty.c.

> > +        if ( page == NULL )
> > +            goto cleanup_on_failure;
> > +
> > +        d->arch.dirty.bitmap[i] = 
> > map_domain_page_global(__page_to_mfn(page));
> 
> __map_domain_page_global(page) is your friend, and it can fail so needs
> checking.
> 
> How many pages is this?  global dompage mapping are scarce.

It's not so scarce on ARM32 as on x86, IIRC we have 2GB of domheap
space. Not to say we should be profligate with it, but it is less of a
concern than on x86 I think.

> > +int log_dirty_on(struct domain *d)
> > +{
> > +    if ( vlpt_init(d) || bitmap_init(d) )
> > +        return -EINVAL;
> 
> This hides -ENOMEM from each of the init functions.
> 
> I am a fan of
> 
> return vlpt_init(d) ?: bitmap_init(d);
> 
> As an easy way of chaining a set of functions together if they succeed. 
> Ian on the other hand isn't so I doubt you could get away with it.

:-)

I'd prefer:
        rc = foo_init(d)
        if (rc)
                return rc

I'd also be happy enough with if ((rc = foo_init(d))

Ian.


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