Re: [Xen-devel] [PATCH] libxl: support for "rtc_timeoffset" and "localtime"

[Lin Ming <mlin@xxxxxxxxxxxxx>]

On Mon, Mar 19, 2012 at 4:58 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Sun, 2012-03-18 at 11:50 +0000, Lin Ming wrote:
>> Implement "rtc_timeoffset" and "localtime" options compatible as xm.
>>
>> rtc_timeoffset is the offset between host time and guest time.
>> localtime means to specify whether the emulted RTC appears as UTC or is
>> offset by the host.
>>
>> Signed-off-by: Lin Ming <mlin@xxxxxxxxxxxxx
>
> Thanks, this look good!
>
> A few comments:
>
>> ---
>>  tools/libxl/libxl_dom.c     |    3 +++
>>  tools/libxl/libxl_types.idl |    1 +
>>  tools/libxl/xl_cmdimpl.c    |   13 +++++++++++++
>>  tools/libxl/xl_sxp.c        |    1 +
>>  4 files changed, 18 insertions(+), 0 deletions(-)
>
> Please also patch docs/man/xl.cfg.pod.5 to document the newly supported
> options

OK.

>
>> [...]
>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 1d59b89..b080a2b 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -697,6 +697,19 @@ static void parse_config_data(const char 
>> *configfile_filename_report,
>>          }
>>      }
>>
>> +    if (!xlu_cfg_get_long(config, "rtc_timeoffset", &l, 0))
>> +        b_info->rtc_timeoffset = l;
>> +
>> +    if (!xlu_cfg_get_long(config, "localtime", &l, 0) && l) {
>
> I wonder if we ought to include a "bool localtime" in the libxl API and
> move this stuff there. If we think this logic would be generally useful
> to other toolstack then we should do this.

I think it's useful.
Will add it.

>
>> +        time_t t;
>> +        struct tm *tm;
>> +
>> +        t = time(NULL);
>> +        tm = localtime(&t);
>> +
>> +        b_info->rtc_timeoffset -= tm->tm_gmtoff;
>> +    }
>> +
>>      if (!xlu_cfg_get_long (config, "videoram", &l, 0))
>>          b_info->video_memkb = l * 1024;
>>
>> diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
>> index c68b6df..bb1f3da 100644
>> --- a/tools/libxl/xl_sxp.c
>> +++ b/tools/libxl/xl_sxp.c
>> @@ -92,6 +92,7 @@ void printf_info_sexp(int domid, libxl_domain_config 
>> *d_config)
>>          printf("\t\t\t(firmware %s)\n", b_info->u.hvm.firmware);
>>          printf("\t\t\t(video_memkb %"PRId64")\n", b_info->video_memkb);
>>          printf("\t\t\t(shadow_memkb %"PRId64")\n", b_info->shadow_memkb);
>> +        printf("\t\t\t(rtc_timeoffset %"PRId32")\n", 
>> b_info->rtc_timeoffset);
>>          printf("\t\t\t(pae %s)\n", 
>> libxl_defbool_to_string(b_info->u.hvm.pae));
>>          printf("\t\t\t(apic %s)\n",
>>                 libxl_defbool_to_string(b_info->u.hvm.apic));
>
> Is this change compatible with xm's output?

No, will remove it.

>
> In general there is no need to update this file unless it has been shown
> to cause a compatibility issue with xm.

Thanks for the comments.
Lin Ming

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