[v1] libxl: implement virDomainObjCheckIsActive

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

Posted by Sagar Ghuge 8 years, 11 months ago

Add function which raises error if domain is
not active

Signed-off-by: Sagar Ghuge <ghugesss@gmail.com>
---
 src/conf/domain_conf.c | 15 +++++++++++++++
 src/conf/domain_conf.h |  1 +
 2 files changed, 16 insertions(+)

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);
 int virDomainObjWaitUntil(virDomainObjPtr vm,
                           unsigned long long whenms);
 
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Jim Fehlig 8 years, 11 months ago

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@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

>
> 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

>  int virDomainObjWaitUntil(virDomainObjPtr vm,
>                            unsigned long long whenms);
>
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Jim Fehlig 8 years, 11 months ago

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@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

>>
>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 8 years, 11 months ago


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@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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list