On 01/17/2013 10:25 AM, Hitoshi Mitake wrote:
> sheep/cluster/shepherd.c | 704 ++++++++++++++++++++++++++++++++++++
> shepherd/Makefile.am | 44 +++
> shepherd/shepherd.c | 883 ++++++++++++++++++++++++++++++++++++++++++++++
Please separate cluster driver and sph daemon as two patches.
> +enum sph_msg_type {
> + _SPH_MSG_PADDING = 0, /* for making first member 1 */
> + SPH_MSG_JOIN,
simply assign SPH_MSG_JOIN = 1
> + SPH_MSG_JOIN_REPLY,
> + SPH_MSG_JOIN_RETRY,
> +
> + SPH_MSG_NEW_NODE,
> + SPH_MSG_NEW_NODE_REPLY,
> + SPH_MSG_NEW_NODE_FINISH,
> +
> + SPH_MSG_NOTIFY,
> + SPH_MSG_NOTIFY_FORWARD,
> +
> + SPH_MSG_BLOCK,
> + SPH_MSG_BLOCK_FORWARD,
> +
> + SPH_MSG_LEAVE,
> + SPH_MSG_LEAVE_FORWARD,
> +
> + SPH_MSG_MASTER_ELECTION,
> +
> + NR_SPH_MSG, /* this must be last one */
Seems that NR_SPH_MSG isn't used at all. If no outside user, I'd suggest
remove it.
> + iov[0].iov_base = &msg;
> + iov[0].iov_len = sizeof(msg);
> +
> + msg_join = xzalloc(msg_join_len);
> + msg_join->node = this_node;
> + memcpy(msg_join->opaque, kept_opaque, kept_opaque_len);
> +
> + iov[1].iov_base = msg_join;
> + iov[1].iov_len = msg_join_len;
I see a lot of iov handling code, I think you'd better add some iov
helpers to reduce the duplicate code. Also, as a general rule, if we
have one function larger than *60 lines*, we'd rethink the structure and
see if we can refactor it into several smaller (possible inline)
functions that do better self-description.
On 01/17/2013 10:25 AM, Hitoshi Mitake wrote:> +static void
init_addr_port(const char *s, struct sockaddr_in *addr)
> +{
> + /* format: <address>[:<port>] */
> + char *sep, *copied, *p;
> + uint8_t tmp_addr[16];
> + bool addr_only = false;
> +
> + copied = strdup(s);
> + if (!copied)
> + panic("strdup() failed: %m");
> +
> + sep = strchr(copied, ':');
> + if (!sep) {
> + addr_only = true;
> + goto addr_trans;
> + }
I think strtok will make the parsing easier.
On 01/17/2013 10:25 AM, Hitoshi Mitake wrote:> +static void
init_msg_handlers(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(msg_handlers); i++) {
> + if (msg_handlers[i])
> + continue;
> +
> + msg_handlers[i] = msg_invalid;
> + }
> +}
Do we really need this? Simply ignore the invalid message won't cause
trouble? I think you can re-structure enum sph_msg_type and put those
that have handlers at the front. Then we don't need install msg_invalid
handlers for invalid message.
> + vprintf(SDOG_INFO, "received op: %s\n", sph_msg_to_str(rcv.type));
> +
If you use SDOG_INFO intensively, I'd suggest add a new helper iprintf().
Also, any functions that calls printf should be prefixed as 'sph_' to
distinguish it from normal sheep functions.
printf helpers will print function name, so you don't need to embed
function name in those helpers.
Thanks,
Yuan
|
|