Re: [PATCH v6 4/5] promisor-remote: allow a client to check fields

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Christian Couder <christian.couder@xxxxxxxxx> writes:

> diff --git a/promisor-remote.c b/promisor-remote.c
> index ae2c49a0a0..501cb92391 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -388,6 +388,20 @@ static struct string_list *fields_sent(void)
>  	return &fields_list;
>  }
>  
> +static struct string_list *fields_checked(void)
> +{
> +	static struct string_list fields_list = STRING_LIST_INIT_NODUP;
> +	static int initialized = 0;

No need to explicitly 0 initialize "static int"; let BSS take care
of it.  Perhaps we should add an entry to CodingGuidelines if we do
not have one (#leftoverbits).

> +	if (!initialized) {
> +		fields_list.cmp = strcasecmp;
> +		fields_from_config(&fields_list, "promisor.checkFields");
> +		initialized = 1;
> +	}
> +
> +	return &fields_list;
> +}

OK.

> @@ -533,6 +547,61 @@ enum accept_promisor {
>  	ACCEPT_ALL
>  };
>  
> +static int match_field_against_config(const char *field, const char *value,
> +				      struct promisor_info *config_info)
> +{
> +	if (config_info->filter && !strcasecmp(field, promisor_field_filter))
> +		return !strcmp(config_info->filter, value);
> +	else if (config_info->token && !strcasecmp(field, promisor_field_token))
> +		return !strcmp(config_info->token, value);
> +
> +	return 0;
> +}
> +
> +static int all_fields_match(struct promisor_info *advertised,
> +			    struct string_list *config_info,
> +			    int in_list)
> +{
> +	struct string_list* fields = fields_checked();

Asterisk sticks to a variable, not type.  I.e.

	struct string_list *fields = ...;


> +	struct string_list_item *item_checked;
> +
> +	for_each_string_list_item(item_checked, fields) {
> +		int match = 0;
> +		const char *field = item_checked->string;
> +		const char *value = NULL;
> +		struct string_list_item *item;
> +
> +		if (!strcasecmp(field, promisor_field_filter))
> +			value = advertised->filter;
> +		else if (!strcasecmp(field, promisor_field_token))
> +			value = advertised->token;

Hmph, together with match_field_against_config(), do we really need
to have this case insensitive to begin with?  I would suggest making
it a habit to design the interface minimally, and not making case
insensitive comparison as default counts one of them.  If the
comparison were not case insensitive, we do not even need to have
this loop; rather we can just look up from the list of the fields
for an exact string (i.e. promisor_field_filter).  I do not know
offhand what other code will become easier to read and simpler by
such a change.

Thanks.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux