On 12/5/19 2:12 PM, Cornelia Huck wrote:

On Thu, 5 Dec 2019 14:05:19 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

...

Hi Cornelia,

On 12/5/19 12:53 PM, 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.

Reviewed-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> Signed-off-by: Cornelia Huck <cohuck@redhat.com> ---

v2->v3: - added 'static' keyword to hwclock_path

Not sure what tree this is going through; if there's no better place, I can also take this through the s390 tree.

s390 or trivial trees seems appropriate.

...

--- 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..0be301a4ea77 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; + static 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);

In include/qapi/qmp/qerror.h we have:

/* * These macros will go away, please don't use in new code, and do not * add new ones! */

Sigh, it is really hard to keep track here :( I just copied from other callers in this file...

...

Maybe we can replace it by "this feature is not supported on this architecture"? (or without 'on this architecture').

This is not really architecture specific, you'd get this on any setup that does not have /sbin/hwclock.

Q: Is libvirt doing anything with such an error message from QEMU? Do we have the freedom to say e.g "guest-set-time is not supported" or so? Or is it beneficial to print the same error message for any unsupported feature?

Cc'ing Markus who added the command and libvir-list. Using "guest-set-time is not supported" or "this command is not supported": Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

...

...

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