[Ocfs2-tools-devel] [PATCH 12/25] o2cb: Add ops add-heartbeat, remove-heartbeat and heartbeat-mode

[Joel Becker]

On Wed, Jun 23, 2010 at 11:44:22AM -0700, Sunil Mushran wrote:
> +static int get_region(char *device, char **region)
> +{
> +	ocfs2_filesys *fs;
> +	errcode_t ret = 0;
> +
> +	*region = '\0';
> +
> +	if (*device != '/') {
> +		*region = strdup(device);
> +		goto bail;
> +	}

	What if someone is in /dev and asks to add-heartbeat sda1?
Just try opening the name.  Only if you get an error of
OCFS2_ET_NAMED_DEVICE_NOT_FOUND or OCFS2_ET_BAD_MAGIC do you check for
'/'.  If no '/', try it as a uuid.  Also, when someone passes a uuid,
you should be verifying that some device actually has that uuid.

> +	ret = ocfs2_open(device, OCFS2_FLAG_RO | OCFS2_FLAG_HEARTBEAT_DEV_OK,
> +			 0, 0, &fs);
> +	if (ret)
> +		goto bail;
> +
> +	if (!ret) {
> +		*region = strdup(fs->uuid_str);
> +		ocfs2_close(fs);
> +	}

	Why is there a !ret?  You already checked ret.

> +	if (*region)
> +		verbosef(VL_DEBUG, "Device '%s' has uuid '%s'\n", device,
> +			 *region);

	You print this if you found the uuid via device, but you print
no debugging info when you are passed a uuid.  If you add checking of
uuid->device, you can print this in both cases.

Joel

-- 

"Sometimes I think the surest sign intelligent
 life exists elsewhere in the universe is that
 none of it has tried to contact us."
                                -Calvin & Hobbes

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127