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

[Andrew Kane <Andrew.Kane@xxxxxxxxxxxxxxx>]

Dario,

On Jul 24, 2012, at 2:31 PM, Dario Faggioli wrote:

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

Oops. That's what I get for trusting the editor with comments. =)

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

Our thought was to define this following the structure that exists for the
other schedulers, both for consistency and to facilitate future work
on the ARINC 653 scheduler.

If/when we actually need domain-specific configuration like this,
it would only involve changes in the sched_arinc653_domain_set
function, and wouldn't require any changes to
libxl_domain_sched_params_set.

If the preference is to hold off on implementing a
sched_arinc653_domain_set function until there's actually something
to put in it, I'm happy to change it. =)

> 
>> 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)
> 
> 
Regards,
- Andrew

-- 
Andrew Kane
Computer Engineering Intern, DornerWorks Ltd.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel