Message-ID: <20250227161343.5249e9b8@foz.lan>
Date: Thu, 27 Feb 2025 16:13:43 +0100
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Igor Mammedov <imammedo@...hat.com>
Cc: "Michael S . Tsirkin" <mst@...hat.com>, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, Shiju Jose <shiju.jose@...wei.com>,
 qemu-arm@...gnu.org, qemu-devel@...gnu.org, Philippe Mathieu-Daudé <philmd@...aro.org>, Ani Sinha
 <anisinha@...hat.com>, Cleber Rosa <crosa@...hat.com>, Dongjiu Geng
 <gengdongjiu1@...il.com>, Eduardo Habkost <eduardo@...kost.net>, Eric Blake
 <eblake@...hat.com>, John Snow <jsnow@...hat.com>, Marcel Apfelbaum
 <marcel.apfelbaum@...il.com>, Markus Armbruster <armbru@...hat.com>,
 Michael Roth <michael.roth@....com>, Paolo Bonzini <pbonzini@...hat.com>,
 Peter Maydell <peter.maydell@...aro.org>, Shannon Zhao
 <shannon.zhaosl@...il.com>, Yanan Wang <wangyanan55@...wei.com>, Zhao Liu
 <zhao1.liu@...el.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 00/21]Change ghes to use HEST-based offsets and add
 support for error inject

Em Thu, 27 Feb 2025 14:30:28 +0100
Igor Mammedov <imammedo@...hat.com> escreveu:

> On Thu, 27 Feb 2025 12:03:30 +0100
> Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
> 
> > Now that the ghes preparation patches were merged, let's add support
> > for error injection.
> > 
> > On this version, HEST table got added to ACPI tables testing for aarch64 virt.
> > 
> > There are also some patch reorder to help reviewers to check the changes.
> > 
> > The code itself is almost identical to v4, with just a few minor nits addressed.  
> 
> series still has checkpatch errors 'line over 80' which are not false positive,
> it needs to be fixed

The long line warnings are at the patch adding the Python script. IMO,
all but one are false positives:

1. Long lines at patch description because of the tool output example added
   inside the commit description:

	ERROR: line over 90 characters
	#148: FILE: scripts/arm_processor_error.py:83:
	+[Hardware Error]:     bus error, operation type: Generic read (type of instruction or data request cannot be determined)

	ERROR: line over 90 characters
	#153: FILE: scripts/arm_processor_error.py:88:
	+[Hardware Error]:     Program execution can be restarted reliably at the PC associated with the error.

	WARNING: line over 80 characters
	#170: FILE: scripts/arm_processor_error.py:105:
	+[Hardware Error]:    00000000: 13 7b 04 05 01                                   .{...

	WARNING: line over 80 characters
	#174: FILE: scripts/arm_processor_error.py:109:
	+[Firmware Warn]: GHES: Unhandled processor error type 0x10: micro-architectural error

	ERROR: line over 90 characters
	#175: FILE: scripts/arm_processor_error.py:110:
	+[Firmware Warn]: GHES: Unhandled processor error type 0x14: TLB error|micro-architectural error

   IMO, breaking command output at the description is a bad practice.

2. Big strings at help message:

	WARNING: line over 80 characters
	#261: FILE: scripts/arm_processor_error.py:196:
	+                           help="Power State Coordination Interface - PSCI state")

	ERROR: line over 90 characters
	#276: FILE: scripts/arm_processor_error.py:211:
	+                        help="Number of errors: 0: Single error, 1: Multiple errors, 2-65535: Error count if known")

	WARNING: line over 80 characters
	#278: FILE: scripts/arm_processor_error.py:213:
	+                        help="Error information (UEFI 2.10 tables N.18 to N.20)")

	ERROR: line over 90 characters
	#287: FILE: scripts/arm_processor_error.py:222:
	+                        help="Type of the context (0=ARM32 GPR, 5=ARM64 EL1, other values supported)")


	WARNING: line over 80 characters
	#1046: FILE: scripts/qmp_helper.py:442:
	+                           help="Marks the timestamp as precise if --timestamp is used")

	WARNING: line over 80 characters
	#1048: FILE: scripts/qmp_helper.py:444:
	+                           help=f"General Error Data Block flags: {gedb_flags_bits}")

   Those might be changed if we add one variable per string to store the
   help lines, at the expense of doing some code obfuscation.

   I don't think doing it is a good idea.

3. Long class function names that are part of Python's standard library:

	ERROR: line over 90 characters
	#576: FILE: scripts/ghes_inject.py:29:
	+    parser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter,

   We can't change the big name of the argparse formatter. The only
   possible fix would be to obfuscate it by doing:

	format = argparse.ArgumentDefaultsHelpFormatter,
	parser = argparse.ArgumentParser(formatter_class=format,

   IMO this is a bad practice.

4. False-positive warning disable for pylint coding style tool:

	ERROR: line over 90 characters
	#805: FILE: scripts/qmp_helper.py:201:
	+        data.extend(value.to_bytes(num_bytes, byteorder="little"))  # pylint: disable=E1101

	WARNING: line over 80 characters
	#1028: FILE: scripts/qmp_helper.py:424:
	+        g_gen = parser.add_argument_group("Generic Error Data")  # pylint: disable=E1101

   AFAIKT, those need to be at the same line for pylint to process them
   properly.

5. A long name inside an indented block:

	WARNING: line over 80 characters
	#1109: FILE: scripts/qmp_helper.py:505:
	+                                                   value=args.gen_err_valid_bits,

   Again the only solution would be to obfuscate the argument, like:

	a = args.gen_err_valid_bits

							    value=a,

   Not nice, IMHO.

Now, there is one warning that I is not a false positive, which I ended
missing:

	WARNING: line over 80 characters
	#1227: FILE: scripts/qmp_helper.py:623:
	+            ret = self.send_cmd("qom-get", args, may_open=True, return_error=False)

I'll fix it at the next respin.

Regards,
Mauro

Powered by blists - more mailing lists