Re: [Xen-devel] [v5][PATCH 1/2] xen:x86:mm:p2m: introduce set_identity_p2m_entry

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.07.14 at 08:40, <tiejun.chen@xxxxxxxxx> wrote:
> +int set_identity_p2m_entry(struct domain *d, unsigned long gfn)
> +{
> +    p2m_type_t p2mt;
> +    p2m_access_t a;
> +    mfn_t tmp_mfn, mfn = _mfn(gfn);

No need for the mfn variable; instead what currently is tmp_mfn
should be named just mfn, and the _mfn(gfn) construction can be
done right in the function call.

> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int ret = -EBUSY;
> +
> +    gfn_lock(p2m, gfn, 0);
> +
> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
> +
> +    if ( mfn_valid(tmp_mfn) )
> +    {
> +        gdprintk(XENLOG_ERR,
> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);

Pointless cast: Just use %lx in the format string. Additionally I don't
think the message text is correct: You don't really know whether
what's there is another RMRR (or that the context you're being
called in refers to an RMRR at all). On the contrary - if it was an
RMRR (or to be precise, a previously established identity mapping),
you'd want to report success. And generally we have no stop at
the end of log messages.

> +        goto out;

Once again, when error handling is that simple please avoid using
"goto".

Jan


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