[Qemu-devel] Changes since v2 -qga: Add `guest-get-timezone` command

[Qemu-devel] Changes since v2 -qga: Add `guest-get-timezone` command

[Vinzenz 'evilissimo' Feenstra]

Updated the documentation according to comments of eblake

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

[Vinzenz 'evilissimo' Feenstra]

From: Vinzenz Feenstra <vfeenstr@redhat.com>

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 <vfeenstr@redhat.com>
---
 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);
+    GTimeZone *tz = g_time_zone_new_local();
+    gint32 interval = g_time_zone_find_interval(tz, G_TIME_TYPE_STANDARD, 0);
+    gchar const *name = g_time_zone_get_abbreviation(tz, interval);
+    if (name != NULL) {
+        info->offset = g_time_zone_get_offset(tz, interval) / 60;
+        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' }
-- 
2.9.3

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

[sameeh jubran]

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 <vfeenstr@redhat.com>
>
> 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 <vfeenstr@redhat.com>
> ---
>   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
> +    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?
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.
> +        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' }

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

[Marc-André Lureau]

Hi

On Thu, Mar 23, 2017 at 4:41 PM sameeh jubran <sameeh@daynix.com> 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 <vfeenstr@redhat.com>
> >
> > 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 <vfeenstr@redhat.com>
> > ---
> >   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
> > +    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?
> 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?


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

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

[Vinzenz Feenstra]

> On Mar 23, 2017, at 1:51 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> 
> Hi
> 
> On Thu, Mar 23, 2017 at 4:41 PM sameeh jubran <sameeh@daynix.com <mailto:sameeh@daynix.com>> 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 <vfeenstr@redhat.com <mailto:vfeenstr@redhat.com>>
> >
> > 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 <vfeenstr@redhat.com <mailto:vfeenstr@redhat.com>>
> > ---
> >   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