[v2,16/49] binman: Detect when valid images are not produced

Message ID 20200613205717.v2.16.Ia87bd305fe68d527b8148b31d2b16ef40af7d84c@changeid
State Accepted
Commit 13262c93626502873786067fcbe2e2ab5894b90f
Headers show
Series
  • rockchip: x86: Support building ROM files automatically with binman
Related show

Commit Message

Simon Glass June 14, 2020, 2:56 a.m.
When external blobs are missing, show a message indicating that the images
are not functional.

Signed-off-by: Simon Glass <sjg at chromium.org>
---

(no changes since v1)

 tools/binman/control.py                       | 16 +++++++++++--
 tools/binman/entry.py                         | 21 +++++++++++++++++
 tools/binman/etype/blob_ext.py                |  1 +
 tools/binman/etype/section.py                 | 16 ++++++++++++-
 tools/binman/ftest.py                         | 14 ++++++++++-
 .../binman/test/159_blob_ext_missing_sect.dts | 23 +++++++++++++++++++
 tools/patman/tout.py                          |  1 +
 7 files changed, 88 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/159_blob_ext_missing_sect.dts

Comments

Bin Meng June 29, 2020, 6:54 a.m. | #1
Hi Simon,

On Sun, Jun 14, 2020 at 10:57 AM Simon Glass <sjg at chromium.org> wrote:
>
> When external blobs are missing, show a message indicating that the images
> are not functional.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> (no changes since v1)
>
>  tools/binman/control.py                       | 16 +++++++++++--
>  tools/binman/entry.py                         | 21 +++++++++++++++++
>  tools/binman/etype/blob_ext.py                |  1 +
>  tools/binman/etype/section.py                 | 16 ++++++++++++-
>  tools/binman/ftest.py                         | 14 ++++++++++-
>  .../binman/test/159_blob_ext_missing_sect.dts | 23 +++++++++++++++++++
>  tools/patman/tout.py                          |  1 +
>  7 files changed, 88 insertions(+), 4 deletions(-)
>  create mode 100644 tools/binman/test/159_blob_ext_missing_sect.dts
>
> diff --git a/tools/binman/control.py b/tools/binman/control.py
> index 8c6eae83f1..343b0a0c35 100644
> --- a/tools/binman/control.py
> +++ b/tools/binman/control.py
> @@ -403,6 +403,9 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
>          allow_resize: True to allow entries to change size (this does a re-pack
>              of the entries), False to raise an exception
>          allow_missing: Allow blob_ext objects to be missing
> +
> +    Returns:
> +        True if one or more external blobs are missing, False if all are present
>      """
>      if get_contents:
>          image.SetAllowMissing(allow_missing)
> @@ -450,6 +453,12 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
>      image.BuildImage()
>      if write_map:
>          image.WriteMap()
> +    missing_list = []
> +    image.CheckMissing(missing_list)
> +    if missing_list:
> +        tout.Warning("Image '%s' is missing external blobs and is non-functional: %s" %
> +                     (image.name, ' '.join([e.name for e in missing_list])))
> +    return bool(missing_list)
>
>
>  def Binman(args):
> @@ -524,14 +533,17 @@ def Binman(args):
>
>              images = PrepareImagesAndDtbs(dtb_fname, args.image,
>                                            args.update_fdt)
> +            missing = False
>              for image in images.values():
> -                ProcessImage(image, args.update_fdt, args.map,
> -                             allow_missing=args.allow_missing)
> +                missing |= ProcessImage(image, args.update_fdt, args.map,
> +                                        allow_missing=args.allow_missing)
>
>              # Write the updated FDTs to our output files
>              for dtb_item in state.GetAllFdts():
>                  tools.WriteFile(dtb_item._fname, dtb_item.GetContents())
>
> +            if missing:
> +                tout.Warning("Some images are invalid")
>          finally:
>              tools.FinaliseOutputDir()
>      finally:
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 90ffd27617..4a42e0bf34 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -84,6 +84,7 @@ class Entry(object):
>          self.image_pos = None
>          self._expand_size = False
>          self.compress = 'none'
> +        self.missing = False
>
>      @staticmethod
>      def Lookup(node_path, etype):
> @@ -794,3 +795,23 @@ features to produce new behaviours.
>              elif self == entries[-1]:
>                  return 'end'
>          return 'middle'
> +
> +    def SetAllowMissing(self, allow_missing):
> +        """Set whether a section allows missing external blobs
> +
> +        Args:
> +            allow_missing: True if allowed, False if not allowed
> +        """
> +        # This is meaningless for

for what?

> +        self._allow_missing = allow_missing
> +

Should the above changes be in patch "[v2,14/49] binman: Allow
external binaries to be missing" ?

> +    def CheckMissing(self, missing_list):
> +        """Check if any entries in this section have missing external blobs
> +
> +        If there are missing blobs, the entries are added to the list
> +
> +        Args:
> +            missing_list: List of Entry objects to be added to
> +        """
> +        if self.missing:
> +            missing_list.append(self)
> diff --git a/tools/binman/etype/blob_ext.py b/tools/binman/etype/blob_ext.py
> index 51779c88c9..8d641001a9 100644
> --- a/tools/binman/etype/blob_ext.py
> +++ b/tools/binman/etype/blob_ext.py
> @@ -34,5 +34,6 @@ class Entry_blob_ext(Entry_blob):
>          # Allow the file to be missing
>          if not self._pathname:
>              self.SetContents(b'')
> +            self.missing = True
>              return True
>          return super().ObtainContents()
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index 44b13b2575..3b42a5890f 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -50,6 +50,7 @@ class Entry_section(Entry):
>          self._skip_at_start = None
>          self._end_4gb = False
>          self._allow_missing = False
> +        self.missing = False
>
>      def ReadNode(self):
>          """Read properties from the image node"""
> @@ -543,7 +544,9 @@ class Entry_section(Entry):
>          Args:
>              allow_missing: True if allowed, False if not allowed
>          """
> -        self._allow_missing = allow_missing
> +        super().SetAllowMissing(allow_missing)
> +        for entry in self._entries.values():
> +            entry.SetAllowMissing(allow_missing)
>
>      def GetAllowMissing(self):
>          """Get whether a section allows missing external blobs
> @@ -552,3 +555,14 @@ class Entry_section(Entry):
>              True if allowed, False if not allowed
>          """
>          return self._allow_missing
> +
> +    def CheckMissing(self, missing_list):
> +        """Check if any entries in this section have missing external blobs
> +
> +        If there are missing blobs, the entries are added to the list
> +
> +        Args:
> +            missing_list: List of Entry objects to be added to
> +        """
> +        for entry in self._entries.values():
> +            entry.CheckMissing(missing_list)
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 928d3608a3..cc551c9f17 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -3380,7 +3380,19 @@ class TestFunctional(unittest.TestCase):
>
>      def testExtblobMissingOk(self):
>          """Test an image with an missing external blob that is allowed"""
> -        self._DoTestFile('158_blob_ext_missing.dts', allow_missing=True)
> +        with test_util.capture_sys_output() as (stdout, stderr):
> +            self._DoTestFile('158_blob_ext_missing.dts', allow_missing=True)
> +        err = stderr.getvalue()
> +        self.assertRegex(err, "Image 'main-section'.*missing.*: blob-ext")
> +
> +    def testExtblobMissingOkSect(self):
> +        """Test an image with an missing external blob that is allowed"""
> +        with test_util.capture_sys_output() as (stdout, stderr):
> +            self._DoTestFile('159_blob_ext_missing_sect.dts',
> +                             allow_missing=True)
> +        err = stderr.getvalue()
> +        self.assertRegex(err, "Image 'main-section'.*missing.*: "
> +                         "blob-ext blob-ext2")
>
>
>  if __name__ == "__main__":
> diff --git a/tools/binman/test/159_blob_ext_missing_sect.dts b/tools/binman/test/159_blob_ext_missing_sect.dts
> new file mode 100644
> index 0000000000..5f14c54138
> --- /dev/null
> +++ b/tools/binman/test/159_blob_ext_missing_sect.dts
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               size = <0x80>;
> +
> +               section {
> +                       blob-ext {
> +                               filename = "missing-file";
> +                       };
> +               };
> +
> +               blob-ext2 {
> +                       type = "blob-ext";
> +                       filename = "missing-file2";
> +               };
> +       };
> +};
> diff --git a/tools/patman/tout.py b/tools/patman/tout.py
> index 91a53f4073..33305263d8 100644
> --- a/tools/patman/tout.py
> +++ b/tools/patman/tout.py
> @@ -171,6 +171,7 @@ def Init(_verbose=WARNING, stdout=sys.stdout):
>
>      # TODO(sjg): Move this into Chromite libraries when we have them
>      stdout_is_tty = hasattr(sys.stdout, 'isatty') and sys.stdout.isatty()
> +    stderr_is_tty = hasattr(sys.stderr, 'isatty') and sys.stderr.isatty()
>
>  def Uninit():
>      ClearProgress()
> --

Regards,
Bin

Patch

diff --git a/tools/binman/control.py b/tools/binman/control.py
index 8c6eae83f1..343b0a0c35 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -403,6 +403,9 @@  def ProcessImage(image, update_fdt, write_map, get_contents=True,
         allow_resize: True to allow entries to change size (this does a re-pack
             of the entries), False to raise an exception
         allow_missing: Allow blob_ext objects to be missing
+
+    Returns:
+        True if one or more external blobs are missing, False if all are present
     """
     if get_contents:
         image.SetAllowMissing(allow_missing)
@@ -450,6 +453,12 @@  def ProcessImage(image, update_fdt, write_map, get_contents=True,
     image.BuildImage()
     if write_map:
         image.WriteMap()
+    missing_list = []
+    image.CheckMissing(missing_list)
+    if missing_list:
+        tout.Warning("Image '%s' is missing external blobs and is non-functional: %s" %
+                     (image.name, ' '.join([e.name for e in missing_list])))
+    return bool(missing_list)
 
 
 def Binman(args):
@@ -524,14 +533,17 @@  def Binman(args):
 
             images = PrepareImagesAndDtbs(dtb_fname, args.image,
                                           args.update_fdt)
+            missing = False
             for image in images.values():
-                ProcessImage(image, args.update_fdt, args.map,
-                             allow_missing=args.allow_missing)
+                missing |= ProcessImage(image, args.update_fdt, args.map,
+                                        allow_missing=args.allow_missing)
 
             # Write the updated FDTs to our output files
             for dtb_item in state.GetAllFdts():
                 tools.WriteFile(dtb_item._fname, dtb_item.GetContents())
 
+            if missing:
+                tout.Warning("Some images are invalid")
         finally:
             tools.FinaliseOutputDir()
     finally:
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 90ffd27617..4a42e0bf34 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -84,6 +84,7 @@  class Entry(object):
         self.image_pos = None
         self._expand_size = False
         self.compress = 'none'
+        self.missing = False
 
     @staticmethod
     def Lookup(node_path, etype):
@@ -794,3 +795,23 @@  features to produce new behaviours.
             elif self == entries[-1]:
                 return 'end'
         return 'middle'
+
+    def SetAllowMissing(self, allow_missing):
+        """Set whether a section allows missing external blobs
+
+        Args:
+            allow_missing: True if allowed, False if not allowed
+        """
+        # This is meaningless for
+        self._allow_missing = allow_missing
+
+    def CheckMissing(self, missing_list):
+        """Check if any entries in this section have missing external blobs
+
+        If there are missing blobs, the entries are added to the list
+
+        Args:
+            missing_list: List of Entry objects to be added to
+        """
+        if self.missing:
+            missing_list.append(self)
diff --git a/tools/binman/etype/blob_ext.py b/tools/binman/etype/blob_ext.py
index 51779c88c9..8d641001a9 100644
--- a/tools/binman/etype/blob_ext.py
+++ b/tools/binman/etype/blob_ext.py
@@ -34,5 +34,6 @@  class Entry_blob_ext(Entry_blob):
         # Allow the file to be missing
         if not self._pathname:
             self.SetContents(b'')
+            self.missing = True
             return True
         return super().ObtainContents()
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 44b13b2575..3b42a5890f 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -50,6 +50,7 @@  class Entry_section(Entry):
         self._skip_at_start = None
         self._end_4gb = False
         self._allow_missing = False
+        self.missing = False
 
     def ReadNode(self):
         """Read properties from the image node"""
@@ -543,7 +544,9 @@  class Entry_section(Entry):
         Args:
             allow_missing: True if allowed, False if not allowed
         """
-        self._allow_missing = allow_missing
+        super().SetAllowMissing(allow_missing)
+        for entry in self._entries.values():
+            entry.SetAllowMissing(allow_missing)
 
     def GetAllowMissing(self):
         """Get whether a section allows missing external blobs
@@ -552,3 +555,14 @@  class Entry_section(Entry):
             True if allowed, False if not allowed
         """
         return self._allow_missing
+
+    def CheckMissing(self, missing_list):
+        """Check if any entries in this section have missing external blobs
+
+        If there are missing blobs, the entries are added to the list
+
+        Args:
+            missing_list: List of Entry objects to be added to
+        """
+        for entry in self._entries.values():
+            entry.CheckMissing(missing_list)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 928d3608a3..cc551c9f17 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3380,7 +3380,19 @@  class TestFunctional(unittest.TestCase):
 
     def testExtblobMissingOk(self):
         """Test an image with an missing external blob that is allowed"""
-        self._DoTestFile('158_blob_ext_missing.dts', allow_missing=True)
+        with test_util.capture_sys_output() as (stdout, stderr):
+            self._DoTestFile('158_blob_ext_missing.dts', allow_missing=True)
+        err = stderr.getvalue()
+        self.assertRegex(err, "Image 'main-section'.*missing.*: blob-ext")
+
+    def testExtblobMissingOkSect(self):
+        """Test an image with an missing external blob that is allowed"""
+        with test_util.capture_sys_output() as (stdout, stderr):
+            self._DoTestFile('159_blob_ext_missing_sect.dts',
+                             allow_missing=True)
+        err = stderr.getvalue()
+        self.assertRegex(err, "Image 'main-section'.*missing.*: "
+                         "blob-ext blob-ext2")
 
 
 if __name__ == "__main__":
diff --git a/tools/binman/test/159_blob_ext_missing_sect.dts b/tools/binman/test/159_blob_ext_missing_sect.dts
new file mode 100644
index 0000000000..5f14c54138
--- /dev/null
+++ b/tools/binman/test/159_blob_ext_missing_sect.dts
@@ -0,0 +1,23 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		size = <0x80>;
+
+		section {
+			blob-ext {
+				filename = "missing-file";
+			};
+		};
+
+		blob-ext2 {
+			type = "blob-ext";
+			filename = "missing-file2";
+		};
+	};
+};
diff --git a/tools/patman/tout.py b/tools/patman/tout.py
index 91a53f4073..33305263d8 100644
--- a/tools/patman/tout.py
+++ b/tools/patman/tout.py
@@ -171,6 +171,7 @@  def Init(_verbose=WARNING, stdout=sys.stdout):
 
     # TODO(sjg): Move this into Chromite libraries when we have them
     stdout_is_tty = hasattr(sys.stdout, 'isatty') and sys.stdout.isatty()
+    stderr_is_tty = hasattr(sys.stderr, 'isatty') and sys.stderr.isatty()
 
 def Uninit():
     ClearProgress()