On 01/23/2012 07:48 AM, Michal Privoznik wrote:
[for some reason, this one didn't get threaded properly - bug in git
send-email?]
> This makes use of the QEMU guest agent to implement the
> virDomainShutdownFlags and virDomainReboot APIs. With
> no flags specified, it will prefer to use the agent, but
> fallback to ACPI. Explicit choice can be made by using
> a suitable flag
>
> * src/qemu/qemu_driver.c: Wire up use of agent
> ---
> src/qemu/qemu_driver.c | 131 ++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 106 insertions(+), 25 deletions(-)
>
> -static int qemuDomainShutdown(virDomainPtr dom) {
> +static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) {
> struct qemud_driver *driver = dom->conn->privateData;
> virDomainObjPtr vm;
> int ret = -1;
> qemuDomainObjPrivatePtr priv;
> + bool useAgent = false;
> +
> + virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
> + VIR_DOMAIN_SHUTDOWN_GUEST_AGENT, -1);
> +
> + if (flags &
> + (VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN &
> + VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) {
Won't work. That statement is always false.
> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Only one flag supported"));
> + return -1;
> + }
Back to my comment in 2/4, I'd prefer that you move this into libvirt.c
(so drivers don't have to duplicate it), where it should look like:
/* At most one of these two flags should be set. */
if ((flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) &&
(flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
> +
> + if (flags &
> + (VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN &
> + VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) {
> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Only one flag supported"));
> + return -1;
> + }
Another instance of broken logic that will always be false, and which I
think belongs better in libvirt.c.
I didn't see anything else wrong in the patch, so if you fix patch 2,
then delete the two no-op hunks from patch 3, you have my ACK.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list