[libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive
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
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) {
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
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_i...
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) {
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_i...
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
participants (3)
-
Cole Robinson -
Jim Fehlig -
Sagar Ghuge