* [PATCH v2] qga: fence guest-set-time if hwclock not available
@ 2019-11-28 18:11 Cornelia Huck
2019-11-28 18:38 ` Laszlo Ersek
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-11-28 18:11 UTC (permalink / raw)
To: Michael Roth
Cc: Laszlo Ersek, Cornelia Huck, Daniel P . Berrangé, 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>
---
v1 (RFC) -> v2:
- use hwclock_path[]
- use access() instead of stat()
---
qga/commands-posix.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165daed8..ffb6420fa9cd 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -156,6 +156,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 = (access(hwclock_path, X_OK) == 0);
+ }
+
+ if (!hwclock_available) {
+ error_setg(errp, QERR_UNSUPPORTED);
+ return;
+ }
/* If user has passed a time, validate and set it. */
if (has_time) {
@@ -195,7 +206,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] 6+ messages in thread
* Re: [PATCH v2] qga: fence guest-set-time if hwclock not available
2019-11-28 18:11 [PATCH v2] qga: fence guest-set-time if hwclock not available Cornelia Huck
@ 2019-11-28 18:38 ` Laszlo Ersek
2019-11-29 7:56 ` Cornelia Huck
2019-11-29 9:35 ` Daniel P. Berrangé
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2019-11-28 18:38 UTC (permalink / raw)
To: Cornelia Huck, Michael Roth; +Cc: Daniel P . Berrangé, qemu-devel
Hi Cornelia,
On 11/28/19 19:11, 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>
> ---
>
> v1 (RFC) -> v2:
> - use hwclock_path[]
> - use access() instead of stat()
>
> ---
> qga/commands-posix.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1c1a165daed8..ffb6420fa9cd 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -156,6 +156,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";
Did you drop the "static" storage-class specifier on purpose?
> + static int hwclock_available = -1;
> +
> + if (hwclock_available < 0) {
> + hwclock_available = (access(hwclock_path, X_OK) == 0);
> + }
> +
> + if (!hwclock_available) {
> + error_setg(errp, QERR_UNSUPPORTED);
> + return;
> + }
>
> /* If user has passed a time, validate and set it. */
> if (has_time) {
> @@ -195,7 +206,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",
I think it's somewhat obscure now that arg="hwclock" is supposed to
match the last pathname component in hwclock_path="/sbin/hwclock".
There are multiple ways to compute "arg" like that, of course. But I
think they all look uglier than the above. So I'm fine if we just keep this.
If you purposely dropped the "static", then:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
If you just missed the "static" and now intend to add it back, then for v3:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> NULL, environ);
> _exit(EXIT_FAILURE);
> } else if (pid < 0) {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] qga: fence guest-set-time if hwclock not available
2019-11-28 18:38 ` Laszlo Ersek
@ 2019-11-29 7:56 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-11-29 7:56 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Daniel P . Berrangé, Michael Roth, qemu-devel
On Thu, 28 Nov 2019 19:38:00 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Cornelia,
>
> On 11/28/19 19:11, 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>
> > ---
> >
> > v1 (RFC) -> v2:
> > - use hwclock_path[]
> > - use access() instead of stat()
> >
> > ---
> > qga/commands-posix.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 1c1a165daed8..ffb6420fa9cd 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -156,6 +156,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";
>
> Did you drop the "static" storage-class specifier on purpose?
No, I just need to do patches when I'm less tired :/
>
> > + static int hwclock_available = -1;
> > +
> > + if (hwclock_available < 0) {
> > + hwclock_available = (access(hwclock_path, X_OK) == 0);
> > + }
> > +
> > + if (!hwclock_available) {
> > + error_setg(errp, QERR_UNSUPPORTED);
> > + return;
> > + }
> >
> > /* If user has passed a time, validate and set it. */
> > if (has_time) {
> > @@ -195,7 +206,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",
>
> I think it's somewhat obscure now that arg="hwclock" is supposed to
> match the last pathname component in hwclock_path="/sbin/hwclock".
>
> There are multiple ways to compute "arg" like that, of course. But I
> think they all look uglier than the above. So I'm fine if we just keep this.
Yes, I was not able to come up with something elegant, either.
[Side note: does really everyone put hwclock under /sbin (i.e., nobody
doing something creative?)]
>
>
> If you purposely dropped the "static", then:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> If you just missed the "static" and now intend to add it back, then for v3:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
Thanks for looking! There'll be a v3.
>
>
>
> > NULL, environ);
> > _exit(EXIT_FAILURE);
> > } else if (pid < 0) {
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] qga: fence guest-set-time if hwclock not available
2019-11-28 18:11 [PATCH v2] qga: fence guest-set-time if hwclock not available Cornelia Huck
2019-11-28 18:38 ` Laszlo Ersek
@ 2019-11-29 9:35 ` Daniel P. Berrangé
2019-12-04 14:12 ` Cornelia Huck
2019-12-04 15:05 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2019-11-29 9:35 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Laszlo Ersek, Michael Roth, qemu-devel
On Thu, Nov 28, 2019 at 07:11:00PM +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>
> ---
>
> v1 (RFC) -> v2:
> - use hwclock_path[]
> - use access() instead of stat()
>
> ---
> qga/commands-posix.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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] 6+ messages in thread
* Re: [PATCH v2] qga: fence guest-set-time if hwclock not available
2019-11-28 18:11 [PATCH v2] qga: fence guest-set-time if hwclock not available Cornelia Huck
2019-11-28 18:38 ` Laszlo Ersek
2019-11-29 9:35 ` Daniel P. Berrangé
@ 2019-12-04 14:12 ` Cornelia Huck
2019-12-04 15:05 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-12-04 14:12 UTC (permalink / raw)
To: Michael Roth; +Cc: Laszlo Ersek, Daniel P . Berrangé, qemu-devel
On Thu, 28 Nov 2019 19:11:00 +0100
Cornelia Huck <cohuck@redhat.com> 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>
Michael, any comments before I send a v3?
> ---
>
> v1 (RFC) -> v2:
> - use hwclock_path[]
> - use access() instead of stat()
>
> ---
> qga/commands-posix.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1c1a165daed8..ffb6420fa9cd 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -156,6 +156,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 = (access(hwclock_path, X_OK) == 0);
> + }
> +
> + if (!hwclock_available) {
> + error_setg(errp, QERR_UNSUPPORTED);
> + return;
> + }
>
> /* If user has passed a time, validate and set it. */
> if (has_time) {
> @@ -195,7 +206,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) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] qga: fence guest-set-time if hwclock not available
2019-11-28 18:11 [PATCH v2] qga: fence guest-set-time if hwclock not available Cornelia Huck
` (2 preceding siblings ...)
2019-12-04 14:12 ` Cornelia Huck
@ 2019-12-04 15:05 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Michael Roth @ 2019-12-04 15:05 UTC (permalink / raw)
To: Cornelia Huck
Cc: Laszlo Ersek, Cornelia Huck, Daniel P . Berrangé, qemu-devel
Quoting Cornelia Huck (2019-11-28 12:11:00)
> 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>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>
> v1 (RFC) -> v2:
> - use hwclock_path[]
> - use access() instead of stat()
>
> ---
> qga/commands-posix.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1c1a165daed8..ffb6420fa9cd 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -156,6 +156,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 = (access(hwclock_path, X_OK) == 0);
> + }
> +
> + if (!hwclock_available) {
> + error_setg(errp, QERR_UNSUPPORTED);
> + return;
> + }
>
> /* If user has passed a time, validate and set it. */
> if (has_time) {
> @@ -195,7 +206,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 [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-04 15:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 18:11 [PATCH v2] qga: fence guest-set-time if hwclock not available Cornelia Huck
2019-11-28 18:38 ` Laszlo Ersek
2019-11-29 7:56 ` Cornelia Huck
2019-11-29 9:35 ` Daniel P. Berrangé
2019-12-04 14:12 ` Cornelia Huck
2019-12-04 15:05 ` Michael Roth
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).