[Cluster-devel] [PATCH] cman: improve cman/qdisk interactions
Lon Hohberger
lhh at redhat.com
Wed Sep 7 18:07:45 UTC 2011
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.
More information about the Cluster-devel
mailing list