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

Commit Message

Cornelia Huck Dec. 5, 2019, 11:53 a.m. UTC

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 <[e-mail]>
Reviewed-by: Daniel P. Berrangé <[e-mail]>
Reviewed-by: Michael Roth <[e-mail]>
Signed-off-by: Cornelia Huck <[e-mail]>
---

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.

---
 qga/commands-posix.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Dec. 5, 2019, 1:05 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Daniel P. Berrangé <[e-mail]>
> Reviewed-by: Michael Roth <[e-mail]>
> Signed-off-by: Cornelia Huck <[e-mail]>
> ---
> 
> 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!
  */

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

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

Cornelia Huck Dec. 5, 2019, 1:12 p.m. UTC | #2

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

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

Philippe Mathieu-Daudé Dec. 5, 2019, 1:20 p.m. UTC | #3

On 12/5/19 2:12 PM, Cornelia Huck wrote:
> On Thu, 5 Dec 2019 14:05:19 +0100
> Philippe Mathieu-Daudé <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Daniel P. Berrangé <[e-mail]>
>>> Reviewed-by: Michael Roth <[e-mail]>
>>> Signed-off-by: Cornelia Huck <[e-mail]>
>>> ---
>>>
>>> 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é <[e-mail]>

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

Michal Prívozník Dec. 5, 2019, 2:21 p.m. UTC | #4

On 12/5/19 2:12 PM, Cornelia Huck wrote:
> On Thu, 5 Dec 2019 14:05:19 +0100
> Philippe Mathieu-Daudé <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Daniel P. Berrangé <[e-mail]>
>>> Reviewed-by: Michael Roth <[e-mail]>
>>> Signed-off-by: Cornelia Huck <[e-mail]>
>>> ---
>>>
>>> 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?

No. Libvirt threats error messages as an opaque data. In a few cases we 
check for the class of the error and for instance issue a different 
command if class was CommandNotFound. You are free to change error 
messages as you wish.

Note that this is not true for HMP - there libvirt does some parsing to 
figure out the source of error. For instance:

https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9054682d608b13347880f36cacd0e023151322e6;hb=HEAD#l36

Michal

Laszlo Ersek Dec. 5, 2019, 3:24 p.m. UTC | #5

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 <[e-mail]>
>> Reviewed-by: Daniel P. Berrangé <[e-mail]>
>> Reviewed-by: Michael Roth <[e-mail]>
>> Signed-off-by: Cornelia Huck <[e-mail]>
>> ---
>>
>> 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.)

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

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

Thanks,
Laszlo

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

Markus Armbruster Dec. 6, 2019, 7:15 a.m. UTC | #6

Laszlo Ersek <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Daniel P. Berrangé <[e-mail]>
>>> Reviewed-by: Michael Roth <[e-mail]>
>>> Signed-off-by: Cornelia Huck <[e-mail]>
>>> ---
>>>
>>> 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) {
>>>
>>

Markus Armbruster Dec. 6, 2019, 7:17 a.m. UTC | #7

Cornelia Huck <[e-mail]> writes:

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

[...]

Laszlo Ersek Dec. 6, 2019, 9:02 a.m. UTC | #8

On 12/06/19 08:15, Markus Armbruster wrote:
> Laszlo Ersek <[e-mail]> 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 <[e-mail]>
>>>> Reviewed-by: Daniel P. Berrangé <[e-mail]>
>>>> Reviewed-by: Michael Roth <[e-mail]>
>>>> Signed-off-by: Cornelia Huck <[e-mail]>
>>>> ---
>>>>
>>>> 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.

Good point, I didn't think of (e.g.) error_set_qmp_unsupported().

Thank you for the details!
Laszlo

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

Cornelia Huck Dec. 9, 2019, 6:33 p.m. UTC | #9

On Fri, 06 Dec 2019 08:17:27 +0100
Markus Armbruster <[e-mail]> wrote:

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

Philippe Mathieu-Daudé Dec. 10, 2019, 4:38 p.m. UTC | #10

On 12/9/19 7:33 PM, Cornelia Huck wrote:
> On Fri, 06 Dec 2019 08:17:27 +0100
> Markus Armbruster <[e-mail]> wrote:
> 
>> Cornelia Huck <[e-mail]> writes:
>>
>>> On Thu, 5 Dec 2019 14:05:19 +0100
>>> Philippe Mathieu-Daudé <[e-mail]> 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 <[e-mail]>
>>>>> Reviewed-by: Daniel P. Berrangé <[e-mail]>
>>>>> Reviewed-by: Michael Roth <[e-mail]>
>>>>> Signed-off-by: Cornelia Huck <[e-mail]>
>>>>> ---
>>>>>
>>>>> 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é <[e-mail]>

Cornelia Huck Dec. 10, 2019, 5:07 p.m. UTC | #11

On Thu,  5 Dec 2019 12:53:50 +0100
Cornelia Huck <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Daniel P. Berrangé <[e-mail]>
> Reviewed-by: Michael Roth <[e-mail]>
> Signed-off-by: Cornelia Huck <[e-mail]>
> ---
> 
> 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.
> 
> ---
>  qga/commands-posix.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Queued to s390-next.

Patch

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