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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 12/05/19 14:05, Philippe Mathieu-Daudé 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!
>>  */
>
> Obviously, the last word on this belongs to Markus (CC'd) -- he added
> that comment. I'd just like to point out *when* that comment was added:
> approx. four and half years ago. (See commit 4629ed1e9896.)

Yes, with a big push to finally kill qerror_report().  I was too
exhausted to finish the job and kill all the remaining QERR_, too.

I'm dead serious on the "do not add new ones" part.

On the "don't use in new code" part, I'm quite willing to tolerate
reasonable exceptions, e.g. to maintain consistency with old code.

This one looks like a reasonable exception to me.

> I've always associated QERR_UNSUPPORTED with QMP interfaces rejecting
> invocation due to lack of support. I don't think one more instance of
> QERR_UNSUPPORTED will matter much, when we'll "finally" :) convert or
> eliminate the other 35! (Yes, I've counted.)
>
> In case it's unacceptable to add one more QERR_UNSUPPORTED: what is the
> official solution that replaces it?
>
> I assume it was explained in the series that included commit
> 4629ed1e9896, but I can't easily tell. (And, there is no "QERR_" match
> in docs/.)

See "exhausted" above.

> Hmmm, more history digging... In the 4629ed1e9896..v4.2.0-rc4 set of
> commits, the following commits introduced new instances of
> QERR_UNSUPPORTED:
>
> - e09484efbc9d ("qmp: add QMP interface "query-cpu-model-expansion"", 
> 2016-09-06)
> - 0031e0d68339 ("qmp: add QMP interface "query-cpu-model-comparison"", 
> 2016-09-06)
> - b18b6043341d ("qmp: add QMP interface "query-cpu-model-baseline"", 
> 2016-09-06)
> - 1007a37e2082 ("smbios: filter based on CONFIG_SMBIOS rather than TARGET", 
> 2017-01-16)
> - 9f57061c3555 ("acpi: filter based on CONFIG_ACPI_X86 rather than TARGET", 
> 2017-01-16)
> - 39164c136cba ("qmp/hmp: add query-vm-generation-id and 'info 
> vm-generation-id' commands", 2017-03-02)
> - 161a56a9065f ("qga: Add 'guest-get-users' command", 2017-04-26)
> - 53c58e64d0a2 ("qga: Add `guest-get-timezone` command", 2017-04-27)
> - e674605f9821 ("qemu-ga: check if utmpx.h is available on the system", 
> 2017-07-17)
>
> I don't claim that all of those additions have stuck with us, to
> v4.2.0-rc4. Yet, in general, practice doesn't seem to have followed the
> intended deprecation.
>
>>
>> Maybe we can replace it by "this feature is not supported on this
>> architecture"? (or without 'on this architecture').
>
> I think if we replace QERR_UNSUPPORTED with anything, it should be
> "similarly standardized". (Lack of support for a given QMP interface is
> pretty common, I think.)

Here's my the general idea on getting rid of qerror.h.  The QERR_ macros
effectively factor out common error message format strings.  DRY is a
legitimate concern.  However, I dislike (1) passing anything but string
literals as format to printf()-style function, and (2) tempting people
to reuse existing error messages where a new error message would be more
helpful.

Note that the error_setg() is *also* common.  So take DRY to the next
level: factor out the common error_setg(errp, "literal error format
string", ...) along with whatever error handling is also common in a
helper function, and call that.

However, do that only where the errors are truly common.  Where they're
just similar, duplicate the error message, and maybe specialize it for
specific error situations.

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