From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lon Hohberger <lhh@redhat.com>
Date: Wed, 7 Sep 2011 14:07:45 -0400
Subject: [Cluster-devel] [PATCH] cman: improve cman/qdisk interactions
In-Reply-To: <1315401025-10670-1-git-send-email-fdinitto@redhat.com>
References: <1315401025-10670-1-git-send-email-fdinitto@redhat.com>
Message-ID: <20110907180745.GA5988@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi,

Good design -- couple of things...

On Wed, Sep 07, 2011 at 03:10:25PM +0200, Fabio M. Di Nitto wrote:
> - cman: use strdup instead of malloc+strcpy (code is more readable)

* Not really a necessary change, but ok.

> - libcman: perform better error checking in register_quorum_device/update_quorum_device
> 
> - Allow qdisk to update device name in cman using a new libcman quorum API call
> - Perform slight better error checking of some update opertaions
> 
> Resolves: rhbz#735917

* All in all, this seems like a lot of patch for a very tiny problem or
set of problems in a very narrow use case (renaming quorum device).


> +	if (quorum_device->name)
> +		free(quorum_device->name);
> +
> +	free(quorum_device);
> +
> +	quorum_device = NULL;
> +
> +	return;

* return is implied in functions returning void (no big deal; stylistic
stuff)

> +static void quorum_device_update_votes(int votes)
> +{
> +	int oldvotes;
> +
> +	/* Update votes even if it existed before */
> +	oldvotes = quorum_device->votes;
> +	quorum_device->votes = votes;
> +
> +	/* If it is a member and votes decreased, recalculate quorum */
> +	if (quorum_device->state == NODESTATE_MEMBER &&
> +	    oldvotes != votes) {
> +		recalculate_quorum(1, 0);
> +	}
> +}

* Code does not match comments; 'oldvotes != votes' is not a check
for decrease, and recalculate_quorum(1, 0) allows both increases and
decreases to votes.  Is the comment wrong or the code?  (I think the
comment is wrong...)


> +		if (errno == EBUSY) {
> +			logt_print(LOG_NOTICE, "quorum device is already registered, updating\n");
> +			ret = update_device(&ctx);
> +			if (ret) {
> +				logt_print(LOG_ERR, "DEBUG: unable to update quorum device info\n");
> +				goto out;
> +			}

* Is this a debug or an error message?  An error message with DEBUG: in
it is confusing; can we have one or the other?

-- 
Lon Hohberger - Red Hat, Inc.