Re: [PATCH v3 3/5] config: report config parse errors using cb

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 23, 2023, 18:41 UTC
Message-ID: <20231023184137.994212-1-jonathantanmy@google.com>
In-Reply-To: <a888045c04d27864edf5751ea8641fdba596779c.1695330852.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 13 quoted lines

> From: Glen Choo <chooglen@google.com>
> 
> In a subsequent commit, config parsing will become its own library, and
> it's likely that the caller will want flexibility in handling errors
> (instead of being limited to the error handling we have in-tree).
> 
> Move the Git-specific error handling into a config_parser_event_fn_t
> that responds to config errors, and make git_parse_source() always
> return -1 (careful inspection shows that it was always returning -1
> already). This makes CONFIG_ERROR_SILENT obsolete since that is
> equivalent to not specifying an error event listener. Also, remove
> CONFIG_ERROR_UNSET and the config_source 'default', since all callers
> are now expected to specify the error handling they want.

I think this has to be better explained. So:

- There is already a config_parser_event_fn_t that can be configured
by a user to receive emitted config events. This callback can return
negative to halt further config parsing.

- Currently, it is git_parse_source() that detects when an error
occurs, and it emits a CONFIG_EVENT_ERROR and either dies, prints
an error, or swallows the error depending on error_action; no
matter what error_action is, it halts config parsing, as one would
expect. This commit moves the die/print/swallow handling to a
config_parser_event_fn_t that will see the CONFIG_EVENT_ERROR and die/
print/swallow.

- This new config_parser_event_fn_t does not need to swallow, since
that's the same as not passing in a callback. So it just needs to die/
print.

Show 26 quoted lines

> @@ -1039,6 +1042,29 @@ static int do_event(struct config_source *cs, enum config_event_t type,
>  	return 0;
>  }
>  
> +static int do_event_and_flush(struct config_source *cs,
> +			      enum config_event_t type,
> +			      struct parse_event_data *data)
> +{
> +	int maybe_ret;
> +
> +	if ((maybe_ret = flush_event(cs, type, data)) < 1)
> +		return maybe_ret;
> +
> +	start_event(cs, type, data);
> +
> +	if ((maybe_ret = flush_event(cs, type, data)) < 1)
> +		return maybe_ret;
> +
> +	/*
> +	 * Not actually EOF, but this indicates we don't have a valid event
> +	 * to flush next time around.
> +	 */
> +	data->previous_type = CONFIG_EVENT_EOF;
> +
> +	return 0;
> +}

A lot of this function only makes sense if the type is ERROR, so maybe rename this as flush_and_emit_error() (and don't take in a type). As it is, right now there is some confusion about how you can flush (I'm referring to the second flush) with the same type as what you passed to start_event().

Also, I don't think we should set data->previous_type here. Instead there should be a comment saying that if you're emitting ERROR, you should halt config parsing. The return value here is useless too (it signals whether we should halt config parsing, but the caller should always halt, so we don't need to return anything).