The code for determining resctrl allocation path could be reused
for monitor. Refactor it for reusing.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
---
src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index c9a79f7..03001cc 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl,
}
+static char *
+virResctrlDeterminePath(const char *pathparent,
+ const char *prefix,
+ const char *id)
+{
+ char *path = NULL;
+
+ if (!id) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Resctrl resource ID must be set before creation"));
+ return NULL;
+ }
+
+ if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
+ return NULL;
+
+ return path;
+}
+
+
int
virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
const char *machinename)
@@ -2274,15 +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
if (!alloc)
return 0;
- if (!alloc->id) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Resctrl Allocation ID must be set before creation"));
+ if (alloc->path) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Resctrl group path is expected to be NULL"));
return -1;
}
- if (!alloc->path &&
- virAsprintf(&alloc->path, "%s/%s-%s",
- SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
+ alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
+ machinename,
+ alloc->id);
+ if (!alloc->path)
return -1;
return 0;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> The code for determining resctrl allocation path could be reused
> for monitor. Refactor it for reusing.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> ---
> src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------
> 1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index c9a79f7..03001cc 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl,
> }
>
>
> +static char *
> +virResctrlDeterminePath(const char *pathparent,
s/pathparent/parentpath
> + const char *prefix,
> + const char *id)
> +{
> + char *path = NULL;
> +
> + if (!id) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Resctrl resource ID must be set before creation"));
s/Resctrl resource ID/'%s' resctrl ID/
where %s is @parentpath
Working in the @parentpath, then we'd know which it was Alloc or Monitor
> + return NULL;
> + }
> +
> + if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
parentpath
> + return NULL;
> +
> + return path;
> +}
> +
> +
> int
> virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> const char *machinename)
> @@ -2274,15 +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> if (!alloc)
> return 0;
>
> - if (!alloc->id) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Resctrl Allocation ID must be set before creation"));
> + if (alloc->path) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Resctrl group path is expected to be NULL"));
Resctrl alloc group path is already set '%s'
I think this is an internal error not an invalid arg, too
John
> return -1;
> }
>
> - if (!alloc->path &&
> - virAsprintf(&alloc->path, "%s/%s-%s",
> - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
> + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
> + machinename,
> + alloc->id);
> + if (!alloc->path)
> return -1;
>
> return 0;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> -----Original Message-----
> From: John Ferlan [mailto:jferlan@redhat.com]
> Sent: Wednesday, October 10, 2018 7:09 AM
> To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com
> Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>;
> Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com>
> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining
> allocation path
>
>
>
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> > The code for determining resctrl allocation path could be reused for
> > monitor. Refactor it for reusing.
> >
> > Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> > ---
> > src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------
> > 1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> > c9a79f7..03001cc 100644
> > --- a/src/util/virresctrl.c
> > +++ b/src/util/virresctrl.c
> > @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr
> > resctrl, }
> >
> >
> > +static char *
> > +virResctrlDeterminePath(const char *pathparent,
>
> s/pathparent/parentpath
>
OK.
> > + const char *prefix,
> > + const char *id) {
> > + char *path = NULL;
> > +
> > + if (!id) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("Resctrl resource ID must be set before
> > + creation"));
>
> s/Resctrl resource ID/'%s' resctrl ID/
>
> where %s is @parentpath
>
> Working in the @parentpath, then we'd know which it was Alloc or Monitor
How about changes like these?
if (!id) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Resctrl resource ID must be set before creation"));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl ID must be set before determining resctrl "
+ "path under '%s'"),
+ parentpath);
return NULL;
}
>
> > + return NULL;
> > + }
> > +
> > + if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
>
> parentpath
Will be renamed.
>
> > + return NULL;
> > +
> > + return path;
> > +}
> > +
> > +
> > int
> > virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> > const char *machinename) @@ -2274,15
> > +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> > if (!alloc)
> > return 0;
> >
> > - if (!alloc->id) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > - _("Resctrl Allocation ID must be set before creation"));
> > + if (alloc->path) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("Resctrl group path is expected to be
> > + NULL"));
>
> Resctrl alloc group path is already set '%s'
>
> I think this is an internal error not an invalid arg, too
Will be changed.
if (alloc->path) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("Resctrl group path is expected to be NULL"));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl allocation path is already set to '%s'"),
+ alloc->path);
return -1;
}
>
> John
>
Thanks for review.
Huaqiang
> > return -1;
> > }
> >
> > - if (!alloc->path &&
> > - virAsprintf(&alloc->path, "%s/%s-%s",
> > - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
> > + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
> > + machinename,
> > + alloc->id);
> > + if (!alloc->path)
> > return -1;
> >
> > return 0;
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/10/18 9:55 AM, Wang, Huaqiang wrote:
>
>
>> -----Original Message-----
>> From: John Ferlan [mailto:jferlan@redhat.com]
>> Sent: Wednesday, October 10, 2018 7:09 AM
>> To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com
>> Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>;
>> Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com>
>> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining
>> allocation path
>>
>>
>>
>> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>>> The code for determining resctrl allocation path could be reused for
>>> monitor. Refactor it for reusing.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
>>> ---
>>> src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------
>>> 1 file changed, 27 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
>>> c9a79f7..03001cc 100644
>>> --- a/src/util/virresctrl.c
>>> +++ b/src/util/virresctrl.c
>>> @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr
>>> resctrl, }
>>>
>>>
>>> +static char *
>>> +virResctrlDeterminePath(const char *pathparent,
>>
>> s/pathparent/parentpath
>>
>
> OK.
>
>>> + const char *prefix,
>>> + const char *id) {
>>> + char *path = NULL;
>>> +
>>> + if (!id) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> + _("Resctrl resource ID must be set before
>>> + creation"));
>>
>> s/Resctrl resource ID/'%s' resctrl ID/
>>
>> where %s is @parentpath
>>
>> Working in the @parentpath, then we'd know which it was Alloc or Monitor
>
> How about changes like these?
>
> if (!id) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Resctrl resource ID must be set before creation"));
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Resctrl ID must be set before determining resctrl "
> + "path under '%s'"),
> + parentpath);
> return NULL;
> }
>
Seems reasonable... although instead of "path under", just go with
"parentpath='%s'"
John
>
>>
>>> + return NULL;
>>> + }
>>> +
>>> + if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
>>
>> parentpath
>
> Will be renamed.
>
>>
>>> + return NULL;
>>> +
>>> + return path;
>>> +}
>>> +
>>> +
>>> int
>>> virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
>>> const char *machinename) @@ -2274,15
>>> +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
>>> if (!alloc)
>>> return 0;
>>>
>>> - if (!alloc->id) {
>>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> - _("Resctrl Allocation ID must be set before creation"));
>>> + if (alloc->path) {
>>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> + _("Resctrl group path is expected to be
>>> + NULL"));
>>
>> Resctrl alloc group path is already set '%s'
>>
>> I think this is an internal error not an invalid arg, too
>
> Will be changed.
>
> if (alloc->path) {
> - virReportError(VIR_ERR_INVALID_ARG, "%s",
> - _("Resctrl group path is expected to be NULL"));
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Resctrl allocation path is already set to '%s'"),
> + alloc->path);
> return -1;
> }
>
>>
>> John
>>
>
>
> Thanks for review.
> Huaqiang
>
>>> return -1;
>>> }
>>>
>>> - if (!alloc->path &&
>>> - virAsprintf(&alloc->path, "%s/%s-%s",
>>> - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
>>> + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
>>> + machinename,
>>> + alloc->id);
>>> + if (!alloc->path)
>>> return -1;
>>>
>>> return 0;
>>>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> -----Original Message-----
> From: John Ferlan [mailto:jferlan@redhat.com]
> Sent: Thursday, October 11, 2018 5:41 AM
> To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com
> Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>;
> Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com>
> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining
> allocation path
>
>
>
> On 10/10/18 9:55 AM, Wang, Huaqiang wrote:
> >
> >
> >> -----Original Message-----
> >> From: John Ferlan [mailto:jferlan@redhat.com]
> >> Sent: Wednesday, October 10, 2018 7:09 AM
> >> To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com
> >> Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing
> >> <bing.niu@intel.com>; Ding, Jian-feng <jian-feng.ding@intel.com>;
> >> Zang, Rui <rui.zang@intel.com>
> >> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for
> >> determining allocation path
> >>
> >>
> >>
> >> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> >>> The code for determining resctrl allocation path could be reused for
> >>> monitor. Refactor it for reusing.
> >>>
> >>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> >>> ---
> >>> src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------
> >>> 1 file changed, 27 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
> >>> c9a79f7..03001cc 100644
> >>> --- a/src/util/virresctrl.c
> >>> +++ b/src/util/virresctrl.c
> >>> @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr
> >>> resctrl, }
> >>>
> >>>
> >>> +static char *
> >>> +virResctrlDeterminePath(const char *pathparent,
> >>
> >> s/pathparent/parentpath
> >>
> >
> > OK.
> >
> >>> + const char *prefix,
> >>> + const char *id) {
> >>> + char *path = NULL;
> >>> +
> >>> + if (!id) {
> >>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >>> + _("Resctrl resource ID must be set before
> >>> + creation"));
> >>
> >> s/Resctrl resource ID/'%s' resctrl ID/
> >>
> >> where %s is @parentpath
> >>
> >> Working in the @parentpath, then we'd know which it was Alloc or
> >> Monitor
> >
> > How about changes like these?
> >
> > if (!id) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > - _("Resctrl resource ID must be set before creation"));
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Resctrl ID must be set before determining resctrl "
> > + "path under '%s'"),
> > + parentpath);
> > return NULL;
> > }
> >
>
> Seems reasonable... although instead of "path under", just go with
> "parentpath='%s'"
Thanks for advice, will be fixed.
>
> John
>
> >
> >>
> >>> + return NULL;
> >>> + }
> >>> +
> >>> + if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
> >>
> >> parentpath
> >
> > Will be renamed.
> >
> >>
> >>> + return NULL;
> >>> +
> >>> + return path;
> >>> +}
> >>> +
> >>> +
> >>> int
> >>> virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> >>> const char *machinename) @@ -2274,15
> >>> +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
> >>> if (!alloc)
> >>> return 0;
> >>>
> >>> - if (!alloc->id) {
> >>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >>> - _("Resctrl Allocation ID must be set before creation"));
> >>> + if (alloc->path) {
> >>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> >>> + _("Resctrl group path is expected to be
> >>> + NULL"));
> >>
> >> Resctrl alloc group path is already set '%s'
> >>
> >> I think this is an internal error not an invalid arg, too
> >
> > Will be changed.
> >
> > if (alloc->path) {
> > - virReportError(VIR_ERR_INVALID_ARG, "%s",
> > - _("Resctrl group path is expected to be NULL"));
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Resctrl allocation path is already set to '%s'"),
> > + alloc->path);
> > return -1;
> > }
> >
> >>
> >> John
> >>
> >
> >
> > Thanks for review.
> > Huaqiang
> >
> >>> return -1;
> >>> }
> >>>
> >>> - if (!alloc->path &&
> >>> - virAsprintf(&alloc->path, "%s/%s-%s",
> >>> - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
> >>> + alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
> >>> + machinename,
> >>> + alloc->id);
> >>> + if (!alloc->path)
> >>> return -1;
> >>>
> >>> return 0;
> >>>
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.