Patchwork [2/3] qga: add guest-get-time command

login
register
mail settings
Submitter Lei Li
Date Jan. 6, 2013, 10:06 a.m.
Message ID <1357466820-12860-3-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/209732/
State New
Headers show

Comments

Lei Li - Jan. 6, 2013, 10:06 a.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   12 ++++++++++++
 qga/qapi-schema.json |   17 +++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
Eric Blake - Jan. 7, 2013, 10:04 p.m.
On 01/06/2013 03:06 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   12 ++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 

> +++ b/qga/qapi-schema.json
> @@ -100,6 +100,23 @@
>               'utc-offset': 'int' } }
>  
>  ##
> +# @guest-get-time:
> +#
> +# Get the information about host time in UTC and the
> +# UTC offset.

About the host time, or about the guest time?  In other words, doesn't
this command exist for the host to ask the guest what time the _guest_
thinks it is, so that the host can then decide whether to issue a
followup command to tell the guest to adjust its time?

> +#
> +# This command tries to get the host time which is
> +# presumably correct, since need to be able to resynchronize
> +# clock to host in guest.
> +#
> +# Returns: @HostTimeInfo on success.

For that matter, should we name the type in patch 1/3 'TimeInfo',
instead of 'HostTimeInfo', as it is not intrinsically tied to host or
guest, but more a function of who is being queried?
Luiz Capitulino - Jan. 9, 2013, 1:33 p.m.
On Sun,  6 Jan 2013 18:06:59 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   12 ++++++++++++
>  qga/qapi-schema.json |   17 +++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 26b0fa0..190199d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -109,6 +109,18 @@ static HostTimeInfo *get_host_time(void)
>      return host_time;
>  }
>  
> +struct HostTimeInfo *qmp_guest_get_time(Error **errp)
> +{
> +    HostTimeInfo *host_time = get_host_time();

The command is called guest_get_time() and runs in the guest, but it returns
HostTimeInfo. Is this correct?

> +
> +    if (!host_time) {
> +        error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to get host time");
> +        return NULL;
> +    }
> +
> +    return host_time;
> +}
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 7793aff..4a8b93c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -100,6 +100,23 @@
>               'utc-offset': 'int' } }
>  
>  ##
> +# @guest-get-time:
> +#
> +# Get the information about host time in UTC and the
> +# UTC offset.
> +#
> +# This command tries to get the host time which is
> +# presumably correct, since need to be able to resynchronize
> +# clock to host in guest.
> +#
> +# Returns: @HostTimeInfo on success.
> +#
> +# Since 1.4
> +##
> +{ 'command': 'guest-get-time',
> +  'returns': 'HostTimeInfo' }
> +
> +##
>  # @GuestAgentCommandInfo:
>  #
>  # Information about guest agent commands.
Lei Li - Jan. 11, 2013, 7:37 a.m.
On 01/08/2013 06:04 AM, Eric Blake wrote:
> On 01/06/2013 03:06 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   12 ++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> +++ b/qga/qapi-schema.json
>> @@ -100,6 +100,23 @@
>>                'utc-offset': 'int' } }
>>   
>>   ##
>> +# @guest-get-time:
>> +#
>> +# Get the information about host time in UTC and the
>> +# UTC offset.
> About the host time, or about the guest time?  In other words, doesn't
> this command exist for the host to ask the guest what time the _guest_
> thinks it is, so that the host can then decide whether to issue a
> followup command to tell the guest to adjust its time?

No, this command is for getting host time. You might want to take a look at
the RFC and the reply from Mike I sent few days ago for suggestions and
discussions.

http://article.gmane.org/gmane.comp.emulators.qemu/186126

>> +#
>> +# This command tries to get the host time which is
>> +# presumably correct, since need to be able to resynchronize
>> +# clock to host in guest.
>> +#
>> +# Returns: @HostTimeInfo on success.
> For that matter, should we name the type in patch 1/3 'TimeInfo',
> instead of 'HostTimeInfo', as it is not intrinsically tied to host or
> guest, but more a function of who is being queried?

Yes, it make sense. Luiz feel confused about this 'HostTimeInfo' too,
I think 'TimeInfo' might be a good idea. :)
Lei Li - Jan. 11, 2013, 7:50 a.m.
On 01/09/2013 09:33 PM, Luiz Capitulino wrote:
> On Sun,  6 Jan 2013 18:06:59 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qga/commands-posix.c |   12 ++++++++++++
>>   qga/qapi-schema.json |   17 +++++++++++++++++
>>   2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 26b0fa0..190199d 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -109,6 +109,18 @@ static HostTimeInfo *get_host_time(void)
>>       return host_time;
>>   }
>>   
>> +struct HostTimeInfo *qmp_guest_get_time(Error **errp)
>> +{
>> +    HostTimeInfo *host_time = get_host_time();
> The command is called guest_get_time() and runs in the guest, but it returns
> HostTimeInfo. Is this correct?

Okay, looks like this 'HostTimeInfo' brings a lots of confusion. I will change it
to 'TimeInfo' as I replied to another patch.

>> +
>> +    if (!host_time) {
>> +        error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to get host time");
>> +        return NULL;
>> +    }
>> +
>> +    return host_time;
>> +}
>> +
>>   typedef struct GuestFileHandle {
>>       uint64_t id;
>>       FILE *fh;
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index 7793aff..4a8b93c 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -100,6 +100,23 @@
>>                'utc-offset': 'int' } }
>>   
>>   ##
>> +# @guest-get-time:
>> +#
>> +# Get the information about host time in UTC and the
>> +# UTC offset.
>> +#
>> +# This command tries to get the host time which is
>> +# presumably correct, since need to be able to resynchronize
>> +# clock to host in guest.
>> +#
>> +# Returns: @HostTimeInfo on success.
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'guest-get-time',
>> +  'returns': 'HostTimeInfo' }
>> +
>> +##
>>   # @GuestAgentCommandInfo:
>>   #
>>   # Information about guest agent commands.
Michael Roth - Jan. 11, 2013, 5:28 p.m.
On Fri, Jan 11, 2013 at 03:37:26PM +0800, Lei Li wrote:
> On 01/08/2013 06:04 AM, Eric Blake wrote:
> >On 01/06/2013 03:06 AM, Lei Li wrote:
> >>Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >>---
> >>  qga/commands-posix.c |   12 ++++++++++++
> >>  qga/qapi-schema.json |   17 +++++++++++++++++
> >>  2 files changed, 29 insertions(+), 0 deletions(-)
> >>
> >>+++ b/qga/qapi-schema.json
> >>@@ -100,6 +100,23 @@
> >>               'utc-offset': 'int' } }
> >>  ##
> >>+# @guest-get-time:
> >>+#
> >>+# Get the information about host time in UTC and the
> >>+# UTC offset.
> >About the host time, or about the guest time?  In other words, doesn't
> >this command exist for the host to ask the guest what time the _guest_
> >thinks it is, so that the host can then decide whether to issue a
> >followup command to tell the guest to adjust its time?
> 
> No, this command is for getting host time. You might want to take a look at
> the RFC and the reply from Mike I sent few days ago for suggestions and
> discussions.
> 
> http://article.gmane.org/gmane.comp.emulators.qemu/186126

As mentioned elsewhere, what your guest-get-time implementation is actually
returning is the guest time. But that's exactly what we want, since it
provides all the information the host needs to calculate what value it
should pass to guest-set-time (really we just need the utc-offset for this,
but the other values are useful for other use cases. In fact, if you
document guest-*-time to always return values relative to UTC 1970 epoch, we
don't even actually need *that* unless we need to change the guest utc-offset
for some reason, but again, nice to handle other use cases) 

> 
> >>+#
> >>+# This command tries to get the host time which is
> >>+# presumably correct, since need to be able to resynchronize
> >>+# clock to host in guest.
> >>+#
> >>+# Returns: @HostTimeInfo on success.
> >For that matter, should we name the type in patch 1/3 'TimeInfo',
> >instead of 'HostTimeInfo', as it is not intrinsically tied to host or
> >guest, but more a function of who is being queried?
> 
> Yes, it make sense. Luiz feel confused about this 'HostTimeInfo' too,
> I think 'TimeInfo' might be a good idea. :)

Fourth'd :)

> 
> 
> 
> -- 
> Lei
>

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 26b0fa0..190199d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -109,6 +109,18 @@  static HostTimeInfo *get_host_time(void)
     return host_time;
 }
 
+struct HostTimeInfo *qmp_guest_get_time(Error **errp)
+{
+    HostTimeInfo *host_time = get_host_time();
+
+    if (!host_time) {
+        error_set(errp, QERR_QGA_COMMAND_FAILED, "Failed to get host time");
+        return NULL;
+    }
+
+    return host_time;
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 7793aff..4a8b93c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -100,6 +100,23 @@ 
              'utc-offset': 'int' } }
 
 ##
+# @guest-get-time:
+#
+# Get the information about host time in UTC and the
+# UTC offset.
+#
+# This command tries to get the host time which is
+# presumably correct, since need to be able to resynchronize
+# clock to host in guest.
+#
+# Returns: @HostTimeInfo on success.
+#
+# Since 1.4
+##
+{ 'command': 'guest-get-time',
+  'returns': 'HostTimeInfo' }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.