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

[Harry Butterworth <harry@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>]

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