Re: [Xen-devel] [PATCH] libxl: libxl_domain_sched_params_set case for ARINC 653 scheduler

[Dario Faggioli <raistlin@xxxxxxxx>]

Hi Andrew,

On Tue, 2012-07-24 at 12:59 -0400, Andrew Kane wrote: 
> Implements sched_arinc653_domain_set to match the existing API. Currently,
> there is no domain-specific configuration when using the ARINC 653 scheduler,
> so we simply return success.
> 
Cool, thanks for doing this. I've only a small question...

> diff -r 4a28c496acbf -r 857a035d6a4a tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Mon Jul 23 17:58:33 2012 +0100
> +++ b/tools/libxl/libxl.c     Tue Jul 24 11:41:39 2012 -0400
> @@ -3642,6 +3642,14 @@ libxl_scheduler libxl_get_scheduler(libx
>      return sched;
>  }
>  
> +static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid,
> +                                     const libxl_domain_sched_params *scinfo)
> +{
> +    // Currently, the ARINC 653 scheduler does not take any domain-specific
> +    //     configuration, so we simply return success.
>
I think using C (/* */) style for comment is highly recommended, if not
required. :-)

> +    return 0;
> +}
> +
>
It's certainly not a bit deal, and the compiler might be doing this on
its own already but, as the function doesn't actually do anything, why
not putting the comment down in the switch [*]?

That way we still avoid LIBXL_SCHEDULING_ARNIC653 being caught by the
default: clause and resulting in an error, but without introducing an
empty function that might look confusing.

If in future there will be the need for a specific
sched_arnic653_domain_set function, we can add it then.

> static int sched_credit_domain_get(libxl__gc *gc, uint32_t domid,
>                                     libxl_domain_sched_params *scinfo)
>  {
> @@ -3909,6 +3917,9 @@ int libxl_domain_sched_params_set(libxl_
>      case LIBXL_SCHEDULER_CREDIT2:
>          ret=sched_credit2_domain_set(gc, domid, scinfo);
>          break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +        ret=sched_arinc653_domain_set(gc, domid, scinfo);
> +        break;
>
[*] I mean right here... :-)

> default:
>          LOG(ERROR, "Unknown scheduler");
>          ret=ERROR_INVAL;
> 
Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel