Code review comment for ~redriver/cloud-init:frbsd-azure-branch

Hi Scott,

I have removed the Azure specific check for "get_mount_info".
get_mount_info_freebsd() was invoked only if the normal parse_mount failed on FreeBSD platform.
What do you think?

--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -565,6 +565,10 @@ def is_ipv4(instr):
     return len(toks) == 4

+def is_FreeBSD():
+ return system_info()['platform'].startswith('FreeBSD')
+
+
 def get_cfg_option_bool(yobj, key, default=False):
     if key not in yobj:
         return default
@@ -2055,11 +2059,56 @@ def parse_mtab(path):
     return None

+def find_freebsd_part(label_part):
+ if label_part.startswith("/dev/label/"):
+ target_label = label_part[5:]
+ (label_part, err) = subp(['glabel', 'status', '-s'])
+ for labels in label_part.split("\n"):
+ items = labels.split()
+ if len(items) > 0 and items[0].startswith(target_label):
+ label_part = items[2]
+ break
+ label_part = str(label_part)
+ return label_part
+
+
+def get_path_dev_freebsd(path, mnt_list):
+ path_found = None
+ for line in mnt_list.split("\n"):
+ items = line.split()
+ if (len(items) > 2 and os.path.exists(items[1] + path)):
+ path_found = line
+ break
+ return path_found
+
+
+def get_mount_info_freebsd(path, log=LOG):
+ (result, err) = subp(['mount', '-p', path], rcs=[0, 1])
+ if len(err):
+ # find a path if the input is not a mounting point
+ (mnt_list, err) = subp(['mount', '-p'])
+ path_found = get_path_dev_freebsd(path, mnt_list)
+ if (path_found is None):
+ return None
+ result = path_found
+ ret = result.split()
+ label_part = find_freebsd_part(ret[0])
+ return "/dev/" + label_part, ret[2], ret[1]
+
+
 def parse_mount(path):
     (mountoutput, _err) = subp("mount")
     mount_locs = mountoutput.splitlines()
     for line in mount_locs:
         m = re.search(r'^(/dev/[\S]+) on (/.*) \((.+), .+, (.+)\)$', line)
+ if not m:
+ continue
+ # check whether the dev refers to a label on FreeBSD
+ # for example, if dev is '/dev/label/rootfs', we should
+ # continue finding the real device like '/dev/da0'.
+ devm = re.search('^(/dev/.+)p([0-9])$', m.group(1))
+ if (not devm and is_FreeBSD()):
+ return get_mount_info_freebsd(path)
         devpth = m.group(1)

-----Original Message-----
From: Hongjiang Zhang
Sent: Tuesday, April 25, 2017 11:38 AM
To: 'Scott Moser' <email address hidden>
Cc: '<email address hidden>' <email address hidden>; Kylie Liang <email address hidden>; Josh Poulson <email address hidden>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

Hello Scott,

I'm not sure whether you are still confused about why we need Azure specific code, or is there anything I can do to help make any progress on this merge?

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang
Sent: Friday, April 14, 2017 10:58 AM
To: Scott Moser <email address hidden>
Cc: <email address hidden>; Kylie Liang <email address hidden>; Josh Poulson <email address hidden>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

The reason is FreeBSD on Azure have more than 1 disks attached, and the rootfs located in one disk, for example, /dev/da0, but I cannot directly write something below:
/dev/da0 / ufs rw 1 1
Because the rootfs may be switched to /dev/da1. The solution is label the disk. For FreeBSD on Azure, I labeled /dev/da0 with "rootfs", so for get_mount_info, I have to find "rootfs" first, and then find the corresponding /dev/da0.

But on Linux, there is a /proc/$$/mountinfo to help find the device. For Azure, there is also /dev/disk/cloud/azure_root on Linux.
Both make the get_mount_info easy to implement.

-----Original Message-----
From: Scott Moser [mailto:<email address hidden>] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <email address hidden>
Cc: <email address hidden>; Kylie Liang <email address hidden>; Josh Poulson <email address hidden>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

On Fri, 14 Apr 2017, Hongjiang Zhang wrote:

> Hi Scott,
>
> I have manually merged your modifications to my branch and fixed a unit test issue.
> Do you think it is ok for merging?

The only thing I had left was that I'm still confused as what why we need azure specific code in that code path.

I have to think about it some more and probably go play with it a little on freebsd.

>
> Thanks
> Hongjiang Zhang
>
>
> -----Original Message-----
> From: <email address hidden> [mailto:<email address hidden>] On Behalf
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: <email address hidden>
> Subject: Re: [Merge] ~redriver/cloud-init:frbsd-azure-branch into
> cloud-init:master
>
> Other than that and I think it looks really good.
> thank you for adding the unit tests.
>
> --
> https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.la
> unchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>