On 05/04/2011 08:45 AM, Jiri Denemark wrote:
> ---
> tools/virsh.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 162 insertions(+), 33 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 5d8b025..6dae9fa 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -223,6 +223,8 @@ typedef struct __vshControl {
> int log_fd; /* log file descriptor */
> char *historydir; /* readline history directory name */
> char *historyfile; /* readline history file name */
> + bool noGetState; /* virDomainGetState is not supported in
> + * current connection */
Nice - remembering the failure once, so you don't have to repeat it.
However, I don't like double negatives; could we instead have:
bool useGetInfo /* must use virDomainGetInfo, since virDomainGetState
failed */
> @@ -571,6 +575,7 @@ vshReconnect(vshControl *ctl) {
> else
> vshError(ctl, "%s", _("Reconnected to the hypervisor"));
> disconnected = 0;
> + ctl->noGetState = false;
then a default state of ctl->useGetInfo = false still makes sense,
> @@ -3063,12 +3068,6 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
> return false;
> }
>
> - if (virDomainGetInfo(dom, &info) != 0) {
> - virDomainFree(dom);
> - vshError(ctl, "%s", _("Unable to verify current MemorySize"));
> - return false;
> - }
> -
This seems like one case where we still want to keep the
virDomainGetInfo and actually query the guest.
> +static int
> +vshDomainState(vshControl *ctl, virDomainPtr dom, int *reason)
> +{
> + virDomainInfo info;
> +
> + if (reason)
> + *reason = -1;
> +
> + if (!ctl->noGetState) {
Here, if you follow my rename, you avoid the double negative ("!" and
"no"), and instead have:
if (!ctl->useGetInfo)
But overall, the patch looks nice.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
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