Re: [PATCHv4 10/16] staging: usbip: TLS for all userspace communication

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sat, Oct 19, 2013 at 04:39:13PM +0200, Dominik Paulus wrote:
> @@ -104,8 +105,10 @@ static int import_device(int sockfd, struct usbip_usb_device *udev)
>  		return -1;
>  	}
>  
> -	rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> +	usbip_net_bye(conn);
> +	rc = usbip_vhci_attach_device(port, conn->sockfd, udev->busnum,
>  				      udev->devnum, udev->speed);
> +
>  	if (rc < 0) {

Don't put a blank line between the function call and the check.  They
logically are one idea.

>  
> -	rc = usbip_net_recv(sockfd, (void *) &reply, sizeof(reply));
> +	rc = usbip_net_recv(conn, (void *) &reply, sizeof(reply));

There is no need to cast to void here, btw.  That's just noise.

>  	do {
> -		if (sending)
> -			nbytes = send(sockfd, buff, bufflen, 0);
> +		if (!conn->have_crypto && sending)
> +			nbytes = send(conn->sockfd, buff, bufflen, 0);
> +		else if (!conn->have_crypto && !sending)
> +			nbytes = recv(conn->sockfd, buff, bufflen, MSG_WAITALL);
> +#ifdef HAVE_GNUTLS
> +		else if (sending)
> +			nbytes = gnutls_record_send(conn->session, buff, bufflen);
>  		else
> -			nbytes = recv(sockfd, buff, bufflen, MSG_WAITALL);
> +			nbytes = gnutls_record_recv(conn->session, buff, bufflen);
> +#else
> +		/*
> +		 * Assertion to let gcc be able to infer proper initialization
> +		 * of nbytes.
> +		 */
> +		assert(!conn->have_crypto);
> +#endif

This is messy and I feel like it should be abstracted into a function
so we can hide the ifdef in a header file.

		if (sending)
			nbytes = usbip_send(conn, buff, bufflen, 0);
		else
			nbytes = usbip_recv(...

We'd still have the ifdef but hidden away.


> +int usbip_net_srp_server_handshake(struct usbip_connection *conn)
> +{
> +	int ret;
> +
> +	if (gnutls_init(&conn->session, GNUTLS_SERVER) != 0)
> +		return -1;
> +	gnutls_priority_set_direct(conn->session, "NORMAL:-KX-ALL:+SRP", NULL);
> +	if (gnutls_credentials_set(conn->session, GNUTLS_CRD_SRP,
> +		usbip_net_srp_cred) != 0)
> +		return -1;
> +

Kernel style is more beautiful:

	ret = gnutls_credentials_set(conn->session, GNUTLS_CRD_SRP,
				     usbip_net_srp_cred);
	if (ret)
		return ret;

> +void usbip_net_bye(struct usbip_connection *conn)
> +{
> +#ifdef HAVE_GNUTLS
> +	if (conn->have_crypto) {
> +		gnutls_bye(conn->session, GNUTLS_SHUT_RDWR);
> +
> +		gnutls_deinit(conn->session);
> +		if (!conn->server)
> +			gnutls_srp_free_client_credentials(conn->srp_client_cred);
> +
> +		conn->have_crypto = 0;
> +	}
> +#else
> +	(void)conn;

What is this about?  I assume that GCC warns, but which version of GCC
are you using because that sounds horrible.

> +#endif

regards,
dan carpenter