Re: [Qemu-devel] [PATCH v2] qga: Add `guest-get-timezone` command

[Vinzenz Feenstra]

> On Mar 23, 2017, at 1:51 PM, Marc-André Lureau <address@hidden> wrote:
> 
> Hi
> 
> On Thu, Mar 23, 2017 at 4:41 PM sameeh jubran <address@hidden 
> <mailto:address@hidden>> wrote:
> I have tested the patch on Windows 2012 R2 and it seems to be working good.
> 
> Please see some comments inline.
> 
> 
> On 3/22/2017 7:16 PM, Vinzenz 'evilissimo' Feenstra wrote:
> > From: Vinzenz Feenstra <address@hidden <mailto:address@hidden>>
> >
> > Adds a new command `guest-get-timezone` reporting the currently
> > configured timezone on the system. The information on what timezone is
> > currently is configured is useful in case of Windows VMs where the
> > offset of the hardware clock is required to have the same offset. This
> > can be used for management systems like `oVirt` to detect the timezone
> > difference and warn administrators of the misconfiguration.
> >
> > Signed-off-by: Vinzenz Feenstra <address@hidden <mailto:address@hidden>>
> > ---
> >   qga/commands.c       | 19 +++++++++++++++++++
> >   qga/qapi-schema.json | 26 ++++++++++++++++++++++++++
> >   2 files changed, 45 insertions(+)
> >
> > diff --git a/qga/commands.c b/qga/commands.c
> > index 4d92946..83d7f99 100644
> > --- a/qga/commands.c
> > +++ b/qga/commands.c
> > @@ -499,3 +499,22 @@ int ga_parse_whence(GuestFileWhence *whence, Error 
> > **errp)
> >       error_setg(errp, "invalid whence code %"PRId64, whence->u.value);
> >       return -1;
> >   }
> > +
> > +GuestTimezone *qmp_guest_get_timezone(Error **errp)
> > +{
> > +    GuestTimezone *info = g_new0(GuestTimezone, 1);
> Check for null
> > +    GTimeZone *tz = g_time_zone_new_local();
> Check for null
> > +    gint32 interval = g_time_zone_find_interval(tz, G_TIME_TYPE_STANDARD, 
> > 0);
> Check if interval is -1

Will add the checks above in v3

> > +    gchar const *name = g_time_zone_get_abbreviation(tz, interval);
> > +    if (name != NULL) {
> > +        info->offset = g_time_zone_get_offset(tz, interval) / 60;
> g_time_zone_get_offset returns the offset in seconds, dividing by 60
> will give us the offset in minutes.
> Is there a reason to display it in minutes instead of hours?

Yes, there are offsets that are in minutes, some are 45 minutes, some 30 
minutes so hours
aren’t enough.

> Either way I think you should display the units in the output.
> 
> And 60 is a magic number, Even though it is very clear, I think it is
> better to add a define for that or check if glib already has one.
> 
> Instead, why not follow glib API and return seconds?

In an earlier iteration I did use a Windows API function and the offset value 
in Windows is returned
in minutes. See here the documentation for the  ‘Bias’ fields:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms725481(v=vs.85).aspx 
<https://msdn.microsoft.com/en-us/library/windows/desktop/ms725481(v=vs.85).aspx>

That was for me the main motivation, plus the seconds granularity is not 
required for the offsets,
minutes is granular enough. However I could change it to seconds of course, 
seems more
reasonable in the light of the glib support for it.


>  
> > +        info->zone = g_strdup(name);
> > +    } else {
> > +        error_setg(errp, "Timezone lookup failed");
> > +        g_free(info);
> > +        info = NULL;
> > +    }
> > +    g_time_zone_unref(tz);
> > +    return info;
> > +}
> > +
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index a02dbf2..62d6909 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -1042,3 +1042,29 @@
> >     'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
> >                  '*input-data': 'str', '*capture-output': 'bool' },
> >     'returns': 'GuestExec' }
> > +
> > +
> > +##
> > +# @GuestTimezone:
> > +#
> > +# @zone:    Timezone name
> > +# @offset:  Offset to UTC in minutes. For western timezones the offset has 
> > a
> > +#           negative value and for eastern the offset is positive value
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'struct': 'GuestTimezone',
> > +  'data':   { 'zone': 'str', 'offset': 'int' } }
> > +
> > +
> > +##
> > +# @guest-get-timezone:
> > +#
> > +# Retrieves the timezone information from the guest.
> > +#
> > +# Returns: A GuestTimezone dictionary.
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'command': 'guest-get-timezone',
> > +  'returns': 'GuestTimezone' }
> 
> -- 
> Marc-André Lureau