[libvirt] [PATCH 1/3] libxl: Implement virDomainObjCheckIsActive

[John Ferlan]

On 02/28/2017 05:17 PM, Jim Fehlig wrote:
> On 02/28/2017 11:32 AM, Jim Fehlig wrote:
>> On 02/24/2017 01:03 AM, Sagar Ghuge wrote:
>>> Add function which raises error if domain is
>>> not active
>>>
>>> Signed-off-by: Sagar Ghuge <ghugesss at gmail.com>
>>> ---
>>>  src/conf/domain_conf.c | 15 +++++++++++++++
>>>  src/conf/domain_conf.h |  1 +
>>>  2 files changed, 16 insertions(+)
>>
>> This patch doesn't touch any libxl code, so the commit summary should be
>>
>> conf: Implement virDomainObjCheckIsActive
> 
> BTW, since this new function simply wraps the existing
> virDomainObjIsActive with error reporting, maybe a better name is
> virDomainObjIsActiveWithError. Just a suggestion. Perhaps Cole or others
> have an opinion about the name.
> 
> Regards,
> Jim
> 

When all else fails, go with longer names ;-)

I would think "eventually" there's two scenarios you're trying to cover

1. Common action/message if active is unexpected:

    if (virDomainObjIsActive(vm)) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       "%s", _("Domain is already running"));
        goto cleanup;
    }

2. Common action/message if inactive is unexpected:

    if (!virDomainObjIsActive(vm)) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       "%s", _("Domain is not running"));
        goto cleanup;
    }

Although changing virDomainObjIsActive would seem to be not feasible as
there are callers that don't care about messaging the answer; however,
what if we added usage of va_args which would then message based on
whether a 2nd/3rd boolean arg exists.

e.g.

    va_start(ap, vm);
    if ((wantError = va_arg(ap, int))) {
        if ((isRunning = va_arg(ap, int)))
            virReportError(... "Domain is already running");
        else
            virReportError(... "Domain is not running");
    }

For those callers that care, the arguments would be "true" if you want a
message and "true" if that message should indicate already running or
either "false" or not provided if not running.  Leaving out a few key
details I'm sure, but I've seen other patches that expand the APIs
created for the simple purpose of providing the error message not get
accepted, so this is just another "idea".

John

>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 97d42fe..a44454c 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2997,6 +2997,21 @@ virDomainObjWait(virDomainObjPtr vm)
>>>  }
>>>
>>>
>>> +int
>>> +virDomainObjCheckIsActive(virDomainObjPtr vm)
>>> +{
>>> +    if (!virDomainObjIsActive(vm)) {
>>> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>> +                       _("domain is not running"));
>>> +        return -1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +
>>> +
>>>  /**
>>>   * Waits for domain condition to be triggered for a specific period
>>> of time.
>>>   *
>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>> index 1e53cc3..cfeb1ba 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -2559,6 +2559,7 @@ bool virDomainObjTaint(virDomainObjPtr obj,
>>>
>>>  void virDomainObjBroadcast(virDomainObjPtr vm);
>>>  int virDomainObjWait(virDomainObjPtr vm);
>>> +int virDomainObjCheckIsActive(virDomainObjPtr vm);
>>
>> I realize functions aren't listed in alphabetical order, but placement
>> here
>> seems a bit odd. Maybe add it after the existing declaration of
>> virDomainObjIsActive(). Also, I think this function should return a
>> bool instead
>> of int.
>>
>> Regards,
>> Jim
> 
> -- 
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list