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

[Muli Ben-Yehuda <mulix@xxxxxxxxx>]

> +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.

> +             {

No internal blocks please.

> +             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).

> +                     if (provider->page == NULL) {
> +                             return_value = -ENOMEM;
> +
> +                             goto EXIT_NO_EMPTY_PAGE_RANGE;

common style is not to put the label in all uppercaps.

> +                     if (xenidc_buffer_resource_provider_init_or_exit
> +                         (provider, 0)
> +                         != 0) {
> +                             vfree(provider);

using vmalloc/vfree is discouraged unless you must.

> +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?

> +             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?

> +typedef struct xenidc_buffer_resource_provider_struct
> +    xenidc_buffer_resource_provider;

don't typedef structs.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


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