From mboxrd@z Thu Jan  1 00:00:00 1970
From: Fabio M. Di Nitto <fdinitto@redhat.com>
Date: Thu, 08 Sep 2011 08:39:49 +0200
Subject: [Cluster-devel] [PATCH] cman: improve cman/qdisk interactions
In-Reply-To: <20110907180745.GA5988@redhat.com>
References: <1315401025-10670-1-git-send-email-fdinitto@redhat.com>
	<20110907180745.GA5988@redhat.com>
Message-ID: <4E686335.4060000@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

On 9/7/2011 8:07 PM, Lon Hohberger wrote:
> 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.

Yeah I agree, I just didn?t like malloc + strcpy and lack of memset to
guarantee 0 byte end string.

> 
>> - 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).

It looks big only because I turn a lot of code in register_quorum_device
shared with update_quorum_device

So far there are 2 problems clearly identified. One is purely cosmetic
(renaming the device and show it correctly), the other might be less
cosmetic, where, after renaming a device, qdiskd cannot update votes in
cman anylonger. This is because the vote updates are pushed via
register_quorum, that after a rename, will always return -EBUSY.

I agree that both are corner cases for most users, but it?s also easy to
fix.

> 
> 
>> +	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)

Yes, I like it explicit but either way I can drop it.

> 
>> +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...)

I believe the comment is incorrect. That code has been copied pristine
from register_quorum and shared with update_quorum.

> 
> 
>> +		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?
> 

Plain oversight... it should be an error message since we are aborting
startup.

Fabio