| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20061130193806.GA3631@elf.ucw.cz>
Date: Thu, 30 Nov 2006 20:38:06 +0100
From: Pavel Machek <pavel@....cz>
To: Steven Whitehouse <swhiteho@...hat.com>
Cc: cluster-devel@...hat.com, linux-kernel@...r.kernel.org,
Patrick Caulfield <pcaulfie@...hat.com>
Subject: Re: [DLM] Add support for tcp communications [38/70]
Hi!
> From: Patrick Caulfield <pcaulfie@...hat.com>
> Date: Thu, 2 Nov 2006 11:19:21 -0500
> Subject: [PATCH] [DLM] Add support for tcp communications
>
> The following patch adds a TCP based communications layer
> to the DLM which is compile time selectable. The existing SCTP
> layer gives the advantage of allowing multihoming, whereas
> the TCP layer has been heavily tested in previous versions of
> the DLM and is known to be robust and therefore can be used as
> a baseline for performance testing.
>
> Signed-off-by: Patrick Caulfield <pcaulfie@...hat.com>
> Signed-off-by: Steven Whitehouse <swhiteho@...hat.com>
Some comments from coding-style-police...
> +struct cbuf {
> + unsigned base;
> + unsigned len;
> + unsigned mask;
> +};
I'd prefer unsigned int here.
> +struct connection {
> + struct socket *sock;
> + unsigned long flags;
> + struct page *rx_page;
> + atomic_t waiting_requests;
> + struct cbuf cb;
> + int eagain_flag;
> +};
Quite unusual indentiation style w.r.t. pointers...
> +static struct nodeinfo *nodeid2nodeinfo(int nodeid, gfp_t alloc)
> +{
> + struct nodeinfo *ni;
> + int r;
> + int n;
> +
> + down_read(&nodeinfo_lock);
> + ni = idr_find(&nodeinfo_idr, nodeid);
> + up_read(&nodeinfo_lock);
> +
> + if (!ni && alloc) {
> + down_write(&nodeinfo_lock);
> +
> + ni = idr_find(&nodeinfo_idr, nodeid);
> + if (ni)
> + goto out_up;
...
> + if (nodeid > max_nodeid)
> + max_nodeid = nodeid;
> + out_up:
> + up_write(&nodeinfo_lock);
> + }
> +
> + return ni;
Can we do if (ni || !alloc) return ni, and avoid gotos inside of
block?
> +/* Something happened to an association */
> +static void process_sctp_notification(struct msghdr *msg, char *buf)
> +{
> + union sctp_notification *sn = (union sctp_notification *)buf;
> +
> + if (sn->sn_header.sn_type == SCTP_ASSOC_CHANGE) {
> + switch (sn->sn_assoc_change.sac_state) {
> +
> + case SCTP_COMM_UP:
> + case SCTP_RESTART:
> + {
...
> + /* Retry INIT later */
> + ni = > assoc2nodeinfo(sn->sn_assoc_change.sac_assoc_id);
Too long lines, and rgetting rid of outer if should be really easy.
> +
> +/* Initiate an SCTP association. In theory we could just use sendmsg() on
> + the first IP address and it should work, but this allows us to set up the
> + association before sending any valuable data that we can't afford to lose.
> + It also keeps the send path clean as it can now always use the association ID */
Too long lines.
> +static void lowcomms_state_change(struct sock *sk)
> +{
> +/* struct connection *con = sock2con(sk); */
Can we get commented code removed?
> +
> + switch (sk->sk_state) {
> + case TCP_ESTABLISHED:
> + lowcomms_write_space(sk);
> + break;
> +
> + case TCP_FIN_WAIT1:
> + case TCP_FIN_WAIT2:
> + case TCP_TIME_WAIT:
> + case TCP_CLOSE:
> + case TCP_CLOSE_WAIT:
> + case TCP_LAST_ACK:
> + case TCP_CLOSING:
> + /* FIXME: I think this causes more trouble than it solves.
> + lowcomms wil reconnect anyway when there is something to
> + send. This just attempts reconnection if a node goes down!
> + */
> + /* lowcomms_connect_sock(con); */
Here too? Is the fixme still applicable?
> +{
> + saddr->ss_family = dlm_local_addr.ss_family;
> + if (saddr->ss_family == AF_INET) {
> + struct sockaddr_in *in4_addr = (struct sockaddr_in *)saddr;
> + in4_addr->sin_port = cpu_to_be16(port);
> + *addr_len = sizeof(struct sockaddr_in);
> + }
> + else {
} else { on one line, please.
...
> + if (ret < 0)
> + goto out_close;
> + CBUF_EAT(&con->cb, ret);
> +
> + if (CBUF_EMPTY(&con->cb) && !call_again_soon) {
> + __free_page(con->rx_page);
> + con->rx_page = NULL;
> + }
> +
> + out:
> + if (call_again_soon)
> + goto out_resched;
> + up_read(&con->sock_sem);
> + ret = 0;
> + goto out_ret;
Could we just return 0 here?
> + out_resched:
> + lowcomms_data_ready(con->sock->sk, 0);
> + up_read(&con->sock_sem);
> + ret = 0;
> + schedule();
> + goto out_ret;
And here?
> + out_close:
> + up_read(&con->sock_sem);
> + if (ret != -EAGAIN && !test_bit(CF_IS_OTHERCON, &con->flags)) {
> + close_connection(con, FALSE);
> + /* Reconnect when there is something to send */
> + }
> +
> + out_ret:
> + return ret;
> +static struct socket *create_listen_sock(struct connection *con, struct sockaddr_storage *saddr)
> +{
> + struct socket *sock = NULL;
> + mm_segment_t fs;
> + int result = 0;
> + int one = 1;
> + int addr_len;
Tabs vs spaces?
> + result = sock->ops->listen(sock, 5);
> + if (result < 0) {
> + printk("dlm: Can't listen on port %d\n", dlm_config.tcp_port);
> + sock_release(sock);
> + sock = NULL;
> + goto create_out;
> + }
> +
> + create_out:
> + return sock;
> +}
I seriously think you are overdoing the gotos.
> +/* Called from recovery when it knows that a node has
> + left the cluster */
> +int dlm_lowcomms_close(int nodeid)
> +{
> + struct connection *con;
> +
> + if (!connections)
> + goto out;
> +
> + log_print("closing connection to node %d", nodeid);
> + con = nodeid2con(nodeid, 0);
> + if (con) {
> + clean_one_writequeue(con);
> + close_connection(con, TRUE);
> + atomic_set(&con->waiting_requests, 0);
> + }
> + return 0;
> +
> + out:
> + return -1;
> +}
Could we just return -1 directly? Should it follow 0/-errno convention
other functions here use? Can we get rid of TRUE/FALSE macros?
> +/*
> + * Return the largest buffer size we can cope with.
> + */
> +int lowcomms_max_buffer_size(void)
> +{
> + return PAGE_CACHE_SIZE;
> +}
Uhuh.
> +/* This is quite likely to sleep... */
> +int dlm_lowcomms_start(void)
> +{
> + int error = 0;
> +
> + error = -ENOTCONN;
One statement?
> + connections = kmalloc(sizeof(struct connection *) *
> + NODE_INCREMENT, GFP_KERNEL);
> + if (!connections)
> + goto out;
> +
> + memset(connections, 0,
> + sizeof(struct connection *) * NODE_INCREMENT);
kzalloc?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists