Date:	Sun, 11 May 2008 02:31:18 +0200
From:	Tomasz Grobelny <tomasz@...belny.oswiecenia.net>
To:	Gerrit Renker <gerrit@....abdn.ac.uk>
Cc:	acme@...hat.com, dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] [QPOLICY]: cmsg header parsing fixes

Dnia Friday 09 of May 2008, Gerrit Renker napisał:
> | > | > | -		default:
> | > | > | -			return -EINVAL;
>
> <...>
>
> | > By taking them out, you are robbing yourself of the possibility to
> | > distinguish between truly invalid arguments ("EINVALs") and
> | > incompatibilities.
> |
> | But do we need such distinction? If kernel cannot understand part of cmsg
> | data we should just ignore this part. The application will work, maybe
> | slightly less effective than the author intended but it's better than
> | returning error and making it completely useless. This could be called
> | best-effort approach. However, we could return error on unknown or
> | invalid data provided applications can detect which parameters are
> | supported prior to packet submission.
> |
> | To summarise I see two options here:
> | 1. Provide a way for applications to ask at runtime which DCCP_SCM_xxx
> | options are supported by running kernel. And always return EINVAL on
> | unsupported data.
> | 2. Allow ignoring unknown data, it doesn't have to be the default. This
> | could take a form of setsockopt(IGNORE_UNKNOWN_CMSG_DATA) call.
> |
> | Both ways the default would be as you propose but applications would be
> | able to work around older kernel version. What do you think about it?
>
> With that explanation I understand clearer where you would like to go. I
> don't like (2) since it makes the API more complex (instead of thinking
> what to accept, the API then also needs to know what to ignore).
>
It just ignores everything that it cannot understand, I don't see anything 
complex about that. At least when it comes to implemetation. But I understand 
that applications should always provide correct parameters. So let's agree 
that (1) is to be implemented in future.

> So I'd prefer (1). Like the struct qpolicy_args, this could be something
> done later, when there are more qpolicies.
>
> A similar mechanism was used for the CCIDs with
> ccid_getsockopt_available_ccids(), which would be one possibility -
> returning an array of possible policy IDs.
>
List of possible qpolicies is one thing, the other is a list of parameters 
supported by choosen qpolicy.

> I don't know what your extension plans are, my suggestion is to wrap up
> the patches in the qpolicy subtree, cast them as one patch within the
> test tree and continue any additions/further work based on that.
>
Agreed, the only thing that may need correction now is this line:
	skb->priority = *(__u32 *)CMSG_DATA(cmsg);
But I really don't know how it should look like, and I have neither knowledge 
nor machines to test on other architectures. Other than that we could 
consider the patch ready.

As for my extension plans, I can see a few things that could be improved:
 - adding timeout parameter,
 - providing information about available qpolicies and parameters to 
userspace,
 - providing statistical information about qpolicy to userspace applications.
(That doesn't necessarily mean I wish to write patches for all these 
features.)
-- 
Regards,
Tomasz Grobelny
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists