* [PATCH RFC] qga: fence guest-set-time if hwclock not available
@ 2019-11-28 12:36 Cornelia Huck
2019-11-28 12:45 ` Daniel P. Berrangé
0 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2019-11-28 12:36 UTC (permalink / raw)
To: Michael Roth; +Cc: Cornelia Huck, qemu-devel
The Posix implementation of guest-set-time invokes hwclock to
set/retrieve the time to/from the hardware clock. If hwclock
is not available, the user is currently informed that "hwclock
failed to set hardware clock to system time", which is quite
misleading. This may happen e.g. on s390x, which has a different
timekeeping concept anyway.
Let's check for the availability of the hwclock command and
return QERR_UNSUPPORTED for guest-set-time if it is not available.
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
Not sure if that is the correct approach, but the current error
message is really quite confusing.
Gave it a quick test with an s390x and an x86_64 guest; invoking
'virsh domtime <value>' now fails with 'not currently supported'
on s390x and continues to work as before on x86_64.
---
qga/commands-posix.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165daed8..bd298a38b716 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -149,6 +149,13 @@ int64_t qmp_guest_get_time(Error **errp)
return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
}
+static int check_hwclock_available(const char *path)
+{
+ struct stat st;
+
+ return (stat(path, &st) < 0) ? 0 : 1;
+}
+
void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
{
int ret;
@@ -156,6 +163,17 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
pid_t pid;
Error *local_err = NULL;
struct timeval tv;
+ const char *hwclock_path = "/sbin/hwclock";
+ static int hwclock_available = -1;
+
+ if (hwclock_available < 0) {
+ hwclock_available = check_hwclock_available(hwclock_path);
+ }
+
+ if (!hwclock_available) {
+ error_setg(errp, QERR_UNSUPPORTED);
+ return;
+ }
/* If user has passed a time, validate and set it. */
if (has_time) {
@@ -195,7 +213,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
/* Use '/sbin/hwclock -w' to set RTC from the system time,
* or '/sbin/hwclock -s' to set the system time from RTC. */
- execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
+ execle(hwclock_path, "hwclock", has_time ? "-w" : "-s",
NULL, environ);
_exit(EXIT_FAILURE);
} else if (pid < 0) {
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH RFC] qga: fence guest-set-time if hwclock not available
2019-11-28 12:36 [PATCH RFC] qga: fence guest-set-time if hwclock not available Cornelia Huck
@ 2019-11-28 12:45 ` Daniel P. Berrangé
2019-11-28 12:49 ` Cornelia Huck
2019-11-28 14:06 ` Laszlo Ersek
0 siblings, 2 replies; 5+ messages in thread
From: Daniel P. Berrangé @ 2019-11-28 12:45 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Michael Roth, qemu-devel
On Thu, Nov 28, 2019 at 01:36:58PM +0100, Cornelia Huck wrote:
> The Posix implementation of guest-set-time invokes hwclock to
> set/retrieve the time to/from the hardware clock. If hwclock
> is not available, the user is currently informed that "hwclock
> failed to set hardware clock to system time", which is quite
> misleading. This may happen e.g. on s390x, which has a different
> timekeeping concept anyway.
>
> Let's check for the availability of the hwclock command and
> return QERR_UNSUPPORTED for guest-set-time if it is not available.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>
> Not sure if that is the correct approach, but the current error
> message is really quite confusing.
I guess the alternative is to just #ifndef __s390x__ the whole
impl of the qmp_guest_set_time method, but I don't have a
strong opinion on which is best.
>
> Gave it a quick test with an s390x and an x86_64 guest; invoking
> 'virsh domtime <value>' now fails with 'not currently supported'
> on s390x and continues to work as before on x86_64.
>
> ---
> qga/commands-posix.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1c1a165daed8..bd298a38b716 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -149,6 +149,13 @@ int64_t qmp_guest_get_time(Error **errp)
> return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
> }
>
> +static int check_hwclock_available(const char *path)
> +{
> + struct stat st;
> +
> + return (stat(path, &st) < 0) ? 0 : 1;
> +}
> +
> void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> {
> int ret;
> @@ -156,6 +163,17 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> pid_t pid;
> Error *local_err = NULL;
> struct timeval tv;
> + const char *hwclock_path = "/sbin/hwclock";
> + static int hwclock_available = -1;
> +
> + if (hwclock_available < 0) {
> + hwclock_available = check_hwclock_available(hwclock_path);
Could do this inline with:
hwclock_available = (access(hwclock_available, X_OK) == 0);
getting a slightly better result as this check for it being
executable as well as existing.
> + }
> +
> + if (!hwclock_available) {
> + error_setg(errp, QERR_UNSUPPORTED);
> + return;
> + }
>
> /* If user has passed a time, validate and set it. */
> if (has_time) {
> @@ -195,7 +213,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>
> /* Use '/sbin/hwclock -w' to set RTC from the system time,
> * or '/sbin/hwclock -s' to set the system time from RTC. */
> - execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
> + execle(hwclock_path, "hwclock", has_time ? "-w" : "-s",
> NULL, environ);
> _exit(EXIT_FAILURE);
> } else if (pid < 0) {
> --
> 2.21.0
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH RFC] qga: fence guest-set-time if hwclock not available
2019-11-28 12:45 ` Daniel P. Berrangé
@ 2019-11-28 12:49 ` Cornelia Huck
2019-11-28 14:41 ` Daniel P. Berrangé
2019-11-28 14:06 ` Laszlo Ersek
1 sibling, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2019-11-28 12:49 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Michael Roth, qemu-devel
On Thu, 28 Nov 2019 12:45:32 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Nov 28, 2019 at 01:36:58PM +0100, Cornelia Huck wrote:
> > The Posix implementation of guest-set-time invokes hwclock to
> > set/retrieve the time to/from the hardware clock. If hwclock
> > is not available, the user is currently informed that "hwclock
> > failed to set hardware clock to system time", which is quite
> > misleading. This may happen e.g. on s390x, which has a different
> > timekeeping concept anyway.
> >
> > Let's check for the availability of the hwclock command and
> > return QERR_UNSUPPORTED for guest-set-time if it is not available.
> >
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >
> > Not sure if that is the correct approach, but the current error
> > message is really quite confusing.
>
> I guess the alternative is to just #ifndef __s390x__ the whole
> impl of the qmp_guest_set_time method, but I don't have a
> strong opinion on which is best.
This hardcodes this as a s390x specialty, though, and I'm not sure that
assumption is and will stay correct.
>
> >
> > Gave it a quick test with an s390x and an x86_64 guest; invoking
> > 'virsh domtime <value>' now fails with 'not currently supported'
> > on s390x and continues to work as before on x86_64.
> >
> > ---
> > qga/commands-posix.c | 20 +++++++++++++++++++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 1c1a165daed8..bd298a38b716 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -149,6 +149,13 @@ int64_t qmp_guest_get_time(Error **errp)
> > return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
> > }
> >
> > +static int check_hwclock_available(const char *path)
> > +{
> > + struct stat st;
> > +
> > + return (stat(path, &st) < 0) ? 0 : 1;
> > +}
> > +
> > void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> > {
> > int ret;
> > @@ -156,6 +163,17 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> > pid_t pid;
> > Error *local_err = NULL;
> > struct timeval tv;
> > + const char *hwclock_path = "/sbin/hwclock";
> > + static int hwclock_available = -1;
> > +
> > + if (hwclock_available < 0) {
> > + hwclock_available = check_hwclock_available(hwclock_path);
>
> Could do this inline with:
>
> hwclock_available = (access(hwclock_available, X_OK) == 0);
>
> getting a slightly better result as this check for it being
> executable as well as existing.
Yes, that looks better. Can do if we agree on this approach.
>
> > + }
> > +
> > + if (!hwclock_available) {
> > + error_setg(errp, QERR_UNSUPPORTED);
> > + return;
> > + }
> >
> > /* If user has passed a time, validate and set it. */
> > if (has_time) {
> > @@ -195,7 +213,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> >
> > /* Use '/sbin/hwclock -w' to set RTC from the system time,
> > * or '/sbin/hwclock -s' to set the system time from RTC. */
> > - execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
> > + execle(hwclock_path, "hwclock", has_time ? "-w" : "-s",
> > NULL, environ);
> > _exit(EXIT_FAILURE);
> > } else if (pid < 0) {
> > --
> > 2.21.0
> >
> >
>
> Regards,
> Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH RFC] qga: fence guest-set-time if hwclock not available
2019-11-28 12:49 ` Cornelia Huck
@ 2019-11-28 14:41 ` Daniel P. Berrangé
0 siblings, 0 replies; 5+ messages in thread
From: Daniel P. Berrangé @ 2019-11-28 14:41 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Michael Roth, qemu-devel
On Thu, Nov 28, 2019 at 01:49:32PM +0100, Cornelia Huck wrote:
> On Thu, 28 Nov 2019 12:45:32 +0000
> Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> > On Thu, Nov 28, 2019 at 01:36:58PM +0100, Cornelia Huck wrote:
> > > The Posix implementation of guest-set-time invokes hwclock to
> > > set/retrieve the time to/from the hardware clock. If hwclock
> > > is not available, the user is currently informed that "hwclock
> > > failed to set hardware clock to system time", which is quite
> > > misleading. This may happen e.g. on s390x, which has a different
> > > timekeeping concept anyway.
> > >
> > > Let's check for the availability of the hwclock command and
> > > return QERR_UNSUPPORTED for guest-set-time if it is not available.
> > >
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > ---
> > >
> > > Not sure if that is the correct approach, but the current error
> > > message is really quite confusing.
> >
> > I guess the alternative is to just #ifndef __s390x__ the whole
> > impl of the qmp_guest_set_time method, but I don't have a
> > strong opinion on which is best.
>
> This hardcodes this as a s390x specialty, though, and I'm not sure that
> assumption is and will stay correct.
>
> >
> > >
> > > Gave it a quick test with an s390x and an x86_64 guest; invoking
> > > 'virsh domtime <value>' now fails with 'not currently supported'
> > > on s390x and continues to work as before on x86_64.
> > >
> > > ---
> > > qga/commands-posix.c | 20 +++++++++++++++++++-
> > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index 1c1a165daed8..bd298a38b716 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -149,6 +149,13 @@ int64_t qmp_guest_get_time(Error **errp)
> > > return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
> > > }
> > >
> > > +static int check_hwclock_available(const char *path)
> > > +{
> > > + struct stat st;
> > > +
> > > + return (stat(path, &st) < 0) ? 0 : 1;
> > > +}
> > > +
> > > void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> > > {
> > > int ret;
> > > @@ -156,6 +163,17 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> > > pid_t pid;
> > > Error *local_err = NULL;
> > > struct timeval tv;
> > > + const char *hwclock_path = "/sbin/hwclock";
> > > + static int hwclock_available = -1;
> > > +
> > > + if (hwclock_available < 0) {
> > > + hwclock_available = check_hwclock_available(hwclock_path);
> >
> > Could do this inline with:
> >
> > hwclock_available = (access(hwclock_available, X_OK) == 0);
> >
> > getting a slightly better result as this check for it being
> > executable as well as existing.
>
> Yes, that looks better. Can do if we agree on this approach.
I'm fine with this
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] qga: fence guest-set-time if hwclock not available
2019-11-28 12:45 ` Daniel P. Berrangé
2019-11-28 12:49 ` Cornelia Huck
@ 2019-11-28 14:06 ` Laszlo Ersek
1 sibling, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2019-11-28 14:06 UTC (permalink / raw)
To: Daniel P. Berrangé, Cornelia Huck; +Cc: Michael Roth, qemu-devel
On 11/28/19 13:45, Daniel P. Berrangé wrote:
> On Thu, Nov 28, 2019 at 01:36:58PM +0100, Cornelia Huck wrote:
>> The Posix implementation of guest-set-time invokes hwclock to
>> set/retrieve the time to/from the hardware clock. If hwclock
>> is not available, the user is currently informed that "hwclock
>> failed to set hardware clock to system time", which is quite
>> misleading. This may happen e.g. on s390x, which has a different
>> timekeeping concept anyway.
>>
>> Let's check for the availability of the hwclock command and
>> return QERR_UNSUPPORTED for guest-set-time if it is not available.
>>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>
>> Not sure if that is the correct approach, but the current error
>> message is really quite confusing.
>
> I guess the alternative is to just #ifndef __s390x__ the whole
> impl of the qmp_guest_set_time method, but I don't have a
> strong opinion on which is best.
>
>>
>> Gave it a quick test with an s390x and an x86_64 guest; invoking
>> 'virsh domtime <value>' now fails with 'not currently supported'
>> on s390x and continues to work as before on x86_64.
>>
>> ---
>> qga/commands-posix.c | 20 +++++++++++++++++++-
>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 1c1a165daed8..bd298a38b716 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -149,6 +149,13 @@ int64_t qmp_guest_get_time(Error **errp)
>> return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
>> }
>>
>> +static int check_hwclock_available(const char *path)
>> +{
>> + struct stat st;
>> +
>> + return (stat(path, &st) < 0) ? 0 : 1;
>> +}
>> +
>> void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>> {
>> int ret;
>> @@ -156,6 +163,17 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>> pid_t pid;
>> Error *local_err = NULL;
>> struct timeval tv;
>> + const char *hwclock_path = "/sbin/hwclock";
slight style / semantics improvement could be
static const char hwclock_path[] = "/sbin/hwclock";
Thanks
Laszlo
>> + static int hwclock_available = -1;
>> +
>> + if (hwclock_available < 0) {
>> + hwclock_available = check_hwclock_available(hwclock_path);
>
> Could do this inline with:
>
> hwclock_available = (access(hwclock_available, X_OK) == 0);
>
> getting a slightly better result as this check for it being
> executable as well as existing.
>
>> + }
>> +
>> + if (!hwclock_available) {
>> + error_setg(errp, QERR_UNSUPPORTED);
>> + return;
>> + }
>>
>> /* If user has passed a time, validate and set it. */
>> if (has_time) {
>> @@ -195,7 +213,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>>
>> /* Use '/sbin/hwclock -w' to set RTC from the system time,
>> * or '/sbin/hwclock -s' to set the system time from RTC. */
>> - execle("/sbin/hwclock", "hwclock", has_time ? "-w" : "-s",
>> + execle(hwclock_path, "hwclock", has_time ? "-w" : "-s",
>> NULL, environ);
>> _exit(EXIT_FAILURE);
>> } else if (pid < 0) {
>> --
>> 2.21.0
>>
>>
>
> Regards,
> Daniel
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-28 15:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-28 12:36 [PATCH RFC] qga: fence guest-set-time if hwclock not available Cornelia Huck
2019-11-28 12:45 ` Daniel P. Berrangé
2019-11-28 12:49 ` Cornelia Huck
2019-11-28 14:41 ` Daniel P. Berrangé
2019-11-28 14:06 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).