[libvirt] [PATCH 28/34] conf: Extract code for parsing thread resource scheduler info

[John Ferlan]

On 01/14/2016 11:27 AM, Peter Krempa wrote:
> As the scheduler info elements are represented orthogonally to how it
> makes sense to actually store the information, the extracted code will
> be later used when converting between XML and internal definitions.
> ---
>  src/conf/domain_conf.c | 69 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 24 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 14b6c80..b4f6fe9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14521,36 +14521,34 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>      return ret;
>  }
> 
> -static int
> -virDomainThreadSchedParse(xmlNodePtr node,
> -                          unsigned int minid,
> -                          unsigned int maxid,
> -                          const char *name,
> -                          virDomainThreadSchedParamPtr sp)
> +
> +static virBitmapPtr
> +virDomainSchedulerParse(xmlNodePtr node,
> +                        const char *name,
> +                        virProcessSchedPolicy *policy,
> +                        int *priority)

I see 'sp' being changed to two args by reference is necessary for patch
29...

>  {
> +    virBitmapPtr ret = NULL;
>      char *tmp = NULL;
>      int pol = 0;
> 
> -    tmp = virXMLPropString(node, name);
> -    if (!tmp) {
> +    if (!(tmp = virXMLPropString(node, name))) {
>          virReportError(VIR_ERR_XML_ERROR,
>                         _("Missing attribute '%s' in element '%sched'"),

Considering what I'll note later, this needs to change to "Missing
attribute '%s' in element '%sched'"

>                         name, name);
>          goto error;
>      }
> 
> -    if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0)
> +    if (virBitmapParse(tmp, 0, &ret, VIR_DOMAIN_CPUMASK_LEN) < 0)
>          goto error;
> 
> -    if (virBitmapIsAllClear(sp->ids) ||
> -        virBitmapNextSetBit(sp->ids, -1) < minid ||
> -        virBitmapLastSetBit(sp->ids) > maxid) {
> -
> +    if (virBitmapIsAllClear(ret)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("Invalid value of '%s': %s"),
> +                       _("'%s' scheduler bitmap '%s' is empty"),
>                         name, tmp);
>          goto error;
>      }
> +
>      VIR_FREE(tmp);
> 
>      if (!(tmp = virXMLPropString(node, "scheduler"))) {
> @@ -14561,22 +14559,22 @@ virDomainThreadSchedParse(xmlNodePtr node,
> 
>      if ((pol = virProcessSchedPolicyTypeFromString(tmp)) <= 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Invalid scheduler attribute: '%s'"),
> -                       tmp);
> +                       _("Invalid scheduler attribute: '%s'"), tmp);
>          goto error;
>      }
> -    sp->policy = pol;
> +    *policy = pol;
> 
>      VIR_FREE(tmp);
> -    if (sp->policy == VIR_PROC_POLICY_FIFO ||
> -        sp->policy == VIR_PROC_POLICY_RR) {
> -        tmp = virXMLPropString(node, "priority");
> -        if (!tmp) {
> +
> +    if (pol == VIR_PROC_POLICY_FIFO ||
> +        pol == VIR_PROC_POLICY_RR) {
> +        if (!(tmp = virXMLPropString(node, "priority"))) {
>              virReportError(VIR_ERR_XML_ERROR, "%s",
>                             _("Missing scheduler priority"));
>              goto error;
>          }
> -        if (virStrToLong_i(tmp, NULL, 10, &sp->priority) < 0) {
> +
> +        if (virStrToLong_i(tmp, NULL, 10, priority) < 0) {
>              virReportError(VIR_ERR_XML_ERROR, "%s",
>                             _("Invalid value for element priority"));
>              goto error;
> @@ -14584,11 +14582,34 @@ virDomainThreadSchedParse(xmlNodePtr node,
>          VIR_FREE(tmp);
>      }
> 
> -    return 0;
> +    return ret;
> 
>   error:
>      VIR_FREE(tmp);
> -    return -1;
> +    virBitmapFree(ret);
> +    return NULL;
> +}
> +
> +
> +static int
> +virDomainThreadSchedParse(xmlNodePtr node,
> +                          unsigned int minid,
> +                          unsigned int maxid,
> +                          const char *name,
> +                          virDomainThreadSchedParamPtr sp)
> +{
> +    if (!(sp->ids = virDomainSchedulerParse(node, name, &sp->policy,
> +                                            &sp->priority)))
> +        return -1;
> +
> +    if (virBitmapNextSetBit(sp->ids, -1) < minid ||
> +        virBitmapLastSetBit(sp->ids) > maxid) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("%ssched bitmap is out of range"), name);

Ahh - I see this is setting up for a future patch where %s will be
"vcpus" or "iothreads"... It's the attribute name found in the XML for
the <iothreadsched iothreads='xxx'" or "<vcpusched vcpus='xxx'"

Wasn't really "clear" without a description and without peeking ahead a
few patches. I think the message should be:

"%sched bitmap is out of range" since 'name' is passed as "vcpus" or
"iothreads" (with the 's' already). The resultant element name of
vcpusched or iothreadsched is then generated (similar to how it's done
in virDomainFormatSchedDef).


John

> +        return -1;
> +    }
> +
> +    return 0;
>  }
> 
>