Re: [libvirt] [PATCH] xen: ensure /usr/sbin/xend exists before checking status
Eric Blake wrote:
On 04/28/2014 12:42 PM, Jim Fehlig wrote:
> With xend on the way out, installations may not even have
> /usr/sbin/xend, which results in the following error when the
> drivers are probed
>
> 2014-04-28 18:21:19.271+0000: 22129: error : virCommandWait:2426 :
> internal error: Child process (/usr/sbin/xend status) unexpected exit
> status 127: libvirt: error : cannot execute binary /usr/sbin/xend:
> No such file or directory
>
> Check for existence of /usr/sbin/xend before trying to run it with
> the 'status' option.
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> src/libxl/libxl_driver.c | 19 ++++++++++++-------
> src/xen/xen_driver.c | 13 ++++++++-----
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
> @@ -256,14 +255,20 @@ libxlDriverShouldLoad(bool privileged)
> }
>
> /* Don't load if legacy xen toolstack (xend) is in use */
> - cmd = virCommandNewArgList("/usr/sbin/xend", "status",
NULL);
> - if (virCommandRun(cmd, &status) == 0 && status == 0) {
> - VIR_INFO("Legacy xen tool stack seems to be in use, disabling "
> - "libxenlight driver.");
> + if (virFileExists("/usr/sbin/xend")) {
> + virCommandPtr cmd;
> +
> + cmd = virCommandNewArgList("/usr/sbin/xend", "status",
NULL);
> + if (virCommandRun(cmd, &status) == 0 && status == 0) {
>
Pre-patch, passing &status to virCommandRun had the effect of avoiding a
log message if the command ran with non-zero status. But now that we
are requiring the file to exist, and in particular, since we are already
using NULL later on...
> +++ b/src/xen/xen_driver.c
> @@ -315,13 +315,16 @@ xenUnifiedProbe(void)
> static bool
> xenUnifiedXendProbe(void)
> {
> - virCommandPtr cmd;
> bool ret = false;
>
> - cmd = virCommandNewArgList("/usr/sbin/xend", "status",
NULL);
> - if (virCommandRun(cmd, NULL) == 0)
> - ret = true;
> - virCommandFree(cmd);
> + if (virFileExists("/usr/sbin/xend")) {
> + virCommandPtr cmd;
> +
> + cmd = virCommandNewArgList("/usr/sbin/xend", "status",
NULL);
> + if (virCommandRun(cmd, NULL) == 0)
>
...here, would it make sense to use virCommandRun(cmd, NULL) instead of
checking status ourselves?
Yes, I think so. No harm in a log message from virCommandRun on
non-zero exit.
If the intent is still to avoid a logged
message when the status is non-zero, a comment in the code might be
useful (for example, see how virStorageBackendQEMUImgBackingFormat in
storage_backend.c has a comment).
ACK with either the code simplification or the added comment.
Code simplified and pushed. Thanks for the review.
Regards,
Jim