Re: [PATCH net-next-2.6 v4 02/12] net-caif: add CAIF socket and configuration headers

[Marcel Holtmann <marcel@holtmann.org>]

Hi Sjur,

I think most issues have been resolved and this should be ready for
merging, but I am bit worried about the userspace API. Can we start a
bit smaller and extend it later? Especially the socket options worry me
a bit.

Dave, personally I would prefer if we can merge this without these
socket options. Since I am really missing the need for it.

> +/**
> + * enum caif_socket_opts - CAIF option values for getsockopt and setsockopt.
> + *
> + * @CAIFSO_LINK_SELECT:		Selector used if multiple CAIF Link layers are
> + *				available. Either a high bandwidth
> + *				link can be selected (CAIF_LINK_HIGH_BANDW) or
> + *				or a low latency link (CAIF_LINK_LOW_LATENCY).
> + *                              This option is of type u_int32_t.
> + *				Alternatively SO_BINDTODEVICE can be used.
> + *
> + * @CAIFSO_REQ_PARAM:		Used to set the request parameters for a
> + *				utility channel. (struct caif_param). This
> + *				option must be set before connecting.
> + *
> + * @CAIFSO_RSP_PARAM:		Gets the request parameters for a utility
> + *				channel. (struct caif_param). This option
> + *				is valid after a successful connect.

These two more look like a combination of setsockopt/getsockopt instead
of two socket options. Maybe it is leftover from a ioctl interface, but
socket options work differently.

Also the caif_param struct seems pointless. Socket options contain a
length parameter anyway. So why bother with a struct that is just a data
field and a length field.

> + * @CAIFSO_CHANNEL_ID:		Gets the channel id on a CAIF Channel.
> + *				This option is valid after a successful connect.
> + *				( u_int32_t)

Where is this used and what is it used for? Is this something that
shouldn't be better part of the sockaddr structure. Then you can use
getpeername for it?

> + * @CAIFSO_NEXT_PAKCET_LEN:	Gets the size of next received packet.
> + *				Value is 0 if no packet is available.
> + *				This option is valid after a successful connect.
> + *				( u_int32_t)

Typo. And why do we need this?

> + * @CAIFSO_MAX_PAKCET_LEN:	Gets the maximum packet size for this
> + *				connection. ( u_int32_t)

Isn't this more like SO_RCVBUF or SO_SNDBUF.

Regards

Marcel