[Cluster-devel] [PATCH] cman: improve cman/qdisk interactions
Fabio M. Di Nitto
fdinitto at redhat.com
Thu Sep 8 06:39:49 UTC 2011
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
More information about the Cluster-devel
mailing list