Message ID | 20191205115350.18713-1-cohuck@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] qga: fence guest-set-time if hwclock not available | expand |
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! */ 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) { >
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? > > > + 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) { > > >
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) { >>> >> >
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? 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
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 <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! > */ 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) { >> >
Laszlo Ersek <lersek@redhat.com> 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 <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! >> */ > > 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) { >>> >>
Cornelia Huck <cohuck@redhat.com> writes: > 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... I'm not faulting you for that. I think this new use is acceptable. For details, see my other reply in this thread. [...]
On 12/06/19 08:15, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> 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 <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! >>> */ >> >> 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) { >>>> >>>
On Fri, 06 Dec 2019 08:17:27 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Cornelia Huck <cohuck@redhat.com> writes: > > > 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... > > 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?
On 12/9/19 7:33 PM, Cornelia Huck wrote: > On Fri, 06 Dec 2019 08:17:27 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Cornelia Huck <cohuck@redhat.com> writes: >> >>> 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... >> >> 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é <philmd@redhat.com>
On Thu, 5 Dec 2019 12:53:50 +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. > > 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. > > --- > qga/commands-posix.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) Queued to s390-next.
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) {