Re: [PATCH] btrfs-progs: lblkid wouldn't find non mapper path input

[Anand Jain <Anand.Jain@oracle.com>]

  Thanks for the comments. Sorry for the delay. more inline
  below. V2 has been sent out.



On 10/30/13 12:40 AM, Josef Bacik wrote:
> On Wed, Oct 30, 2013 at 12:34:50AM +0800, Anand Jain wrote:
>> A new test case when disk is unmounted and if the non mapper
>> disk path is given as the argument to the btrfs filesystem show<arg>
>> we still need this to work but lblkid will pull only mapper disks,
>> it won't match. So this will normalize the input to find btrfs
>> by fsid and pass it to the search.
>>
>> Signed-off-by: Anand Jain<anand.jain@oracle.com>
>> ---
>>   cmds-filesystem.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 files changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
>> index fcabdb0..3a494fd 100644
>> --- a/cmds-filesystem.c
>> +++ b/cmds-filesystem.c
>> @@ -37,6 +37,7 @@
>>   #include "version.h"
>>   #include "commands.h"
>>   #include "list_sort.h"
>> +#include "disk-io.h"
>>
>>   static const char * const filesystem_cmd_group_usage[] = {
>>   	"btrfs filesystem [<group>]<command>  [<args>]",
>> @@ -414,6 +415,39 @@ static int btrfs_scan_kernel(void *search)
>>   	return 0;
>>   }
>>
>> +static int dev_to_fsid(char *dev, __u8 *fsid)
>> +{
>> +	struct btrfs_super_block *disk_super;
>> +	char *buf;
>> +	int ret;
>> +	int fd;
>> +
>> +	buf = malloc(4096);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	fd = open(dev, O_RDONLY);
>> +	if (fd<  0) {
>> +		ret = -errno;
>> +		free(buf);
>> +		return ret;
>> +	}
>> +
>> +	disk_super = (struct btrfs_super_block *)buf;
>> +	ret = btrfs_read_dev_super(fd, disk_super,
>> +			BTRFS_SUPER_INFO_OFFSET);
>> +	if (ret)
>> +		goto out;
>> +
>> +	memcpy(fsid, disk_super->fsid, BTRFS_FSID_SIZE);
>> +	ret = 0;
>> +
>> +out:
>> +	close(fd);
>> +	free(buf);
>> +	return ret;
>> +}
>> +
>>   static const char * const cmd_show_usage[] = {
>>   	"btrfs filesystem show [options] [<path>|<uuid>|<device>|label]",
>>   	"Show the structure of a filesystem",
>> @@ -434,6 +468,8 @@ static int cmd_show(int argc, char **argv)
>>   	int type = 0;
>>   	char mp[BTRFS_PATH_NAME_MAX + 1];
>>   	char path[PATH_MAX];
>> +	__u8 fsid[BTRFS_FSID_SIZE];
>> +	char uuid_buf[37];
>>
>
> No magic numbers.

yep. but it followed the tradition

# egrep 37 *.c | egrep char | wc -l
14

later I shall write a new patch to change all 14 together
including this.

>>   	while (1) {
>>   		int long_index;
>> @@ -480,11 +516,21 @@ static int cmd_show(int argc, char **argv)
>>   				if (!ret)
>>   					/* given block dev is mounted*/
>>   					search = mp;
>> -				else
>> +				else {
>
> This isn't the right format, needs to add braces for the if part too.

  ok. fixed it. (looks like checkpatch.pl needs an update which failed
  to catch this).


>> +					ret = dev_to_fsid(search, fsid);
>> +					if (ret) {
>> +						fprintf(stderr,
>> +							"ERROR: No btrfs on %s\n",
>> +							search);
>> +						return 1;
>> +					}
>> +					uuid_unparse(fsid, uuid_buf);
>> +					search = uuid_buf;
>> +					type = BTRFS_ARG_UUID;
>>   					goto devs_only;
>> +				}
>>   			}
>> -		}
>> -		if (type == BTRFS_ARG_MNTPOINT) {
>> +		} else if (type == BTRFS_ARG_MNTPOINT) {
>
> This looks like it will break something if we fall through from above?  Thanks,


  Thanks for the comments. I reviewed again and I don't think so,
  here we need special handle if the input is a block dev
  (additionally if input is a mount-point we just want to finish
  the job). For rest of the input types (uuid or unknown => which could
  be a label) no special handling is required, just a fall though is
  fine. I have added the additional comments in the code.

Thanks Anand


> Josef
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html