Re: [PATCH 2/4] trace-cmd: Handle invalid opcode parsing gracefully

[Steven Rostedt]

On Fri, 2011-07-15 at 20:00 -0700, Vaibhav Nagarnaik wrote:
> If an invalid opcode is encountered, trace-cmd exits with an error.
> Instead it can be treated as a soft error where the event's print format
> is not parsed and its binary data is dumped out.
> 
> This patch adds a return value to arg_num_eval() function to indicate if
> the parsing was successful. If not, then the error is considered soft
> and the parsing of the offending event fails.
> 
> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@xxxxxxxxxx>

Thanks Vaibhav, applied!

-- Steve

> ---
>  parse-events.c |  125 +++++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 83 insertions(+), 42 deletions(-)
> 
> diff --git a/parse-events.c b/parse-events.c
> index d8f322a..58ffe51 100644
> --- a/parse-events.c
> +++ b/parse-events.c
> @@ -1901,90 +1901,120 @@ eval_type(unsigned long long val, struct print_arg *arg, int pointer)
>  	return eval_type_str(val, arg->typecast.type, pointer);
>  }
>  
> -static long long arg_num_eval(struct print_arg *arg)
> +static int arg_num_eval(struct print_arg *arg, long long *val)
>  {
>  	long long left, right;
> -	long long val = 0;
> +	int ret = 1;
>  
>  	switch (arg->type) {
>  	case PRINT_ATOM:
> -		val = strtoll(arg->atom.atom, NULL, 0);
> +		*val = strtoll(arg->atom.atom, NULL, 0);
>  		break;
>  	case PRINT_TYPE:
> -		val = arg_num_eval(arg->typecast.item);
> -		val = eval_type(val, arg, 0);
> +		ret = arg_num_eval(arg->typecast.item, val);
> +		if (!ret)
> +			break;
> +		*val = eval_type(*val, arg, 0);
>  		break;
>  	case PRINT_OP:
>  		switch (arg->op.op[0]) {
>  		case '|':
> -			left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> +			ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
>  			if (arg->op.op[1])
> -				val = left || right;
> +				*val = left || right;
>  			else
> -				val = left | right;
> +				*val = left | right;
>  			break;
>  		case '&':
> -			left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> +			ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
>  			if (arg->op.op[1])
> -				val = left && right;
> +				*val = left && right;
>  			else
> -				val = left & right;
> +				*val = left & right;
>  			break;
>  		case '<':
> -			left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> +			ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
>  			switch (arg->op.op[1]) {
>  			case 0:
> -				val = left < right;
> +				*val = left < right;
>  				break;
>  			case '<':
> -				val = left << right;
> +				*val = left << right;
>  				break;
>  			case '=':
> -				val = left <= right;
> +				*val = left <= right;
>  				break;
>  			default:
> -				die("unknown op '%s'", arg->op.op);
> +				do_warning("unknown op '%s'", arg->op.op);
> +				ret = 0;
>  			}
>  			break;
>  		case '>':
> -			left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> +			ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
>  			switch (arg->op.op[1]) {
>  			case 0:
> -				val = left > right;
> +				*val = left > right;
>  				break;
>  			case '>':
> -				val = left >> right;
> +				*val = left >> right;
>  				break;
>  			case '=':
> -				val = left >= right;
> +				*val = left >= right;
>  				break;
>  			default:
> -				die("unknown op '%s'", arg->op.op);
> +				do_warning("unknown op '%s'", arg->op.op);
> +				ret = 0;
>  			}
>  			break;
>  		case '=':
> -			left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> -
> -			if (arg->op.op[1] != '=')
> -				die("unknown op '%s'", arg->op.op);
> +			ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
>  
> -			val = left == right;
> +			if (arg->op.op[1] != '=') {
> +				do_warning("unknown op '%s'", arg->op.op);
> +				ret = 0;
> +			} else
> +				*val = left == right;
>  			break;
>  		case '!':
> -			left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> +			ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
>  
>  			switch (arg->op.op[1]) {
>  			case '=':
> -				val = left != right;
> +				*val = left != right;
>  				break;
>  			default:
> -				die("unknown op '%s'", arg->op.op);
> +				do_warning("unknown op '%s'", arg->op.op);
> +				ret = 0;
>  			}
>  			break;
>  		case '-':
> @@ -1992,12 +2022,17 @@ static long long arg_num_eval(struct print_arg *arg)
>  			if (arg->op.left->type == PRINT_NULL)
>  				left = 0;
>  			else
> -				left = arg_num_eval(arg->op.left);
> -			right = arg_num_eval(arg->op.right);
> -			val = left - right;
> +				ret = arg_num_eval(arg->op.left, &left);
> +			if (!ret)
> +				break;
> +			ret = arg_num_eval(arg->op.right, &right);
> +			if (!ret)
> +				break;
> +			*val = left - right;
>  			break;
>  		default:
> -			die("unknown op '%s'", arg->op.op);
> +			do_warning("unknown op '%s'", arg->op.op);
> +			ret = 0;
>  		}
>  		break;
>  
> @@ -2006,10 +2041,11 @@ static long long arg_num_eval(struct print_arg *arg)
>  	case PRINT_STRING:
>  	case PRINT_BSTRING:
>  	default:
> -		die("invalid eval type %d", arg->type);
> +		do_warning("invalid eval type %d", arg->type);
> +		ret = 0;
>  
>  	}
> -	return val;
> +	return ret;
>  }
>  
>  static char *arg_eval (struct print_arg *arg)
> @@ -2023,7 +2059,8 @@ static char *arg_eval (struct print_arg *arg)
>  	case PRINT_TYPE:
>  		return arg_eval(arg->typecast.item);
>  	case PRINT_OP:
> -		val = arg_num_eval(arg);
> +		if (!arg_num_eval(arg, &val))
> +			break;
>  		sprintf(buf, "%lld", val);
>  		return buf;
>  
> @@ -2065,6 +2102,8 @@ process_fields(struct event_format *event, struct print_flag_sym **list, char **
>  		memset(field, 0, sizeof(field));
>  
>  		value = arg_eval(arg);
> +		if (value == NULL)
> +			goto out_free;
>  		field->value = strdup(value);
>  
>  		free_arg(arg);
> @@ -2076,6 +2115,8 @@ process_fields(struct event_format *event, struct print_flag_sym **list, char **
>  			goto out_free;
>  
>  		value = arg_eval(arg);
> +		if (value == NULL)
> +			goto out_free;
>  		field->str = strdup(value);
>  		free_arg(arg);
>  		arg = NULL;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/