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
|
Home