2:49 p.m.
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(a)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
2:31 a.m.
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(a)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...
---
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
2:45 a.m.
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(a)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...
> ---
> 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/
3:25 a.m.
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(a)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...
>
>
>> ---
>> 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
7:49 a.m.
2018-04-13 8:25 GMT+00:00 Erik Skultety <eskultet(a)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(a)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...
> >
> >
> >> ---
> >> 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_err...
5:32 a.m.
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(a)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
2415
days inactive
2416
days old
6 comments
6 participants
participants (6)
-
Clementine Hayat -
Clementine Hayat -
Erik Skultety -
Jiri Denemark -
Ján Tomko -
Michal Privoznik