Re: [XenPPC] [PATCH 3 of 6] [PATCH] xen: implement guest_physmap_max_mem for ppc

[Ryan Harper <ryanh@xxxxxxxxxx>]

* Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx> [2007-02-22 07:40]:
> >>+int do_guest_physmap_max_mem(struct domain *d, unsigned long new_max)
> >>+{
> >>+    ulong *p2m_array = NULL;
> >>+    ulong *p2m_old = NULL;
> >>+    ulong p2m_pages;
> >>+    ulong copy_end = 0;
> >>+
> >>+    /* we don't handle shrinking max_pages */
> >>+    if ( new_max < d->max_pages )
> >>+    {
> >>+        printk("Can't shrink %d max_mem\n", d->domain_id);
> >>  
> >
> >Just some wording, but maybe printk("Can't shrink max_mem of domain 
> >%d\n", d->domain_id); would prevent some users from mis-interpreting 
> >the number as an unit of memory size.

Yeah, definitely worth being more specific.

> >>+    /* free old p2m array if present */
> >>+    if ( p2m_old )
> >>+        free_xenheap_pages(d->arch.p2m, 
> >>get_order_from_pages(d->max_pages));
> >>  
> >Maybe I just don't get it right because I'm new in this area, but if 
> >an old mapping exists you do here summarized:
> >1. save old d->arch.p2m in p2m_old
> >2. create a new p2m_array including the old mapping as copy
> >3. assigning the new array to d->arch.p2m
> >4. ?? if an old mapping was present you free the new one in 
> >d->arch.p2m ??
> >=> this would leave one unreferenced heap allocation in memory 
> >(p2m_old) without a chance to free it after the local variable p2m_old 
> >disappears and the actively used table d->arch.p2m point to freed heap.
> >I assume the freeing code should be
> >
> >+    /* free old p2m array if present */
> >+    if ( p2m_old )
> >+        free_xenheap_pages(p2m_old, get_order_from_pages(d->max_pages));

Yep, good catch.  Thanks.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@xxxxxxxxxx

_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel