[libvirt] [PATCH 6/9] virsh: Rework parseRateStr

[John Ferlan]

On 08/03/2015 02:39 AM, Michal Privoznik wrote:
> The function is used to parse a tuple delimited by commas into
> virNetDevBandwidth structure. So far only three out of fore
> fields are supported: average, peak and burst. The single missing
> field is floor. Well, the parsing works, but I think we can do
> better. Especially when we will need to parse floor too in very
> close future.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  tools/virsh-domain.c | 80 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index a61656f..bb40ddd 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -865,36 +865,58 @@ static const vshCmdOptDef opts_attach_interface[] = {
>  /* parse inbound and outbound which are in the format of
>   * 'average,peak,burst', in which peak and burst are optional,
>   * thus 'average,,burst' and 'average,peak' are also legal. */
> -static int parseRateStr(const char *rateStr, virNetDevBandwidthRatePtr rate)
> +static int parseRateStr(vshControl *ctl,
> +                        const char *rateStr,
> +                        virNetDevBandwidthRatePtr rate)
>  {
> -    const char *average = NULL;
> -    char *peak = NULL, *burst = NULL;
> +    char *token;
> +    char *next;
> +    char *saveptr;

My compiler complained about uninitialized value here especially when
used with strtok_r  (setting = NULL pacifies compiler).

ACK with the adjustment.

John

> +    enum {
> +        AVERAGE, PEAK, BURST
> +    } state;
> +    int ret = -1;
>  
> -    average = rateStr;
> -    if (!average)
> -        return -1;
> -    if (virStrToLong_ull(average, &peak, 10, &rate->average) < 0)
> +    if (!rateStr)
>          return -1;
>  
> -    /* peak will be updated to point to the end of rateStr in case
> -     * of 'average' */
> -    if (peak && *peak != '\0') {
> -        burst = strchr(peak + 1, ',');
> -        if (!(burst && (burst - peak == 1))) {
> -            if (virStrToLong_ull(peak + 1, &burst, 10, &rate->peak) < 0)
> -                return -1;
> +    next = vshStrdup(ctl, rateStr);
> +
> +    for (state = AVERAGE; state <= BURST; state++) {
> +        unsigned long long *tmp;
> +        const char *field_name;
> +
> +        if (!(token = strtok_r(next, ",", &saveptr)))
> +            break;
> +        next = NULL;
> +
> +        switch (state) {
> +        case AVERAGE:
> +            tmp = &rate->average;
> +            field_name = "average";
> +            break;
> +
> +        case PEAK:
> +            tmp = &rate->peak;
> +            field_name = "peak";
> +            break;
> +
> +        case BURST:
> +            tmp = &rate->burst;
> +            field_name = "burst";
> +            break;
>          }
>  
> -        /* burst will be updated to point to the end of rateStr in case
> -         * of 'average,peak' */
> -        if (burst && *burst != '\0') {
> -            if (virStrToLong_ull(burst + 1, NULL, 10, &rate->burst) < 0)
> -                return -1;
> +        if (virStrToLong_ullp(token, NULL, 10, tmp) < 0) {
> +            vshError(ctl, _("malformed %s field"), field_name);
> +            goto cleanup;
>          }
>      }
>  
> -
> -    return 0;
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(next);
> +    return ret;
>  }
>  
>  static bool
> @@ -952,10 +974,8 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  
>      if (inboundStr) {
>          memset(&inbound, 0, sizeof(inbound));
> -        if (parseRateStr(inboundStr, &inbound) < 0) {
> -            vshError(ctl, _("inbound format is incorrect"));
> +        if (parseRateStr(ctl, inboundStr, &inbound) < 0)
>              goto cleanup;
> -        }
>          if (inbound.average == 0) {
>              vshError(ctl, _("inbound average is mandatory"));
>              goto cleanup;
> @@ -963,10 +983,8 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>      }
>      if (outboundStr) {
>          memset(&outbound, 0, sizeof(outbound));
> -        if (parseRateStr(outboundStr, &outbound) < 0) {
> -            vshError(ctl, _("outbound format is incorrect"));
> +        if (parseRateStr(ctl, outboundStr, &outbound) < 0)
>              goto cleanup;
> -        }
>          if (outbound.average == 0) {
>              vshError(ctl, _("outbound average is mandatory"));
>              goto cleanup;
> @@ -3280,10 +3298,8 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
>      memset(&outbound, 0, sizeof(outbound));
>  
>      if (inboundStr) {
> -        if (parseRateStr(inboundStr, &inbound) < 0) {
> -            vshError(ctl, _("inbound format is incorrect"));
> +        if (parseRateStr(ctl, inboundStr, &inbound) < 0)
>              goto cleanup;
> -        }
>          /* we parse the rate as unsigned long long, but the API
>           * only accepts UINT */
>          if (inbound.average > UINT_MAX || inbound.peak > UINT_MAX ||
> @@ -3316,10 +3332,8 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
>      }
>  
>      if (outboundStr) {
> -        if (parseRateStr(outboundStr, &outbound) < 0) {
> -            vshError(ctl, _("outbound format is incorrect"));
> +        if (parseRateStr(ctl, outboundStr, &outbound) < 0)
>              goto cleanup;
> -        }
>          if (outbound.average > UINT_MAX || outbound.peak > UINT_MAX ||
>              outbound.burst > UINT_MAX) {
>              vshError(ctl, _("outbound rate larger than maximum %u"),
>