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

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.

Signed-off-by: Andrew Kane <Andrew.Kane@dornerworks.com>

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.
+    return 0;
+}
+
 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;
     default:
         LOG(ERROR, "Unknown scheduler");
         ret=ERROR_INVAL;

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)




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

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.

On Tue, 2012-07-24 at 15:09 -0400, Andrew Kane wrote: 
> >> +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. =)
> 
:-)
> 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.
> 
Yeah, I got that, and it''s not bad thinking actually.

Thinking a bit more about this, right below
libxl_domain_sched_params_set() (in libxl.c) there is another function
called libxl_domain_sched_params_get(), doing pretty much the same
thing, although of course it retrieves instead of setting.

Shouldn''t you be doing something similar to that too?
> 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. =)
> 
It''s mostly a matter of taste I guess.

The way I pointed is my preference, but I really don''t care that much.
If you send a patch with proper commenting (and perhaps dealing with the
*_get() path), I''ll ack it no matter if you have those empty functions
or not... Which will then mean it''ll be up to Goerge and Ian (added to
the Cc list) to decide what they like better. :-)

Thanks and 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)




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

On Tue, 2012-07-24 at 23:48 +0100, Dario Faggioli wrote:
> On Tue, 2012-07-24 at 15:09 -0400, Andrew Kane wrote: 
> > >> +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. =)
> > 
> :-)
> 
> > 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.
> > 
> Yeah, I got that, and it''s not bad thinking actually.
> 
> Thinking a bit more about this, right below
> libxl_domain_sched_params_set() (in libxl.c) there is another function
> called libxl_domain_sched_params_get(), doing pretty much the same
> thing, although of course it retrieves instead of setting.
> 
> Shouldn''t you be doing something similar to that too?
> 
> > 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. =)
> > 
> It''s mostly a matter of taste I guess.
> 
> The way I pointed is my preference, but I really don''t care that
much.
> If you send a patch with proper commenting (and perhaps dealing with the
> *_get() path), I''ll ack it no matter if you have those empty
functions
> or not... Which will then mean it''ll be up to Goerge and Ian
(added to
> the Cc list) to decide what they like better. :-)
I''m happy with whichever style Andrew as author can be bothered to type
in ;-)

Ian.
> 
> Thanks and Regards,
> Dario
>

On 24/07/12 23:48, Dario Faggioli wrote:
> On Tue, 2012-07-24 at 15:09 -0400, Andrew Kane wrote:
>>>> +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.
=)
>>
> :-)
>
>> 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.
>>
> Yeah, I got that, and it''s not bad thinking actually.
>
> Thinking a bit more about this, right below
> libxl_domain_sched_params_set() (in libxl.c) there is another function
> called libxl_domain_sched_params_get(), doing pretty much the same
> thing, although of course it retrieves instead of setting.
>
> Shouldn''t you be doing something similar to that too?
>
>> 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. =)
>>
> It''s mostly a matter of taste I guess.
>
> The way I pointed is my preference, but I really don''t care that
much.
> If you send a patch with proper commenting (and perhaps dealing with the
> *_get() path), I''ll ack it no matter if you have those empty
functions
> or not... Which will then mean it''ll be up to Goerge and Ian
(added to
> the Cc list) to decide what they like better. :-)
Yeah, the comment needs to be changed to C-style, but I don''t think 
calling a function or not is worth the trouble of talking about. :-)

  -George