linux-kernel - Re: [PATCH v2 14/41] drm/modes: Move named modes parsing to a separate function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220907083945.3cqmz765vmnjt3ol@houat>
Date:   Wed, 7 Sep 2022 10:39:45 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Jani Nikula <jani.nikula@...ux.intel.com>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Ben Skeggs <bskeggs@...hat.com>,
        David Airlie <airlied@...ux.ie>, Chen-Yu Tsai <wens@...e.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Lyude Paul <lyude@...hat.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Karol Herbst <kherbst@...hat.com>,
        Noralf Trønnes <noralf@...nnes.org>,
        Emma Anholt <emma@...olt.net>, Daniel Vetter <daniel@...ll.ch>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Phil Elwell <phil@...pberrypi.com>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Dom Cobley <dom@...pberrypi.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Nouveau Dev <nouveau@...ts.freedesktop.org>,
        linux-sunxi@...ts.linux.dev,
        Mateusz Kwiatkowski <kfyatek+publicgit@...il.com>
Subject: Re: [PATCH v2 14/41] drm/modes: Move named modes parsing to a
 separate function

Hi,

On Tue, Aug 30, 2022 at 04:36:23PM +0300, Jani Nikula wrote:
> On Tue, 30 Aug 2022, Maxime Ripard <maxime@...no.tech> wrote:
> > Hi,
> >
> > On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
> >> On Tue, 30 Aug 2022, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> >> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime@...no.tech> wrote:
> >> >> +#define STR_STRICT_EQ(str, len, cmp) \
> >> >> +       ((strlen(cmp) == len) && !strncmp(str, cmp, len))
> >> >
> >> > This is not part of the move, but newly added.
> >> 
> >> The same construct is also duplicated elsewhere in the series, and I
> >> kept being confused by it.
> >
> > I'm not sure what is confusing, but I can add a comment if needed.
> 
> STR_STRICT_EQ() is what's confusing. I have to look at the
> implementation to understand what it means. What does "strict" string
> equality mean?

Same length, same sequence of characters

> >
> >> The above is precisely the same as:
> >> 
> >> 	str_has_prefix(str, cmp) == len
> >
> > Here, it's used to make sure we don't have a named mode starting with
> > either e, d, or D.
> >
> > If I understood str_has_prefix() right, str_has_prefix("DUMB-MODE", "D")
> > == strlen("DUMB-MODE") would return true, while it's actually what we
> > want to avoid.
> 
> That's not true, str_has_prefix("DUMB-MODE", "D") == strlen("D") is.
> 
> > It's also used indeed in drm_get_tv_mode_from_name(), where we try to
> > match a list of names with one passed as argument.
> >
> > With drm_get_tv_mode_from_name("NSTC", strlen("NTSC")), we would end up
> > calling str_has_prefix("NTSC-J", "NTSC") == strlen("NTSC-J") which would
> > work. However, we end up calling prefix not a prefix, but an entire
> > string we want to match against, which is very confusing to me too.
> 
> If I get this right, you have a string and you want to check if that has
> a certain prefix. Additionally, you want to check the prefix is a
> certain length.
> 
> Sure, that the prefix is a certain length is more of a property of the
> string, which is NUL terminated later than at length, but that's doesn't
> really matter.
> 
> That condition is simply str_has_prefix(string, prefix) == length.

Ack. I'm ok with the implementation being done that way, but I'd really
prefer to still have some macro to make the name less confusing. Would
that work for you? What name would be better in your opinion?

Thanks!
Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)