qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] qga: fence guest-set-time if hwclock not available


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v3] qga: fence guest-set-time if hwclock not available
Date: Tue, 10 Dec 2019 17:38:06 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 12/9/19 7:33 PM, Cornelia Huck wrote:
On Fri, 06 Dec 2019 08:17:27 +0100
Markus Armbruster <address@hidden> wrote:

Cornelia Huck <address@hidden> writes:

On Thu, 5 Dec 2019 14:05:19 +0100
Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
Reviewed-by: Daniel P. Berrangé <address@hidden>
Reviewed-by: Michael Roth <address@hidden>
Signed-off-by: Cornelia Huck <address@hidden>
---

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

I'm not faulting you for that.

I think this new use is acceptable.  For details, see my other reply in
this thread.

Ok, thanks for your explanation there.

I guess I'll queue this on s390-next... Philippe, any objections to
adding your R-b to the unmodified patch?

Certainly, sorry for the delay/noise on this trivial patch, I learned the subtle differences between comments in code and reality :)

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]