[libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive

Sagar Ghuge posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170211083103.24283-1-ghugesss@gmail.com
src/conf/domain_conf.c   | 13 +++++++++++++
src/conf/domain_conf.h   |  1 +
src/libxl/libxl_driver.c |  4 +---
3 files changed, 15 insertions(+), 3 deletions(-)
[libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive
Posted by Sagar Ghuge 7 years, 1 month ago
Add function which raises error if domain is
not active

Signed-off-by: Sagar Ghuge <ghugesss@gmail.com>
---
 src/conf/domain_conf.c   | 13 +++++++++++++
 src/conf/domain_conf.h   |  1 +
 src/libxl/libxl_driver.c |  4 +---
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1bc72a4..10a69af 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2995,6 +2995,19 @@ 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 dd79206..b6c7826 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);
 
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 74cb05a..3a487ac 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1183,10 +1183,8 @@ libxlDomainSuspend(virDomainPtr dom)
     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
         goto cleanup;
 
-    if (!virDomainObjIsActive(vm)) {
-        virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running"));
+    if (virDomainObjCheckIsActive(vm) < 0)
         goto endjob;
-    }
 
     if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
         if (libxl_domain_pause(cfg->ctx, vm->def->id) != 0) {
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive
Posted by Jim Fehlig 7 years, 1 month ago
On 02/11/2017 01:31 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   | 13 +++++++++++++
>  src/conf/domain_conf.h   |  1 +
>  src/libxl/libxl_driver.c |  4 +---
>  3 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1bc72a4..10a69af 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2995,6 +2995,19 @@ 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 dd79206..b6c7826 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);
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 74cb05a..3a487ac 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1183,10 +1183,8 @@ libxlDomainSuspend(virDomainPtr dom)
>      if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>          goto cleanup;
>
> -    if (!virDomainObjIsActive(vm)) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running"));

This is the standard pattern used throughout the libxl driver and many other 
drivers as well. Why do you only want to change this one instance? IMO if such a 
change is desired it should be done consistently across the code base.

Regards,
Jim

> +    if (virDomainObjCheckIsActive(vm) < 0)
>          goto endjob;
> -    }
>
>      if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
>          if (libxl_domain_pause(cfg->ctx, vm->def->id) != 0) {
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive
Posted by Sagar Ghuge 7 years, 1 month ago
Hi Jim,

Thank you for replying. I am planning to participate in GSoC'17 and
this is my first patch towards it. This is from Byte sized task which
I picked up and trying to solve it. Yes you are right, pattern is used
throughout the libxl driver and many others but this is just a
rudimentary patch which will help me to get suggestion that am I on
right path. if yes then I can I can go ahead and make changes
accordingly.

On Sun, Feb 12, 2017 at 7:23 PM, Jim Fehlig <jfehlig@suse.com> wrote:
> On 02/11/2017 01:31 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   | 13 +++++++++++++
>>  src/conf/domain_conf.h   |  1 +
>>  src/libxl/libxl_driver.c |  4 +---
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1bc72a4..10a69af 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2995,6 +2995,19 @@ 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 dd79206..b6c7826 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);
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 74cb05a..3a487ac 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -1183,10 +1183,8 @@ libxlDomainSuspend(virDomainPtr dom)
>>      if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>>          goto cleanup;
>>
>> -    if (!virDomainObjIsActive(vm)) {
>> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
>> running"));
>
>
> This is the standard pattern used throughout the libxl driver and many other
> drivers as well. Why do you only want to change this one instance? IMO if
> such a change is desired it should be done consistently across the code
> base.
>
> Regards,
> Jim
>
>
>> +    if (virDomainObjCheckIsActive(vm) < 0)
>>          goto endjob;
>> -    }
>>
>>      if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
>>          if (libxl_domain_pause(cfg->ctx, vm->def->id) != 0) {
>>
>



-- 
Regards,
Sagar

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive
Posted by Jim Fehlig 7 years, 1 month ago
Sagar Ghuge wrote:
> Hi Jim,
> 
> Thank you for replying. I am planning to participate in GSoC'17 and
> this is my first patch towards it. This is from Byte sized task which
> I picked up and trying to solve it.

Ah, yes, this one

http://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_error_if_domain_is_not_active

> Yes you are right, pattern is used
> throughout the libxl driver and many others but this is just a
> rudimentary patch which will help me to get suggestion that am I on
> right path. if yes then I can I can go ahead and make changes
> accordingly.

It looks good to me, although I'm not sure how to best split up the work. Maybe
a patch that adds virDomainObjCheckIsActive, and then a patch for each driver
that replaces virDomainObjIsActive with virDomainObjCheckIsActive?

AFAICT, Cole added that task to the wiki. Perhaps he has a suggestion on the
best way to organize the work.

Regards,
Jim

> 
> On Sun, Feb 12, 2017 at 7:23 PM, Jim Fehlig <jfehlig@suse.com> wrote:
>> On 02/11/2017 01:31 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   | 13 +++++++++++++
>>>  src/conf/domain_conf.h   |  1 +
>>>  src/libxl/libxl_driver.c |  4 +---
>>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 1bc72a4..10a69af 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2995,6 +2995,19 @@ 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 dd79206..b6c7826 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);
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 74cb05a..3a487ac 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -1183,10 +1183,8 @@ libxlDomainSuspend(virDomainPtr dom)
>>>      if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>>>          goto cleanup;
>>>
>>> -    if (!virDomainObjIsActive(vm)) {
>>> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
>>> running"));
>>
>> This is the standard pattern used throughout the libxl driver and many other
>> drivers as well. Why do you only want to change this one instance? IMO if
>> such a change is desired it should be done consistently across the code
>> base.
>>
>> Regards,
>> Jim
>>
>>
>>> +    if (virDomainObjCheckIsActive(vm) < 0)
>>>          goto endjob;
>>> -    }
>>>
>>>      if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
>>>          if (libxl_domain_pause(cfg->ctx, vm->def->id) != 0) {
>>>
> 
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive
Posted by Cole Robinson 7 years, 1 month ago
On 02/13/2017 01:24 PM, Jim Fehlig wrote:
> Sagar Ghuge wrote:
>> Hi Jim,
>>
>> Thank you for replying. I am planning to participate in GSoC'17 and
>> this is my first patch towards it. This is from Byte sized task which
>> I picked up and trying to solve it.
> 
> Ah, yes, this one
> 
> http://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_error_if_domain_is_not_active
> 
>> Yes you are right, pattern is used
>> throughout the libxl driver and many others but this is just a
>> rudimentary patch which will help me to get suggestion that am I on
>> right path. if yes then I can I can go ahead and make changes
>> accordingly.
> 
> It looks good to me, although I'm not sure how to best split up the work. Maybe
> a patch that adds virDomainObjCheckIsActive, and then a patch for each driver
> that replaces virDomainObjIsActive with virDomainObjCheckIsActive?
> 
> AFAICT, Cole added that task to the wiki. Perhaps he has a suggestion on the
> best way to organize the work.
> 

Sorry for the late response.

For these types of patches, please link to the BiteSizedTask in the mail after
the -- break generated by git format-patch: this keeps the link out of the
commit message since it's not really relevant there, but gives more info to
potential patch reviewers.

For this item I'd say do a patch series containing more instances of
conversions, to show that it's actually simplifying a common code pattern. So
maybe a patch series like:

1) Add the function
2) Convert src/test/*
3) Convert src/libxl/*
4) Convert src/xen/*

That shouldn't be too much work and will give a bit more to comment on, and if
there's no objections, send the remaining conversions as an additional patch
series.

- Cole

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