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

[Harry Butterworth <harry@hebutterworth.freeserve.co.uk>]

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@hebutterworth.freeserve.co.uk>