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@xxxxxxxxxx>
> ---
> 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"),
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list