10:09 p.m.
We had a lot of codes use the same style of cgroup controller check.
This series introduce macro virCheckControllerGoto
to simplify these kinds of check.
Chen Hanxiao (4):
internal: introduce macro virCheckControllerGoto
lxc_driver: use virCheckControllerGoto to simplify cgroup controller
check
qemu_driver: use virCheckControllerGoto to simplify cgroup controller
check
qemu_process: use virCheckControllerGoto to simplify cgroup controller
check
src/internal.h | 19 ++++++++
src/lxc/lxc_driver.c | 115 ++++++++++++++----------------------------------
src/qemu/qemu_driver.c | 70 +++++++++--------------------
src/qemu/qemu_process.c | 9 ++--
4 files changed, 77 insertions(+), 136 deletions(-)
--
1.8.3.1
10:09 p.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Introduce macro virCheckControllerGoto.
Jumps to a label if unsupported controller type were
passed to it.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/internal.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index d8cc5ad..a34f43e 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -362,6 +362,25 @@
} \
} while (0)
+/**
+ * virCheckControllerGoto:
+ * @Cgp: virCgroupPtr pointer
+ * @CgpCtr: cgroup controller type enum
+ * @label: label to jump to on error
+ *
+ * Returns nothing. Jumps to a label if unsupported controller type were
+ * passed to it.
+ */
+# define virCheckControllerGoto(Cgp, CgpCtr, label) \
+ do { \
+ if (!virCgroupHasController(Cgp, CgpCtr)) { \
+ virReportError(VIR_ERR_OPERATION_INVALID, \
+ _("cgroup %s controller is not mounted"), \
+ virCgroupControllerTypeToString(CgpCtr)); \
+ goto label; \
+ } \
+ } while (0)
+
/* Macros to help dealing with mutually exclusive flags. */
/**
--
1.8.3.1
9:49 a.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Introduce macro virCheckControllerGoto.
Jumps to a label if unsupported controller type were
passed to it.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/internal.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index d8cc5ad..a34f43e 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -362,6 +362,25 @@
} \
} while (0)
+/**
+ * virCheckControllerGoto:
+ * @Cgp: virCgroupPtr pointer
+ * @CgpCtr: cgroup controller type enum
+ * @label: label to jump to on error
+ *
+ * Returns nothing. Jumps to a label if unsupported controller type were
+ * passed to it.
+ */
+# define virCheckControllerGoto(Cgp, CgpCtr, label) \
+ do { \
+ if (!virCgroupHasController(Cgp, CgpCtr)) { \
+ virReportError(VIR_ERR_OPERATION_INVALID, \
+ _("cgroup %s controller is not mounted"), \
^
Extra space
s/%s/'%s'/
ACK (I'll adjust before pushing)
John
+ virCgroupControllerTypeToString(CgpCtr));
\
+ goto label; \
+ } \
+ } while (0)
+
/* Macros to help dealing with mutually exclusive flags. */
/**
9:55 a.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
On Wed, Nov 23, 2016 at 10:49:00AM -0500, John Ferlan wrote:
On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao(a)gmail.com>
>
> Introduce macro virCheckControllerGoto.
> Jumps to a label if unsupported controller type were
> passed to it.
>
> Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
> ---
> src/internal.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/src/internal.h b/src/internal.h
> index d8cc5ad..a34f43e 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -362,6 +362,25 @@
> } \
> } while (0)
>
> +/**
> + * virCheckControllerGoto:
> + * @Cgp: virCgroupPtr pointer
> + * @CgpCtr: cgroup controller type enum
> + * @label: label to jump to on error
> + *
> + * Returns nothing. Jumps to a label if unsupported controller type were
> + * passed to it.
> + */
> +# define virCheckControllerGoto(Cgp, CgpCtr, label) \
> + do { \
> + if (!virCgroupHasController(Cgp, CgpCtr)) { \
> + virReportError(VIR_ERR_OPERATION_INVALID, \
> + _("cgroup %s controller is not mounted"), \
^
Extra space
s/%s/'%s'/
ACK (I'll adjust before pushing)
I'm personally *not* in favour of doing this. I don't think that macros
should be used to hide control flow logic, especially when they're hiding
'goto'.
Now, we do have some of this style macros for checking flags. I think those
are more acceptable because flag checking is always done right at the
start of the function.
These cgroup macros by contrast will be placed at arbitrary locations
in the middle of functions, hiding the fact that we're jumping right
out.
So personally I'm NACK on this series, unless there contrasting opinions
people want to put forward.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
10:17 a.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
On 11/23/2016 10:55 AM, Daniel P. Berrange wrote:
On Wed, Nov 23, 2016 at 10:49:00AM -0500, John Ferlan wrote:
>
>
> On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
>> From: Chen Hanxiao <chenhanxiao(a)gmail.com>
>>
>> Introduce macro virCheckControllerGoto.
>> Jumps to a label if unsupported controller type were
>> passed to it.
>>
>> Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
>> ---
>> src/internal.h | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/src/internal.h b/src/internal.h
>> index d8cc5ad..a34f43e 100644
>> --- a/src/internal.h
>> +++ b/src/internal.h
>> @@ -362,6 +362,25 @@
>> } \
>> } while (0)
>>
>> +/**
>> + * virCheckControllerGoto:
>> + * @Cgp: virCgroupPtr pointer
>> + * @CgpCtr: cgroup controller type enum
>> + * @label: label to jump to on error
>> + *
>> + * Returns nothing. Jumps to a label if unsupported controller type were
>> + * passed to it.
>> + */
>> +# define virCheckControllerGoto(Cgp, CgpCtr, label) \
>> + do { \
>> + if (!virCgroupHasController(Cgp, CgpCtr)) { \
>> + virReportError(VIR_ERR_OPERATION_INVALID, \
>> + _("cgroup %s controller is not mounted"),
\
> ^
> Extra space
>
> s/%s/'%s'/
>
> ACK (I'll adjust before pushing)
I'm personally *not* in favour of doing this. I don't think that macros
should be used to hide control flow logic, especially when they're hiding
'goto'.
Now, we do have some of this style macros for checking flags. I think those
are more acceptable because flag checking is always done right at the
start of the function.
These cgroup macros by contrast will be placed at arbitrary locations
in the middle of functions, hiding the fact that we're jumping right
out.
So personally I'm NACK on this series, unless there contrasting opinions
people want to put forward.
I'm ambivalent - to a degree I saw this is as "similar to" places where
we create capitalized macro names and use the "goto" to jump control. In
most of those cases, the label *isn't* included in the macro as a
parameter (use cscope and egroup search on "goto.*\\").
Since "Goto" is in the name, it felt "reasonable" especially since
the
label is included as a parameter. If label wasn't a parameter, then my
opinion would have been different, especially seeing as how in certain
functions we will change between 'cleanup', 'error', and/or
'endjob'
depending on where we are in the function. So even though cleanup could
exist, if the code moved inside a job going to cleanup wouldn't be the
right thing (of course this logic also gives credence to your position
for not making this change).
In any case, I'll hold off on doing anything with this to see what other
opinions exist.
John
10:19 a.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
On Wed, Nov 23, 2016 at 11:17:13AM -0500, John Ferlan wrote:
On 11/23/2016 10:55 AM, Daniel P. Berrange wrote:
> On Wed, Nov 23, 2016 at 10:49:00AM -0500, John Ferlan wrote:
>>
>>
>> On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
>>> From: Chen Hanxiao <chenhanxiao(a)gmail.com>
>>>
>>> Introduce macro virCheckControllerGoto.
>>> Jumps to a label if unsupported controller type were
>>> passed to it.
>>>
>>> Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
>>> ---
>>> src/internal.h | 19 +++++++++++++++++++
>>> 1 file changed, 19 insertions(+)
>>>
>>> diff --git a/src/internal.h b/src/internal.h
>>> index d8cc5ad..a34f43e 100644
>>> --- a/src/internal.h
>>> +++ b/src/internal.h
>>> @@ -362,6 +362,25 @@
>>> } \
>>> } while (0)
>>>
>>> +/**
>>> + * virCheckControllerGoto:
>>> + * @Cgp: virCgroupPtr pointer
>>> + * @CgpCtr: cgroup controller type enum
>>> + * @label: label to jump to on error
>>> + *
>>> + * Returns nothing. Jumps to a label if unsupported controller type were
>>> + * passed to it.
>>> + */
>>> +# define virCheckControllerGoto(Cgp, CgpCtr, label) \
>>> + do { \
>>> + if (!virCgroupHasController(Cgp, CgpCtr)) { \
>>> + virReportError(VIR_ERR_OPERATION_INVALID, \
>>> + _("cgroup %s controller is not
mounted"), \
>> ^
>> Extra space
>>
>> s/%s/'%s'/
>>
>> ACK (I'll adjust before pushing)
>
> I'm personally *not* in favour of doing this. I don't think that macros
> should be used to hide control flow logic, especially when they're hiding
> 'goto'.
>
> Now, we do have some of this style macros for checking flags. I think those
> are more acceptable because flag checking is always done right at the
> start of the function.
>
> These cgroup macros by contrast will be placed at arbitrary locations
> in the middle of functions, hiding the fact that we're jumping right
> out.
>
> So personally I'm NACK on this series, unless there contrasting opinions
> people want to put forward.
>
I'm ambivalent - to a degree I saw this is as "similar to" places where
we create capitalized macro names and use the "goto" to jump control. In
most of those cases, the label *isn't* included in the macro as a
parameter (use cscope and egroup search on "goto.*\\").
Since "Goto" is in the name, it felt "reasonable" especially since
the
label is included as a parameter. If label wasn't a parameter, then my
opinion would have been different, especially seeing as how in certain
functions we will change between 'cleanup', 'error', and/or
'endjob'
depending on where we are in the function. So even though cleanup could
exist, if the code moved inside a job going to cleanup wouldn't be the
right thing (of course this logic also gives credence to your position
for not making this change).
For me it just doesn't fit with the way I scan code. When I'm looking
at the control flow/structure of a method I'm not closely reading the
method names to see if they contain the suffix "Goto" - I'm just scanning
the structure for normal C constructs for/if/while/goto.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
9:27 a.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
At 2016-11-24 00:19:42, "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Wed, Nov 23, 2016 at 11:17:13AM -0500, John Ferlan wrote:
>
>
> On 11/23/2016 10:55 AM, Daniel P. Berrange wrote:
> > On Wed, Nov 23, 2016 at 10:49:00AM -0500, John Ferlan wrote:
> >>
> >>
> >> On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
> >>> From: Chen Hanxiao <chenhanxiao(a)gmail.com>
> >>>
> >>> Introduce macro virCheckControllerGoto.
> >>> Jumps to a label if unsupported controller type were
> >>> passed to it.
> >>>
> >>> Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
> >>> ---
> >>> src/internal.h | 19 +++++++++++++++++++
> >>> 1 file changed, 19 insertions(+)
> >>>
> >>> diff --git a/src/internal.h b/src/internal.h
> >>> index d8cc5ad..a34f43e 100644
> >>> --- a/src/internal.h
> >>> +++ b/src/internal.h
> >>> @@ -362,6 +362,25 @@
> >>> }
\
> >>> } while (0)
> >>>
> >>> +/**
> >>> + * virCheckControllerGoto:
> >>> + * @Cgp: virCgroupPtr pointer
> >>> + * @CgpCtr: cgroup controller type enum
> >>> + * @label: label to jump to on error
> >>> + *
> >>> + * Returns nothing. Jumps to a label if unsupported controller type
were
> >>> + * passed to it.
> >>> + */
> >>> +# define virCheckControllerGoto(Cgp, CgpCtr, label)
\
> >>> + do {
\
> >>> + if (!virCgroupHasController(Cgp, CgpCtr)) {
\
> >>> + virReportError(VIR_ERR_OPERATION_INVALID,
\
> >>> + _("cgroup %s controller is not
mounted"), \
> >> ^
> >> Extra space
> >>
> >> s/%s/'%s'/
> >>
> >> ACK (I'll adjust before pushing)
> >
> > I'm personally *not* in favour of doing this. I don't think that macros
> > should be used to hide control flow logic, especially when they're hiding
> > 'goto'.
> >
> > Now, we do have some of this style macros for checking flags. I think those
> > are more acceptable because flag checking is always done right at the
> > start of the function.
> >
> > These cgroup macros by contrast will be placed at arbitrary locations
> > in the middle of functions, hiding the fact that we're jumping right
> > out.
> >
> > So personally I'm NACK on this series, unless there contrasting opinions
> > people want to put forward.
> >
>
> I'm ambivalent - to a degree I saw this is as "similar to" places
where
> we create capitalized macro names and use the "goto" to jump control. In
> most of those cases, the label *isn't* included in the macro as a
> parameter (use cscope and egroup search on "goto.*\\").
>
> Since "Goto" is in the name, it felt "reasonable" especially
since the
> label is included as a parameter. If label wasn't a parameter, then my
> opinion would have been different, especially seeing as how in certain
> functions we will change between 'cleanup', 'error', and/or
'endjob'
> depending on where we are in the function. So even though cleanup could
> exist, if the code moved inside a job going to cleanup wouldn't be the
> right thing (of course this logic also gives credence to your position
> for not making this change).
For me it just doesn't fit with the way I scan code. When I'm looking
at the control flow/structure of a method I'm not closely reading the
method names to see if they contain the suffix "Goto" - I'm just scanning
the structure for normal C constructs for/if/while/goto.
We had a lot of similar code for doing such kind of thing, such as:
virCheckControllerGoto.
There are many other xxxGoto in internal.h.
We had a lot of similar cgroup controller check works,
So a glance at the macros and its name may not bring too much troubles.
The macro in this set did help like other similar macros.
Regards,
- Chen
3:20 a.m.
New subject: [libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto
On Thu, Nov 24, 2016 at 09:27:51 +0800, Chen Hanxiao wrote:
At 2016-11-24 00:19:42, "Daniel P. Berrange"
<berrange(a)redhat.com> wrote:
>On Wed, Nov 23, 2016 at 11:17:13AM -0500, John Ferlan wrote:
>> On 11/23/2016 10:55 AM, Daniel P. Berrange wrote:
>> > On Wed, Nov 23, 2016 at 10:49:00AM -0500, John Ferlan wrote:
>> >> On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
>> >>> From: Chen Hanxiao <chenhanxiao(a)gmail.com>
>> >>>
>> >>> Introduce macro virCheckControllerGoto.
>> >>> Jumps to a label if unsupported controller type were
>> >>> passed to it.
>> >>>
>> >>> Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
>> >>> ---
>> >>> src/internal.h | 19 +++++++++++++++++++
>> >>> 1 file changed, 19 insertions(+)
>> >>>
>> >>> diff --git a/src/internal.h b/src/internal.h
>> >>> index d8cc5ad..a34f43e 100644
>> >>> --- a/src/internal.h
>> >>> +++ b/src/internal.h
>> >>> @@ -362,6 +362,25 @@
>> >>> }
\
>> >>> } while (0)
>> >>>
>> >>> +/**
>> >>> + * virCheckControllerGoto:
>> >>> + * @Cgp: virCgroupPtr pointer
>> >>> + * @CgpCtr: cgroup controller type enum
>> >>> + * @label: label to jump to on error
>> >>> + *
>> >>> + * Returns nothing. Jumps to a label if unsupported controller
type were
>> >>> + * passed to it.
>> >>> + */
>> >>> +# define virCheckControllerGoto(Cgp, CgpCtr, label)
\
>> >>> + do {
\
>> >>> + if (!virCgroupHasController(Cgp, CgpCtr)) {
\
>> >>> + virReportError(VIR_ERR_OPERATION_INVALID,
\
>> >>> + _("cgroup %s controller is not
mounted"), \
>> >> ^
>> >> Extra space
>> >>
>> >> s/%s/'%s'/
>> >>
>> >> ACK (I'll adjust before pushing)
>> >
>> > I'm personally *not* in favour of doing this. I don't think that
macros
>> > should be used to hide control flow logic, especially when they're
hiding
>> > 'goto'.
>> >
>> > Now, we do have some of this style macros for checking flags. I think
those
>> > are more acceptable because flag checking is always done right at the
>> > start of the function.
>> >
>> > These cgroup macros by contrast will be placed at arbitrary locations
>> > in the middle of functions, hiding the fact that we're jumping right
>> > out.
>> >
>> > So personally I'm NACK on this series, unless there contrasting
opinions
>> > people want to put forward.
>> >
>>
>> I'm ambivalent - to a degree I saw this is as "similar to" places
where
>> we create capitalized macro names and use the "goto" to jump control.
In
>> most of those cases, the label *isn't* included in the macro as a
>> parameter (use cscope and egroup search on "goto.*\\").
>>
>> Since "Goto" is in the name, it felt "reasonable" especially
since the
>> label is included as a parameter. If label wasn't a parameter, then my
>> opinion would have been different, especially seeing as how in certain
>> functions we will change between 'cleanup', 'error', and/or
'endjob'
>> depending on where we are in the function. So even though cleanup could
>> exist, if the code moved inside a job going to cleanup wouldn't be the
>> right thing (of course this logic also gives credence to your position
>> for not making this change).
>
>For me it just doesn't fit with the way I scan code. When I'm looking
>at the control flow/structure of a method I'm not closely reading the
>method names to see if they contain the suffix "Goto" - I'm just
scanning
>the structure for normal C constructs for/if/while/goto.
>
We had a lot of similar code for doing such kind of thing, such as:
virCheckControllerGoto.
There are many other xxxGoto in internal.h.
We had a lot of similar cgroup controller check works,
So a glance at the macros and its name may not bring too much troubles.
The macro in this set did help like other similar macros.
I agree with Dan here. We should not make the code look less C-like. I
originally did not want to rant about this but my preference is not to
do stuff like this.
I concur with Dan's NACK
10:09 p.m.
New subject: [libvirt] [PATCH 2/4] lxc_driver: use virCheckControllerGoto to simplify cgroup controller check
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Use macro virCheckControllerGoto
to simplify cgroup controller check codes.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/lxc/lxc_driver.c | 115 +++++++++++++++------------------------------------
1 file changed, 34 insertions(+), 81 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4a0165a..69cdd38 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -847,12 +847,9 @@ lxcDomainSetMemoryParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup memory controller is not mounted"));
- goto endjob;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, endjob);
#define VIR_GET_LIMIT_PARAMETER(PARAM, VALUE) \
if ((rc = virTypedParamsGetULLong(params, nparams, PARAM, &VALUE)) < 0) \
@@ -967,12 +964,9 @@ lxcDomainGetMemoryParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto cleanup;
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup memory controller is not
mounted"));
- goto cleanup;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, cleanup);
if ((*nparams) == 0) {
/* Current number of memory parameters supported by cgroups */
@@ -1863,11 +1857,8 @@ static char *lxcDomainGetSchedulerType(virDomainPtr dom,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (nparams) {
if (virCgroupSupportsCpuBW(priv->cgroup))
@@ -1990,13 +1981,9 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom,
goto endjob;
}
- if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto endjob;
- }
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, endjob);
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
@@ -2128,11 +2115,8 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom,
goto out;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (virCgroupGetCpuShares(priv->cgroup, &shares) < 0)
goto cleanup;
@@ -2388,11 +2372,8 @@ lxcDomainBlockStats(virDomainPtr dom,
goto endjob;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
if (!*path) {
/* empty path - return entire domain blkstats instead */
@@ -2474,11 +2455,8 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
goto endjob;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
if (!*path) {
/* empty path - return entire domain blkstats instead */
@@ -2611,13 +2589,9 @@ lxcDomainSetBlkioParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
ret = 0;
if (def) {
@@ -2818,11 +2792,8 @@ lxcDomainGetBlkioParameters(virDomainPtr dom,
goto cleanup;
if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, cleanup);
/* fill blkio weight here */
if (virCgroupGetBlkioWeight(priv->cgroup, &val) < 0)
@@ -3856,11 +3827,8 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
src = virDomainDiskGetSource(def);
if (src == NULL) {
@@ -4408,11 +4376,8 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm,
if (virAsprintf(&dst, "/dev/%s", def->dst) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (lxcDomainAttachDeviceUnlink(vm, dst) < 0) {
virDomainAuditDisk(vm, def->src, NULL, "detach", false);
@@ -4527,11 +4492,8 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver,
usbsrc->bus, usbsrc->device) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (!(usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL)))
goto cleanup;
@@ -4587,11 +4549,8 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (lxcDomainAttachDeviceUnlink(vm, def->source.caps.u.storage.block) < 0) {
virDomainAuditHostdev(vm, def, "detach", false);
@@ -4637,11 +4596,8 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (lxcDomainAttachDeviceUnlink(vm, def->source.caps.u.misc.chardev) < 0) {
virDomainAuditHostdev(vm, def, "detach", false);
@@ -5438,11 +5394,8 @@ lxcDomainGetCPUStats(virDomainPtr dom,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPUACCT controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUACCT, cleanup);
if (start_cpu == -1)
ret = virCgroupGetDomainTotalCpuStats(priv->cgroup,
--
1.8.3.1
9:49 a.m.
New subject: [libvirt] [PATCH 2/4] lxc_driver: use virCheckControllerGoto to simplify cgroup controller check
$SUBJ: long line... consider:
lxc: Use virCheckControllerGoto
Then for all "lxc" files changed, use this patch...
On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Use macro virCheckControllerGoto
to simplify cgroup controller check codes.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/lxc/lxc_driver.c | 115 +++++++++++++++------------------------------------
1 file changed, 34 insertions(+), 81 deletions(-)
I adjusted all the calls to put the VIR_CGROUP_CONTROLLER_ on the same
line as the virCheckControllerGoto. In some cases, putting all 3 args on
the same line was also possible.
ACK w/ the attached squashed in to change lxc_process.c for
virLXCProcessStart to use virCheckControllerGoto.
John
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4a0165a..69cdd38 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -847,12 +847,9 @@ lxcDomainSetMemoryParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup memory controller is not mounted"));
- goto endjob;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, endjob);
#define VIR_GET_LIMIT_PARAMETER(PARAM, VALUE) \
if ((rc = virTypedParamsGetULLong(params, nparams, PARAM, &VALUE)) < 0) \
@@ -967,12 +964,9 @@ lxcDomainGetMemoryParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto cleanup;
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup memory controller is not
mounted"));
- goto cleanup;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, cleanup);
if ((*nparams) == 0) {
/* Current number of memory parameters supported by cgroups */
@@ -1863,11 +1857,8 @@ static char *lxcDomainGetSchedulerType(virDomainPtr dom,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (nparams) {
if (virCgroupSupportsCpuBW(priv->cgroup))
@@ -1990,13 +1981,9 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom,
goto endjob;
}
- if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto endjob;
- }
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, endjob);
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
@@ -2128,11 +2115,8 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom,
goto out;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (virCgroupGetCpuShares(priv->cgroup, &shares) < 0)
goto cleanup;
@@ -2388,11 +2372,8 @@ lxcDomainBlockStats(virDomainPtr dom,
goto endjob;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
if (!*path) {
/* empty path - return entire domain blkstats instead */
@@ -2474,11 +2455,8 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
goto endjob;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
if (!*path) {
/* empty path - return entire domain blkstats instead */
@@ -2611,13 +2589,9 @@ lxcDomainSetBlkioParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
ret = 0;
if (def) {
@@ -2818,11 +2792,8 @@ lxcDomainGetBlkioParameters(virDomainPtr dom,
goto cleanup;
if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, cleanup);
/* fill blkio weight here */
if (virCgroupGetBlkioWeight(priv->cgroup, &val) < 0)
@@ -3856,11 +3827,8 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
src = virDomainDiskGetSource(def);
if (src == NULL) {
@@ -4408,11 +4376,8 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm,
if (virAsprintf(&dst, "/dev/%s", def->dst) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (lxcDomainAttachDeviceUnlink(vm, dst) < 0) {
virDomainAuditDisk(vm, def->src, NULL, "detach", false);
@@ -4527,11 +4492,8 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver,
usbsrc->bus, usbsrc->device) < 0)
goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (!(usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL)))
goto cleanup;
@@ -4587,11 +4549,8 @@ lxcDomainDetachDeviceHostdevStorageLive(virDomainObjPtr vm,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (lxcDomainAttachDeviceUnlink(vm, def->source.caps.u.storage.block) < 0) {
virDomainAuditHostdev(vm, def, "detach", false);
@@ -4637,11 +4596,8 @@ lxcDomainDetachDeviceHostdevMiscLive(virDomainObjPtr vm,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("devices cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_DEVICES, cleanup);
if (lxcDomainAttachDeviceUnlink(vm, def->source.caps.u.misc.chardev) < 0) {
virDomainAuditHostdev(vm, def, "detach", false);
@@ -5438,11 +5394,8 @@ lxcDomainGetCPUStats(virDomainPtr dom,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPUACCT controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUACCT, cleanup);
if (start_cpu == -1)
ret = virCgroupGetDomainTotalCpuStats(priv->cgroup,
10:09 p.m.
New subject: [libvirt] [PATCH 3/4] qemu_driver: use virCheckControllerGoto to simplify cgroup controller check
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Use macro virCheckControllerGoto
to simplify cgroup controller check codes.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/qemu/qemu_driver.c | 70 +++++++++++++++-----------------------------------
1 file changed, 21 insertions(+), 49 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 38c8414..4e75095 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8807,11 +8807,8 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (nparams) {
if (virCgroupSupportsCpuBW(priv->cgroup))
@@ -9061,13 +9058,9 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
- }
+ if (flags & VIR_DOMAIN_AFFECT_LIVE)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
ret = 0;
if (def) {
@@ -9277,11 +9270,8 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
goto cleanup;
if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, cleanup);
/* fill blkio weight here */
if (virCgroupGetBlkioWeight(priv->cgroup, &val) < 0)
@@ -9372,12 +9362,9 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup memory controller is not mounted"));
- goto endjob;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, endjob);
#define VIR_GET_LIMIT_PARAMETER(PARAM, VALUE) \
if ((rc = virTypedParamsGetULLong(params, nparams, PARAM, &VALUE)) < 0) \
@@ -9513,11 +9500,8 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
mem_soft_limit = persistentDef->mem.soft_limit;
swap_hard_limit = persistentDef->mem.swap_hard_limit;
} else {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup memory controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, cleanup);
if (virCgroupGetMemoryHardLimit(priv->cgroup, &mem_hard_limit) < 0)
goto cleanup;
@@ -9686,11 +9670,8 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
goto endjob;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup cpuset controller is not mounted"));
- goto endjob;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUSET, endjob);
if (mode != -1 &&
virDomainNumatuneGetMode(def->numa, -1, &config_mode) == 0 &&
@@ -10164,12 +10145,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
goto endjob;
}
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup CPU controller is not mounted"));
- goto endjob;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, endjob);
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
@@ -10573,11 +10551,8 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
if (persistentDef) {
data = persistentDef->cputune;
} else if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (virCgroupGetCpuShares(priv->cgroup, &data.shares) < 0)
goto cleanup;
@@ -18013,11 +17988,8 @@ qemuDomainGetCPUStats(virDomainPtr domain,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPUACCT controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUACCT, cleanup);
if (qemuDomainHasVcpuPids(vm) &&
!(guestvcpus = virDomainDefGetOnlineVcpumap(vm->def)))
--
1.8.3.1
9:49 a.m.
New subject: [libvirt] [PATCH 3/4] qemu_driver: use virCheckControllerGoto to simplify cgroup controller check
$SUBJ... shorten to:
qemu: Use virCheckControllerGoto
merge patch 3 and 4 together:
On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Use macro virCheckControllerGoto
to simplify cgroup controller check codes.
Use macro virCheckControllerGoto to simplify cgroup controller check calls.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/qemu/qemu_driver.c | 70 +++++++++++++++-----------------------------------
1 file changed, 21 insertions(+), 49 deletions(-)
I adjusted all the calls to put the VIR_CGROUP_CONTROLLER_ on the same
line as the virCheckControllerGoto. In some cases, putting all 3 args on
the same line was also possible.
ACK
John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 38c8414..4e75095 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8807,11 +8807,8 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (nparams) {
if (virCgroupSupportsCpuBW(priv->cgroup))
@@ -9061,13 +9058,9 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto endjob;
- }
- }
+ if (flags & VIR_DOMAIN_AFFECT_LIVE)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, endjob);
ret = 0;
if (def) {
@@ -9277,11 +9270,8 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
goto cleanup;
if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("blkio cgroup isn't mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_BLKIO, cleanup);
/* fill blkio weight here */
if (virCgroupGetBlkioWeight(priv->cgroup, &val) < 0)
@@ -9372,12 +9362,9 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup memory controller is not mounted"));
- goto endjob;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, endjob);
#define VIR_GET_LIMIT_PARAMETER(PARAM, VALUE) \
if ((rc = virTypedParamsGetULLong(params, nparams, PARAM, &VALUE)) < 0) \
@@ -9513,11 +9500,8 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
mem_soft_limit = persistentDef->mem.soft_limit;
swap_hard_limit = persistentDef->mem.swap_hard_limit;
} else {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup memory controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_MEMORY, cleanup);
if (virCgroupGetMemoryHardLimit(priv->cgroup, &mem_hard_limit) < 0)
goto cleanup;
@@ -9686,11 +9670,8 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
goto endjob;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup cpuset controller is not mounted"));
- goto endjob;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUSET, endjob);
if (mode != -1 &&
virDomainNumatuneGetMode(def->numa, -1, &config_mode) == 0
&&
@@ -10164,12 +10145,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
goto endjob;
}
- if (def &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cgroup CPU controller is not mounted"));
- goto endjob;
- }
+ if (def)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, endjob);
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
@@ -10573,11 +10551,8 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
if (persistentDef) {
data = persistentDef->cputune;
} else if (def) {
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPU controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
if (virCgroupGetCpuShares(priv->cgroup, &data.shares) < 0)
goto cleanup;
@@ -18013,11 +17988,8 @@ qemuDomainGetCPUStats(virDomainPtr domain,
goto cleanup;
}
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup CPUACCT controller is not
mounted"));
- goto cleanup;
- }
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPUACCT, cleanup);
if (qemuDomainHasVcpuPids(vm) &&
!(guestvcpus = virDomainDefGetOnlineVcpumap(vm->def)))
10:09 p.m.
New subject: [libvirt] [PATCH 4/4] qemu_process: use virCheckControllerGoto to simplify cgroup controller check
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Use macro virCheckControllerGoto
to simplify cgroup controller check codes.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/qemu/qemu_process.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1b67aee..4033de0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2330,12 +2330,9 @@ qemuProcessSetupPid(virDomainObjPtr vm,
char *mem_mask = NULL;
int ret = -1;
- if ((period || quota) &&
- !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("cgroup cpu is required for scheduler tuning"));
- goto cleanup;
- }
+ if (period || quota)
+ virCheckControllerGoto(priv->cgroup,
+ VIR_CGROUP_CONTROLLER_CPU, cleanup);
/* Infer which cpumask shall be used. */
if (cpumask)
--
1.8.3.1
9:50 a.m.
New subject: [libvirt] [PATCH 4/4] qemu_process: use virCheckControllerGoto to simplify cgroup controller check
On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Use macro virCheckControllerGoto
to simplify cgroup controller check codes.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
src/qemu/qemu_process.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Merging w/ patch 3
ACK
John
(will push shortly)
6:39 p.m.
At 2016-11-10 08:59:51, "Chen Hanxiao" <chen_han_xiao(a)126.com> wrote:
At 2016-11-04 11:09:16, "Chen Hanxiao" <chen_han_xiao(a)126.com> wrote:
>We had a lot of codes use the same style of cgroup controller check.
>This series introduce macro virCheckControllerGoto
>to simplify these kinds of check.
>
ping
ping
Regards,
- Chen
2920
days inactive
2940
days old
15 comments
4 participants
participants (4)
-
Chen Hanxiao -
Daniel P. Berrange -
John Ferlan -
Peter Krempa