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

[Sunil Mushran]

On 06/23/2010 07:50 PM, Joel Becker wrote:
> 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.
>    

We intent to allow people to run this tool on a different system
entirely. So we cannot verify the uuid. And the / check will be
documented. As in, if you are specifying a device, ensure you
provide the full path.

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

duh!

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

Have added print for both cases.