src/conf/domain_conf.c | 11 +++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 96 +++++++++------------------------------- 4 files changed, 34 insertions(+), 76 deletions(-)
Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c.
It calls virDomainObjIsActive, raises error and returns.
There is a lot of occurence of this pattern and it will save 3 lines on
each call. Knowing that there is over 100 occurences, it will remove 300
lines from the code base.
Signed-off-by: Clementine Hayat <clem@lse.epita.fr>
---
Patch proposed for gsoc2018.
src/conf/domain_conf.c | 11 +++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 96 +++++++++-------------------------------
4 files changed, 34 insertions(+), 76 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d23182f18..86d28c26a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6003,6 +6003,17 @@ virDomainDefValidate(virDomainDefPtr def,
return 0;
}
+int
+virDomainObjCheckIsActive(virDomainObjPtr dom)
+{
+ if (!virDomainObjIsActive(dom)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ return -1;
+ }
+ return 0;
+}
+
/**
* virDomainDeviceLoadparmIsValid
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bbaa24137..8de4c4145 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2722,6 +2722,8 @@ virDomainObjIsActive(virDomainObjPtr dom)
return dom->def->id != -1;
}
+int virDomainObjCheckIsActive(virDomainObjPtr dom);
+
int virDomainDefSetVcpusMax(virDomainDefPtr def,
unsigned int vcpus,
virDomainXMLOptionPtr xmlopt);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cab324c4d..d90df3583 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -474,6 +474,7 @@ virDomainNostateReasonTypeFromString;
virDomainNostateReasonTypeToString;
virDomainObjAssignDef;
virDomainObjBroadcast;
+virDomainObjCheckIsActive;
virDomainObjCopyPersistentDef;
virDomainObjEndAPI;
virDomainObjFormat;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fcd79bd71..22cc9bddb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3537,11 +3537,8 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
if (virDomainSaveFlagsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto cleanup;
- }
ret = qemuDomainSaveInternal(driver, vm, path, compressed,
compressedpath, dxml, flags);
@@ -3595,11 +3592,9 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
if (virDomainManagedSaveEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto cleanup;
- }
+
if (!vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot do managed save for transient domain"));
@@ -3939,11 +3934,8 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
VIR_DOMAIN_JOB_OPERATION_DUMP) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
priv->job.current->statsType = QEMU_DOMAIN_JOB_STATS_TYPE_SAVEDUMP;
@@ -4054,11 +4046,8 @@ qemuDomainScreenshot(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
/* Well, even if qemu allows multiple graphic cards, heads, whatever,
* screenshot command does not */
@@ -4165,11 +4154,8 @@ processWatchdogEvent(virQEMUDriverPtr driver,
goto cleanup;
}
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
flags |= cfg->autoDumpBypassCache ? VIR_DUMP_BYPASS_CACHE: 0;
if ((ret = doCoreDump(driver, vm, dumpfile, flags,
@@ -10841,11 +10827,8 @@ qemuDomainBlockResize(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_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 (!(disk = virDomainDiskByName(vm->def, path, false))) {
virReportError(VIR_ERR_INVALID_ARG,
@@ -11001,11 +10984,8 @@ qemuDomainBlockStats(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
if (qemuDomainBlocksStatsGather(driver, vm, path, &blockstats) < 0)
goto endjob;
@@ -11058,11 +11038,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
if ((nstats = qemuDomainBlocksStatsGather(driver, vm, path,
&blockstats)) < 0)
@@ -11128,11 +11105,8 @@ qemuDomainInterfaceStats(virDomainPtr dom,
if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto cleanup;
- }
if (!(net = virDomainNetFind(vm->def, device)))
goto cleanup;
@@ -11484,11 +11458,8 @@ qemuDomainMemoryStatsInternal(virQEMUDriverPtr driver,
int ret = -1;
long rss;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
return -1;
- }
if (vm->def->memballoon &&
vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
@@ -11638,11 +11609,8 @@ qemuDomainMemoryPeek(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
if (virAsprintf(&tmp, "%s/qemu.mem.XXXXXX", cfg->cacheDir) < 0)
goto endjob;
@@ -13294,11 +13262,8 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
return -1;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto cleanup;
- }
if (!priv->job.current) {
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
@@ -13426,11 +13391,8 @@ static int qemuDomainAbortJob(virDomainPtr dom)
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_ABORT) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
@@ -13493,11 +13455,8 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MIGRATION_OP) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
@@ -13538,11 +13497,8 @@ qemuDomainMigrateGetMaxDowntime(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
@@ -13591,11 +13547,8 @@ qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
@@ -13642,11 +13595,8 @@ qemuDomainMigrateSetCompressionCache(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MIGRATION_OP) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
@@ -13704,11 +13654,8 @@ qemuDomainMigrateSetMaxSpeed(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MIGRATION_OP) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
VIR_DEBUG("Setting migration bandwidth to %luMbs", bandwidth);
qemuDomainObjEnterMonitor(driver, vm);
@@ -13779,11 +13726,8 @@ qemuDomainMigrateStartPostCopy(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MIGRATION_OP) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain is not running"));
+ if (virDomainObjCheckIsActive(vm) < 0)
goto endjob;
- }
priv = vm->privateData;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 12, 2018 at 07:49:15PM +0000, Clementine Hayat wrote: >Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c. >It calls virDomainObjIsActive, raises error and returns. *raises error if necessary > >There is a lot of occurence of this pattern and it will save 3 lines on >each call. Knowing that there is over 100 occurences, it will remove 300 >lines from the code base. False advertising. If you don't want to send them all in one patch, I suggest spliting them per-subdirectory: conf, qemu, libxl, vz, ... (and use that prefix in the commit summary) > >Signed-off-by: Clementine Hayat <clem@lse.epita.fr> If you have any accents in your name, feel free to use them. Even danpb recently decided the world is ready for UTF-8: https://libvirt.org/git/?p=libvirt.git;a=search;s=Daniel+P.+Berrang%C3%A9;st=author >--- >Patch proposed for gsoc2018. > > src/conf/domain_conf.c | 11 +++++ > src/conf/domain_conf.h | 2 + > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 96 +++++++++------------------------------- > 4 files changed, 34 insertions(+), 76 deletions(-) > The changes look good but the patch feels incomplete. Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/13/2018 09:31 AM, Ján Tomko wrote: > On Thu, Apr 12, 2018 at 07:49:15PM +0000, Clementine Hayat wrote: >> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c. >> It calls virDomainObjIsActive, raises error and returns. > > *raises error if necessary > >> >> There is a lot of occurence of this pattern and it will save 3 lines on >> each call. Knowing that there is over 100 occurences, it will remove 300 >> lines from the code base. > > False advertising. > > If you don't want to send them all in one patch, I suggest spliting them > per-subdirectory: conf, qemu, libxl, vz, ... (and use that prefix in the > commit summary) > >> >> Signed-off-by: Clementine Hayat <clem@lse.epita.fr> > > If you have any accents in your name, feel free to use them. Even danpb > recently decided the world is ready for UTF-8: > https://libvirt.org/git/?p=libvirt.git;a=search;s=Daniel+P.+Berrang%C3%A9;st=author > > >> --- >> Patch proposed for gsoc2018. >> >> src/conf/domain_conf.c | 11 +++++ >> src/conf/domain_conf.h | 2 + >> src/libvirt_private.syms | 1 + >> src/qemu/qemu_driver.c | 96 +++++++++------------------------------- >> 4 files changed, 34 insertions(+), 76 deletions(-) >> > > The changes look good but the patch feels incomplete. I was just looking at this patch. Yes it is incomplete but I think that was the point. Give upstream taste what the patch looks like before diving in and changing all those 108 occurrences (I did change them locally). The patch looks good to me, but as Jan suggests, you can break this big change into smaller (=easier to review) patches. In the first you can just introduce the function. And then have patch per each folder. Alternatively, you can write a small semantic patch [1] and use that to generate the diff. But this is rather advanced stuff. Michal 1: http://coccinelle.lip6.fr/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Apr 13, 2018 at 09:45:48AM +0200, Michal Privoznik wrote: > On 04/13/2018 09:31 AM, Ján Tomko wrote: > > On Thu, Apr 12, 2018 at 07:49:15PM +0000, Clementine Hayat wrote: > >> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c. > >> It calls virDomainObjIsActive, raises error and returns. > > > > *raises error if necessary > > > >> > >> There is a lot of occurence of this pattern and it will save 3 lines on > >> each call. Knowing that there is over 100 occurences, it will remove 300 > >> lines from the code base. > > > > False advertising. > > > > If you don't want to send them all in one patch, I suggest spliting them > > per-subdirectory: conf, qemu, libxl, vz, ... (and use that prefix in the > > commit summary) > > > >> > >> Signed-off-by: Clementine Hayat <clem@lse.epita.fr> > > > > If you have any accents in your name, feel free to use them. Even danpb > > recently decided the world is ready for UTF-8: > > https://libvirt.org/git/?p=libvirt.git;a=search;s=Daniel+P.+Berrang%C3%A9;st=author > > > > > >> --- > >> Patch proposed for gsoc2018. > >> > >> src/conf/domain_conf.c | 11 +++++ > >> src/conf/domain_conf.h | 2 + > >> src/libvirt_private.syms | 1 + > >> src/qemu/qemu_driver.c | 96 +++++++++------------------------------- > >> 4 files changed, 34 insertions(+), 76 deletions(-) > >> > > > > The changes look good but the patch feels incomplete. > > I was just looking at this patch. Yes it is incomplete but I think that > was the point. Give upstream taste what the patch looks like before > diving in and changing all those 108 occurrences (I did change them > locally). > > The patch looks good to me, but as Jan suggests, you can break this big > change into smaller (=easier to review) patches. In the first you can > just introduce the function. And then have patch per each folder. I agree with the intention of the patch and the comments, I'd maybe change the name slightly --> virDomainObjCheckActive (instead of *IsActive) or even something that emphasizes a bit more that it will report error on inactive, so that the caller doesn't have to care about that anymore - from the top of my head "virDomainObjReportInactive"... Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
2018-04-13 8:25 GMT+00:00 Erik Skultety <eskultet@redhat.com>: > On Fri, Apr 13, 2018 at 09:45:48AM +0200, Michal Privoznik wrote: >> On 04/13/2018 09:31 AM, Ján Tomko wrote: >> > On Thu, Apr 12, 2018 at 07:49:15PM +0000, Clementine Hayat wrote: >> >> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c. >> >> It calls virDomainObjIsActive, raises error and returns. >> > >> > *raises error if necessary Yes, thank you. >> >> There is a lot of occurence of this pattern and it will save 3 lines on >> >> each call. Knowing that there is over 100 occurences, it will remove 300 >> >> lines from the code base. >> > >> > False advertising. >> > >> > If you don't want to send them all in one patch, I suggest spliting them >> > per-subdirectory: conf, qemu, libxl, vz, ... (and use that prefix in the >> > commit summary) I'll do that thank you. >> >> Signed-off-by: Clementine Hayat <clem@lse.epita.fr> >> > >> > If you have any accents in your name, feel free to use them. Even danpb >> > recently decided the world is ready for UTF-8: >> > https://libvirt.org/git/?p=libvirt.git;a=search;s=Daniel+P.+Berrang%C3%A9;st=author >> > >> > >> >> --- >> >> Patch proposed for gsoc2018. >> >> >> >> src/conf/domain_conf.c | 11 +++++ >> >> src/conf/domain_conf.h | 2 + >> >> src/libvirt_private.syms | 1 + >> >> src/qemu/qemu_driver.c | 96 +++++++++------------------------------- >> >> 4 files changed, 34 insertions(+), 76 deletions(-) >> >> >> > >> > The changes look good but the patch feels incomplete. >> >> I was just looking at this patch. Yes it is incomplete but I think that >> was the point. Give upstream taste what the patch looks like before >> diving in and changing all those 108 occurrences (I did change them >> locally). It's right, it's one of the BiteSizedTasks proposed to begin with [1] and it's asked to do it in two times. First do a small patch and have it review and then change all the occurences. I'm sorry I should have mentioned it. >> The patch looks good to me, but as Jan suggests, you can break this big >> change into smaller (=easier to review) patches. In the first you can >> just introduce the function. And then have patch per each folder. > I agree with the intention of the patch and the comments, I'd maybe change the > name slightly --> virDomainObjCheckActive (instead of *IsActive) or even > something that emphasizes a bit more that it will report error on inactive, so > that the caller doesn't have to care about that anymore - from the top of my > head "virDomainObjReportInactive"... I'm going to do that. I think virDomainObjCheckActive is a good name. For the virDomainObjReportInactive, I have the feeling that, by reading the name, people may expect that the function will return 0 if there was an error. >> Alternatively, you can write a small semantic patch [1] and use that to >> generate the diff. But this is rather advanced stuff. I'll take a look into coccinelle. It may take a bit more time thought. -- Clementine Hayat [1] https://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_error_if_domain_is_not_active -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/13/2018 02:49 PM, Clementine Hayat wrote: > <snip/> > I'll take a look into coccinelle. It may take a bit more time thought. > Yeah, don't waste too much time on it. I merely just wanted to mention it. It not that trivial to learn. But once you do, it's awesome tool. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 12, 2018 at 19:49:15 +0000, Clementine Hayat wrote:
> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c.
> It calls virDomainObjIsActive, raises error and returns.
>
> There is a lot of occurence of this pattern and it will save 3 lines on
> each call. Knowing that there is over 100 occurences, it will remove 300
> lines from the code base.
>
> Signed-off-by: Clementine Hayat <clem@lse.epita.fr>
> ---
> Patch proposed for gsoc2018.
>
> src/conf/domain_conf.c | 11 +++++
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 96 +++++++++-------------------------------
> 4 files changed, 34 insertions(+), 76 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d23182f18..86d28c26a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6003,6 +6003,17 @@ virDomainDefValidate(virDomainDefPtr def,
> return 0;
> }
>
> +int
> +virDomainObjCheckIsActive(virDomainObjPtr dom)
> +{
> + if (!virDomainObjIsActive(dom)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + return -1;
You need to add one more space of indentation in the three lines above.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.