[wic][PATCH 0/4] Code cleanup and refactoring

[wic][PATCH 0/4] Code cleanup and refactoring

[Ed Bartosh]

Hi,

This patchset includes cleanup and refactoring patches made
during implementation of partition UUID support. Without this
work I'd end up in duplicating a lot of code, so I decided to
clean it up a bit.

Please, review.

Ed Bartosh (4):
  wic: removed exec_cmd_quiet and exec_native_cmd_quiet
  wic: move checks to exec_native_cmd
  wic: replaced __run_parted with exec_native_cmd
  wic: pylinted partitionedfs.py

 .../lib/wic/kickstart/custom_commands/partition.py |  21 +----
 scripts/lib/wic/utils/oe/misc.py                   |  27 ++----
 scripts/lib/wic/utils/partitionedfs.py             | 105 +++++++++------------
 3 files changed, 54 insertions(+), 99 deletions(-)

--
EPlease, review.d

[wic][PATCH 1/4] wic: removed exec_cmd_quiet and exec_native_cmd_quiet

[Ed Bartosh]

These functions are not used anywhere.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>

diff --git a/scripts/lib/wic/utils/oe/misc.py b/scripts/lib/wic/utils/oe/misc.py
index 2f5299d..9eaf039 100644
--- a/scripts/lib/wic/utils/oe/misc.py
+++ b/scripts/lib/wic/utils/oe/misc.py
@@ -63,15 +63,6 @@ def exec_cmd(cmd_and_args, as_shell=False, catch=3):
     return out
 
 
-def exec_cmd_quiet(cmd_and_args, as_shell=False):
-    """
-    Execute command, catching nothing in the output
-
-    Exits if rc non-zero
-    """
-    return exec_cmd(cmd_and_args, as_shell, 0)
-
-
 def exec_native_cmd(cmd_and_args, native_sysroot, catch=3):
     """
     Execute native command, catching stderr, stdout
@@ -98,18 +89,6 @@ def exec_native_cmd(cmd_and_args, native_sysroot, catch=3):
 
     return (rc, out)
 
-
-def exec_native_cmd_quiet(cmd_and_args, native_sysroot):
-    """
-    Execute native command, catching nothing in the output
-
-    Need to execute as_shell if the command uses wildcards
-
-    Always need to execute native commands as_shell
-    """
-    return exec_native_cmd(cmd_and_args, native_sysroot, 0)
-
-
 # kickstart doesn't support variable substution in commands, so this
 # is our current simplistic scheme for supporting that
 
-- 
2.1.4

[wic][PATCH 2/4] wic: move checks to exec_native_cmd

[Ed Bartosh]

Checked for return code and output of native commands
inside exec_native_cmd.

Removed similar code from a lot of places where
exec_native_cmd is called.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>

diff --git a/scripts/lib/wic/kickstart/custom_commands/partition.py b/scripts/lib/wic/kickstart/custom_commands/partition.py
index d9f77d9..5d033bb 100644
--- a/scripts/lib/wic/kickstart/custom_commands/partition.py
+++ b/scripts/lib/wic/kickstart/custom_commands/partition.py
@@ -257,10 +257,7 @@ class Wic_PartData(Mic_PartData):
 
         mkfs_cmd = "mkfs.%s -F %s %s %s -d %s" % \
             (self.fstype, extra_imagecmd, rootfs, label_str, image_rootfs)
-        (rc, out) = exec_native_cmd(pseudo + mkfs_cmd, native_sysroot)
-        if rc:
-            print "rootfs_dir: %s" % rootfs_dir
-            msger.error("ERROR: mkfs.%s returned '%s' instead of 0 (which you probably don't want to ignore, use --debug for details) when creating filesystem from rootfs directory: %s" % (self.fstype, rc, rootfs_dir))
+        exec_native_cmd(pseudo + mkfs_cmd, native_sysroot)
 
         # get the rootfs size in the right units for kickstart (kB)
         du_cmd = "du -Lbks %s" % rootfs
@@ -307,9 +304,7 @@ class Wic_PartData(Mic_PartData):
 
         mkfs_cmd = "mkfs.%s -b %d -r %s %s %s" % \
             (self.fstype, rootfs_size * 1024, image_rootfs, label_str, rootfs)
-        (rc, out) = exec_native_cmd(pseudo + mkfs_cmd, native_sysroot)
-        if rc:
-            msger.error("ERROR: mkfs.%s returned '%s' instead of 0 (which you probably don't want to ignore, use --debug for details) when creating filesystem from rootfs directory: %s" % (self.fstype, rc, rootfs_dir))
+        exec_native_cmd(pseudo + mkfs_cmd, native_sysroot)
 
         # get the rootfs size in the right units for kickstart (kB)
         du_cmd = "du -Lbks %s" % rootfs
@@ -357,9 +352,7 @@ class Wic_PartData(Mic_PartData):
         exec_native_cmd(dosfs_cmd, native_sysroot)
 
         mcopy_cmd = "mcopy -i %s -s %s/* ::/" % (rootfs, image_rootfs)
-        rc, out = exec_native_cmd(mcopy_cmd, native_sysroot)
-        if rc:
-            msger.error("ERROR: mcopy returned '%s' instead of 0 (which you probably don't want to ignore, use --debug for details)" % rc)
+        exec_native_cmd(mcopy_cmd, native_sysroot)
 
         chmod_cmd = "chmod 644 %s" % rootfs
         exec_cmd(chmod_cmd)
@@ -432,9 +425,7 @@ class Wic_PartData(Mic_PartData):
 
         mkfs_cmd = "mkfs.%s -F %s %s %s" % \
             (self.fstype, extra_imagecmd, label_str, fs)
-        (rc, out) = exec_native_cmd(mkfs_cmd, native_sysroot)
-        if rc:
-            msger.error("ERROR: mkfs.%s returned '%s' instead of 0 (which you probably don't want to ignore, use --debug for details)" % (self.fstype, rc))
+        exec_native_cmd(mkfs_cmd, native_sysroot)
 
         self.source_file = fs
 
@@ -458,9 +449,7 @@ class Wic_PartData(Mic_PartData):
 
         mkfs_cmd = "mkfs.%s -b %d %s %s" % \
             (self.fstype, self.size * 1024, label_str, fs)
-        (rc, out) = exec_native_cmd(mkfs_cmd, native_sysroot)
-        if rc:
-            msger.error("ERROR: mkfs.%s returned '%s' instead of 0 (which you probably don't want to ignore, use --debug for details)" % (self.fstype, rc))
+        exec_native_cmd(mkfs_cmd, native_sysroot)
 
         self.source_file = fs
 
diff --git a/scripts/lib/wic/utils/oe/misc.py b/scripts/lib/wic/utils/oe/misc.py
index 9eaf039..f08ff15 100644
--- a/scripts/lib/wic/utils/oe/misc.py
+++ b/scripts/lib/wic/utils/oe/misc.py
@@ -86,6 +86,12 @@ def exec_native_cmd(cmd_and_args, native_sysroot, catch=3):
         msger.error("A native program %s required to build the image "
                     "was not found (see details above). Please make sure "
                     "it's installed and try again." % args[0])
+    if out:
+        msger.debug('"%s" output: %s' % (args[0], out))
+
+    if rc != 0:
+        msger.error("exec_cmd: '%s' returned '%s' instead of 0" % \
+                    (cmd_and_args, rc))
 
     return (rc, out)
 
diff --git a/scripts/lib/wic/utils/partitionedfs.py b/scripts/lib/wic/utils/partitionedfs.py
index eacf267..8fd44a6 100644
--- a/scripts/lib/wic/utils/partitionedfs.py
+++ b/scripts/lib/wic/utils/partitionedfs.py
@@ -227,17 +227,7 @@ class Image:
         args = ' '.join(args)
         msger.debug(args)
 
-        rc, out = exec_native_cmd(args, self.native_sysroot)
-
-        if out:
-            msger.debug('"parted" output: %s' % out)
-
-        if rc != 0:
-            # We don't throw exception when return code is not 0, because
-            # parted always fails to reload part table with loop devices. This
-            # prevents us from distinguishing real errors based on return
-            # code.
-            msger.error("WARNING: parted returned '%s' instead of 0 (use --debug for details)" % rc)
+        exec_native_cmd(args, self.native_sysroot)
 
     def __create_partition(self, device, parttype, fstype, start, size):
         """ Create a partition on an image described by the 'device' object. """
-- 
2.1.4

[wic][PATCH 3/4] wic: replaced __run_parted with exec_native_cmd

[Ed Bartosh]

There is no need for yet another wrapper around exec_native_cmd.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>

diff --git a/scripts/lib/wic/utils/partitionedfs.py b/scripts/lib/wic/utils/partitionedfs.py
index 8fd44a6..dcb63e5 100644
--- a/scripts/lib/wic/utils/partitionedfs.py
+++ b/scripts/lib/wic/utils/partitionedfs.py
@@ -220,15 +220,6 @@ class Image:
 
             d['min_size'] *= self.sector_size
 
-    def __run_parted(self, args):
-        """ Run parted with arguments specified in the 'args' list. """
-
-        args.insert(0, "parted")
-        args = ' '.join(args)
-        msger.debug(args)
-
-        exec_native_cmd(args, self.native_sysroot)
-
     def __create_partition(self, device, parttype, fstype, start, size):
         """ Create a partition on an image described by the 'device' object. """
 
@@ -237,12 +228,12 @@ class Image:
         msger.debug("Added '%s' partition, sectors %d-%d, size %d sectors" %
                     (parttype, start, end, size))
 
-        args = ["-s", device, "unit", "s", "mkpart", parttype]
+        cmd = "parted -s %s unit s mkpart %s" % (device, parttype)
         if fstype:
-            args.extend([fstype])
-        args.extend(["%d" % start, "%d" % end])
+            cmd += " %s" % fstype
+        cmd += " %d %d" % (start, end)
 
-        return self.__run_parted(args)
+        return exec_native_cmd(cmd, self.native_sysroot)
 
     def __format_disks(self):
         self.layout_partitions()
@@ -251,8 +242,9 @@ class Image:
             d = self.disks[dev]
             msger.debug("Initializing partition table for %s" % \
                         (d['disk'].device))
-            self.__run_parted(["-s", d['disk'].device, "mklabel",
-                               d['ptable_format']])
+            exec_native_cmd("parted -s %s mklabel %s" % \
+                            (d['disk'].device, d['ptable_format']),
+                            self.native_sysroot)
 
         msger.debug("Creating partitions")
 
@@ -305,8 +297,9 @@ class Image:
                 flag_name = "legacy_boot" if d['ptable_format'] == 'gpt' else "boot"
                 msger.debug("Set '%s' flag for partition '%s' on disk '%s'" % \
                             (flag_name, p['num'], d['disk'].device))
-                self.__run_parted(["-s", d['disk'].device, "set",
-                                   "%d" % p['num'], flag_name, "on"])
+                exec_native_cmd("parted -s %s set %d %s on" % \
+                                (d['disk'].device, p['num'], flag_name),
+                                self.native_sysroot)
 
             # Parted defaults to enabling the lba flag for fat16 partitions,
             # which causes compatibility issues with some firmware (and really
@@ -315,8 +308,9 @@ class Image:
                 if d['ptable_format'] == 'msdos':
                     msger.debug("Disable 'lba' flag for partition '%s' on disk '%s'" % \
                                 (p['num'], d['disk'].device))
-                    self.__run_parted(["-s", d['disk'].device, "set",
-                                       "%d" % p['num'], "lba", "off"])
+                    exec_native_cmd("parted -s %s set %d lba off" % \
+                                    (d['disk'].device, p['num']),
+                                    self.native_sysroot)
 
     def cleanup(self):
         if self.disks:
-- 
2.1.4

Re: [wic][PATCH 3/4] wic: replaced __run_parted with exec_native_cmd

[Ross Burton]

On 05/06/2015 07:13, Ed Bartosh wrote:
 > There is no need for yet another wrapper around exec_native_cmd.

This doesn't appear to apply to master, can you rebase?

(or does it depend on your other series?)

Ross

Re: [wic][PATCH 3/4] wic: replaced __run_parted with exec_native_cmd

[Ed Bartosh]

On Fri, Jun 05, 2015 at 05:33:12PM +0100, Ross Burton wrote:
> On 05/06/2015 07:13, Ed Bartosh wrote:
> > There is no need for yet another wrapper around exec_native_cmd.
> 
> This doesn't appear to apply to master, can you rebase?
> 
> (or does it depend on your other series?)

It depends on this patchset: http://lists.openembedded.org/pipermail/openembedded-core/2015-June/105561.html

--
Regards,
Ed

[wic][PATCH 4/4] wic: pylinted partitionedfs.py

[Ed Bartosh]

Fixed some pylint findings in partitionedfs.py

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>

diff --git a/scripts/lib/wic/utils/partitionedfs.py b/scripts/lib/wic/utils/partitionedfs.py
index dcb63e5..902548f 100644
--- a/scripts/lib/wic/utils/partitionedfs.py
+++ b/scripts/lib/wic/utils/partitionedfs.py
@@ -31,7 +31,7 @@ GPT_OVERHEAD = 34
 # Size of a sector in bytes
 SECTOR_SIZE = 512
 
-class Image:
+class Image(object):
     """
     Generic base object for an image.
 
@@ -58,14 +58,14 @@ class Image:
         assert not self._partitions_layed_out
 
         self.disks[disk_name] = \
-                { 'disk': None,     # Disk object
-                  'numpart': 0,     # Number of allocate partitions
-                  'realpart': 0,    # Number of partitions in the partition table
-                  'partitions': [], # Indexes to self.partitions
-                  'offset': 0,      # Offset of next partition (in sectors)
-                  # Minimum required disk size to fit all partitions (in bytes)
-                  'min_size': 0,
-                  'ptable_format': "msdos" } # Partition table format
+                {'disk': None,     # Disk object
+                 'numpart': 0,     # Number of allocate partitions
+                 'realpart': 0,    # Number of partitions in the partition table
+                 'partitions': [], # Indexes to self.partitions
+                 'offset': 0,      # Offset of next partition (in sectors)
+                 # Minimum required disk size to fit all partitions (in bytes)
+                 'min_size': 0,
+                 'ptable_format': "msdos"} # Partition table format
 
     def add_disk(self, disk_name, disk_obj):
         """ Add a disk object which have to be partitioned. More than one disk
@@ -97,20 +97,20 @@ class Image:
 
         # We still need partition for "/" or non-subvolume
         if mountpoint == "/" or not fsopts:
-            part = { 'ks_pnum' : ks_pnum, # Partition number in the KS file
-                     'size': size, # In sectors
-                     'mountpoint': mountpoint, # Mount relative to chroot
-                     'source_file': source_file, # partition contents
-                     'fstype': fstype, # Filesystem type
-                     'fsopts': fsopts, # Filesystem mount options
-                     'label': label, # Partition label
-                     'disk_name': disk_name, # physical disk name holding partition
-                     'device': None, # kpartx device node for partition
-                     'num': None, # Partition number
-                     'boot': boot, # Bootable flag
-                     'align': align, # Partition alignment
-                     'no_table' : no_table, # Partition does not appear in partition table
-                     'part_type' : part_type } # Partition type
+            part = {'ks_pnum': ks_pnum, # Partition number in the KS file
+                    'size': size, # In sectors
+                    'mountpoint': mountpoint, # Mount relative to chroot
+                    'source_file': source_file, # partition contents
+                    'fstype': fstype, # Filesystem type
+                    'fsopts': fsopts, # Filesystem mount options
+                    'label': label, # Partition label
+                    'disk_name': disk_name, # physical disk name holding partition
+                    'device': None, # kpartx device node for partition
+                    'num': None, # Partition number
+                    'boot': boot, # Bootable flag
+                    'align': align, # Partition alignment
+                    'no_table' : no_table, # Partition does not appear in partition table
+                    'part_type' : part_type} # Partition type
 
             self.__add_partition(part)
 
@@ -213,7 +213,7 @@ class Image:
 
         # Once all the partitions have been layed out, we can calculate the
         # minumim disk sizes.
-        for disk_name, d in self.disks.items():
+        for d in self.disks.values():
             d['min_size'] = d['offset']
             if d['ptable_format'] == "gpt":
                 d['min_size'] += GPT_OVERHEAD
@@ -314,14 +314,14 @@ class Image:
 
     def cleanup(self):
         if self.disks:
-            for dev in self.disks.keys():
+            for dev in self.disks:
                 d = self.disks[dev]
                 try:
                     d['disk'].cleanup()
                 except:
                     pass
 
-    def __write_partition(self, num, source_file, start, size):
+    def __write_partition(self, num, source_file, start, size, image_file):
         """
         Install source_file contents into a partition.
         """
@@ -330,23 +330,20 @@ class Image:
 
         # Start is included in the size so need to substract one from the end.
         end = start + size - 1
-        msger.debug("Installed %s in partition %d, sectors %d-%d, size %d sectors" % (source_file, num, start, end, size))
+        msger.debug("Installed %s in partition %d, sectors %d-%d, "
+                    "size %d sectors" % (source_file, num, start, end, size))
 
         dd_cmd = "dd if=%s of=%s bs=%d seek=%d count=%d conv=notrunc" % \
-            (source_file, self.image_file, self.sector_size, start, size)
+            (source_file, image_file, self.sector_size, start, size)
         exec_cmd(dd_cmd)
 
 
     def assemble(self, image_file):
         msger.debug("Installing partitions")
 
-        self.image_file = image_file
-
         for p in self.partitions:
-            d = self.disks[p['disk_name']]
-
             self.__write_partition(p['num'], p['source_file'],
-                                   p['start'], p['size'])
+                                   p['start'], p['size'], image_file)
 
     def create(self):
         for dev in self.disks.keys():
-- 
2.1.4