diff mbox series

[07/11] edid-decode: always linefeed after hex_block

Message ID 20210914121129.51451-8-joevt@shaw.ca (mailing list archive)
State New, archived
Headers show
Series edid-decode: bug fixes, additions, changes | expand

Commit Message

joevt Sept. 14, 2021, 12:11 p.m. UTC 
hex_block should not return without printing a newline (which occurs when the length is zero). This causes a missing newline after "Application Version: 1" with cta_hdr10plus for an EDID I have.
Any place that calls hex_block will have the same problem if it's possible for the length to be zero.

In other words, a hex_block needs to have a linefeed even if it has zero length, because the caller assumes it will go to the next line as it does when the hex block is not zero length.

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 edid-decode.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Hans Verkuil Sept. 15, 2021, 10:10 a.m. UTC | #1 
On 14/09/2021 14:11, joevt wrote:
> hex_block should not return without printing a newline (which occurs when the length is zero). This causes a missing newline after "Application Version: 1" with cta_hdr10plus for an EDID I have.
> Any place that calls hex_block will have the same problem if it's possible for the length to be zero.
> 
> In other words, a hex_block needs to have a linefeed even if it has zero length, because the caller assumes it will go to the next line as it does when the hex block is not zero length.
> 
> Signed-off-by: Joe van Tunen <joevt@shaw.ca>
> ---
>  edid-decode.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/edid-decode.cpp b/edid-decode.cpp
> index 2316abc..6aa93fb 100644
> --- a/edid-decode.cpp
> +++ b/edid-decode.cpp
> @@ -698,8 +698,10 @@ void hex_block(const char *prefix, const unsigned char *x,
>  {
>  	unsigned i, j;
>  
> -	if (!length)
> +	if (!length) {
> +		printf("\n");
>  		return;
> +	}

Hmm, with this change I get this:

edid-decode -c Digital/Acer/ACR0282/B12D637C1F12 with the linuxhw database:

  Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
    Source physical address: 2.0.0.0
  Unknown CTA-861 tag 0x00, length 0

  Unknown CTA-861 tag 0x00, length 0

  Unknown CTA-861 tag 0x00, length 0

  Unknown CTA-861 tag 0x00, length 0

  Unknown CTA-861 tag 0x00, length 0

  Unknown CTA-861 tag 0x00, length 0

  Unknown CTA-861 tag 0x00, length 0

That looks pretty ugly.

Regards,

	Hans

>  
>  	for (i = 0; i < length; i += step) {
>  		unsigned len = min(step, length - i);
>
joevt Sept. 15, 2021, 3:43 p.m. UTC | #2 
Yes, that's ugly. I will do a search for the EDID that prompted me to make this change. Maybe it's not a problem anymore.
...
Seems like the problem I had was fixed in cta_hdr10plus. I'll do more checking and testing with other calls to hex_block.


On 2021-09-15, 3:10 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote:

    On 14/09/2021 14:11, joevt wrote:
    > hex_block should not return without printing a newline (which occurs when the length is zero). This causes a missing newline after "Application Version: 1" with cta_hdr10plus for an EDID I have.
    > Any place that calls hex_block will have the same problem if it's possible for the length to be zero.
    > 
    > In other words, a hex_block needs to have a linefeed even if it has zero length, because the caller assumes it will go to the next line as it does when the hex block is not zero length.
    > 
    > Signed-off-by: Joe van Tunen <joevt@shaw.ca>
    > ---
    >  edid-decode.cpp | 4 +++-
    >  1 file changed, 3 insertions(+), 1 deletion(-)
    > 
    > diff --git a/edid-decode.cpp b/edid-decode.cpp
    > index 2316abc..6aa93fb 100644
    > --- a/edid-decode.cpp
    > +++ b/edid-decode.cpp
    > @@ -698,8 +698,10 @@ void hex_block(const char *prefix, const unsigned char *x,
    >  {
    >  	unsigned i, j;
    >  
    > -	if (!length)
    > +	if (!length) {
    > +		printf("\n");
    >  		return;
    > +	}

    Hmm, with this change I get this:

    edid-decode -c Digital/Acer/ACR0282/B12D637C1F12 with the linuxhw database:

      Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
        Source physical address: 2.0.0.0
      Unknown CTA-861 tag 0x00, length 0

      Unknown CTA-861 tag 0x00, length 0

      Unknown CTA-861 tag 0x00, length 0

      Unknown CTA-861 tag 0x00, length 0

      Unknown CTA-861 tag 0x00, length 0

      Unknown CTA-861 tag 0x00, length 0

      Unknown CTA-861 tag 0x00, length 0

    That looks pretty ugly.

    Regards,

    	Hans

    >  
    >  	for (i = 0; i < length; i += step) {
    >  		unsigned len = min(step, length - i);
    >
joevt Sept. 15, 2021, 6:27 p.m. UTC | #3 
I found no place that requires this change to hex_block.
This patch should be ignored.

I did find one call to hex_block that should be modified:
cta_hdr10plus outputs hex on the same line as the "Application Version: %u".

It either needs to always output a linefeed before calling hex_block like this:

	printf("    Application Version: %u\n", x[0]);
	hex_block("    ", x + 1, length - 1);

Or it needs to set step to the same value as length, like this:

	printf("    Application Version: %u", x[0]);
	if (length > 1)
		hex_block("  ", x + 1, length - 1, true, length - 1);
	else
		printf("\n");

Those are probably the only acceptable ways to call hex_block (first is multi-line or no-line, second is one-line)

It should also probably check the length:

	if (length == 0) {
		fail("Empty Data Block with length %u.\n", length);
		return;
	}


On 2021-09-15, 8:43 AM, "Joe van Tunen" <joevt@shaw.ca> wrote:

    Yes, that's ugly. I will do a search for the EDID that prompted me to make this change. Maybe it's not a problem anymore.
    ...
    Seems like the problem I had was fixed in cta_hdr10plus. I'll do more checking and testing with other calls to hex_block.


    On 2021-09-15, 3:10 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote:

        On 14/09/2021 14:11, joevt wrote:
        > hex_block should not return without printing a newline (which occurs when the length is zero). This causes a missing newline after "Application Version: 1" with cta_hdr10plus for an EDID I have.
        > Any place that calls hex_block will have the same problem if it's possible for the length to be zero.
        > 
        > In other words, a hex_block needs to have a linefeed even if it has zero length, because the caller assumes it will go to the next line as it does when the hex block is not zero length.
        > 
        > Signed-off-by: Joe van Tunen <joevt@shaw.ca>
        > ---
        >  edid-decode.cpp | 4 +++-
        >  1 file changed, 3 insertions(+), 1 deletion(-)
        > 
        > diff --git a/edid-decode.cpp b/edid-decode.cpp
        > index 2316abc..6aa93fb 100644
        > --- a/edid-decode.cpp
        > +++ b/edid-decode.cpp
        > @@ -698,8 +698,10 @@ void hex_block(const char *prefix, const unsigned char *x,
        >  {
        >  	unsigned i, j;
        >  
        > -	if (!length)
        > +	if (!length) {
        > +		printf("\n");
        >  		return;
        > +	}

        Hmm, with this change I get this:

        edid-decode -c Digital/Acer/ACR0282/B12D637C1F12 with the linuxhw database:

          Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
            Source physical address: 2.0.0.0
          Unknown CTA-861 tag 0x00, length 0

          Unknown CTA-861 tag 0x00, length 0

          Unknown CTA-861 tag 0x00, length 0

          Unknown CTA-861 tag 0x00, length 0

          Unknown CTA-861 tag 0x00, length 0

          Unknown CTA-861 tag 0x00, length 0

          Unknown CTA-861 tag 0x00, length 0

        That looks pretty ugly.

        Regards,

        	Hans

        >  
        >  	for (i = 0; i < length; i += step) {
        >  		unsigned len = min(step, length - i);
        >
diff mbox series

Patch

diff --git a/edid-decode.cpp b/edid-decode.cpp
index 2316abc..6aa93fb 100644
--- a/edid-decode.cpp
+++ b/edid-decode.cpp
@@ -698,8 +698,10 @@  void hex_block(const char *prefix, const unsigned char *x,
 {
 	unsigned i, j;
 
-	if (!length)
+	if (!length) {
+		printf("\n");
 		return;
+	}
 
 	for (i = 0; i < length; i += step) {
 		unsigned len = min(step, length - i);