WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer

To: Muli Ben-Yehuda <mulix@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider
From: Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Date: Mon, 21 Nov 2005 21:30:00 +0000
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Mon, 21 Nov 2005 21:25:45 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20051121201811.GB22728@xxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1132579125.31295.113.camel@xxxxxxxxxxxxxxxxxxxxx> <20051121201811.GB22728@xxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Mon, 2005-11-21 at 22:18 +0200, Muli Ben-Yehuda wrote:
> > +static int xenidc_buffer_resource_provider_init_or_exit
> > +    (xenidc_buffer_resource_provider * provider, int exit) {
> > +   trace1("%p", provider);
> > +
> > +   {
> 
> This function is way way too long.

It's straight line code doing initialisation.  Breaking it up would be
artificial.  I suppose I could split it up to have a separate init
function for each kind of resource.

> 
> > +           {
> 
> No internal blocks please.

Well, I agree that they don't work well with 8 space tabs.  So, while we
have the big tabs I guess they'll have to go.

> 
> > +           if (provider->resource_allocation.empty_page_ranges != 0) {
> > +                   provider->page = balloon_alloc_empty_page_range
> > +                       (provider->resource_allocation.empty_page_ranges
> > +                        *
> > +                        provider->resource_allocation.
> > +                        empty_page_range_page_count);
> 
> lindent brain damage (putting the '*' on its own line).
OK.
> 
> > +                   if (provider->page == NULL) {
> > +                           return_value = -ENOMEM;
> > +
> > +                           goto EXIT_NO_EMPTY_PAGE_RANGE;
> 
> common style is not to put the label in all uppercaps.
> 
OK.
> > +                   if (xenidc_buffer_resource_provider_init_or_exit
> > +                       (provider, 0)
> > +                       != 0) {
> > +                           vfree(provider);
> 
> using vmalloc/vfree is discouraged unless you must.

I don't understand this.  I thought vmalloc was more likely to be
successful than kmalloc because the memory doesn't need to be contiguous
so I thought it was preferable to use vmalloc when possible.

> 
> > +void xenidc_free_buffer_resource_provider
> > +    (xenidc_buffer_resource_provider * provider) {
> > +   trace();
> > +
> > +   (void)xenidc_buffer_resource_provider_init_or_exit(provider, 1);
> > +
> > +   vfree(provider);
> 
> If init_or_exit fails won't you vfree an already freed pointer here?

No, init_or_exit never fails on the exit path so the code is correct.

> 
> > +           xenidc_buffer_resource_list list;
> > +
> > +           unsigned long flags;
> > +
> > +           spin_lock_irqsave(&provider->lock, flags);
> > +
> > +           list = provider->free_resources;
> > +
> > +           spin_unlock_irqrestore(&provider->lock, flags);
> > +
> > +           return list;
> 
> You have a lot of empty lines. This function needs no empty lines, for
> example, except maybe after the variable declarations. Also, does
> 'list' need to be reference counted here?

I'm used to a lot of empty lines.  I can get rid of them all if you
like.

No, 'list' doesn't need to be reference counted.

> 
> > +typedef struct xenidc_buffer_resource_provider_struct
> > +    xenidc_buffer_resource_provider;
> 
> don't typedef structs.

OK.

> 
> Cheers,
> Muli
-- 
Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>


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