Dotaz: zamítnutý patch v linux-media [bylo: asi jsem totalni trotl]

[Kju]

Vypada to, ze jsem asi mimo a proto mi byl odmitnut velice primitivni patch. Jelikoz jsem se v linux-media nedozvedel, co je na mem patchi tak spatne, tak se pokousim zeptat zde.

Zde je muj patch, vcetne cele komunikace. Na moji zaverecnou otazku, plati-li stale NACK, jsem jiz nedostal odpoved (ac Antti dal vesele komunikuje, takze to urcite nema nic spolecneho s odjezdem na dovolenou):

https://patchwork.kernel.org/patch/1001172/

Je mozne, ze muj popis je anglicky nespravne, jinak si totiz nedokazu vysvetlit, proc byl odmitnut. Na druhou stranu - jedna se o 4-radkovy patch, ten je prakticky samokomentujici se.

A tak jen doplnim cesky popis zaplaty: Osetruje pripad, kdy vstupni pole msg[] ma jen jeden prvek.

Podle me pak prece pripadna reference na msg[1] muze (ale uznavam, ze nemusi) vyvolat pad systemu, pripadne jenom prislusneho modulu.

Jestli se mylim, tak me prosim poucte, mozna jsem jen neco prehledl.

PS: vstupni pole msg[] opravdu byva i jen jednoprvkove.

Diky

Honza

---

---

Odpovědi

[dustin]

Není nad vhodně zvolený název tématu...

[Kju]

Uznavam, ze nazev je hloupy. Uz jsem pozadal admina o zmenu (jde-li to).

Honza

[Filip Jirsák]

Byl jsme rychlejší, takže název už jsem opravil. Příště by to chtělo přemýšlet nad názvem příspěvku i nad jeho formátováním – každému by se s dotazem pracovalo mnohem líp, kdyby např. odkazy byly opravdu odkazy.

[Kju]

S tim nazvem jsem to uznal.

A k tomu odkazu - nejak jsem si myslel, ze to detekuje automaticky. Jsem zblbly z ruznych bb-for.

Honza

[Filip Jirsák]

Před každým odesláním dotazu i komentáře se zobrazuje náhled – je dobré se na něj podívat.

[karlosko]

Podle mne je patch spravny. Duvod proc to nikdy nespadne muze byt v tom, ze pokud je predavane pole vytvorene na zasobniku(coz by bylo rozumne minimalne z hlediska vykonu), tak tam za nim vzdycky jeste je co cist a k pristupu mimo pamet tak nemuze dojit - cte to sice nejakou garbage, ale protoze ta se pak nikde nepouzije, je to ve vysledku jedno. Funkce je podle mne na prvni pohled vadna, ale v kontextu toho, jak se vola, muze fungovat bez problemu. Typicky kandidat na objeveni po 10 letech jako "historicky bug"

[Petr Tomášek]

Nějak jsem to nepochopil, řeší tvůj peč nějaký konkrétní problém? Nebo je to jen taková teoretická onanie nad tím, že by tam teoreticky mohlo něco nefungovat (ale nakonec funguje s pečem i bez něj)?

[OldFrog {Ondra Nemecek}]

Patch reší konkrétní chybu kódu - že tato chyba shodnou okolností možná nevede k vadné funkčností neznamená, že to není chyba. Vy neopravujete zjevné chyby v kódu? Já ano a rozhodně bych to nenazval teoretickou onanií. V původním kódu minimálně chybí vysvětlující komentář. Minimálně ten by měl být do kódu doplněn.

Jiná situace by samosebou nastala, pokud by byla funkčnost a bezpečnost daného kódu zajištěna nějak koncepčně - pokud by bylo to součásti určité strategie (návrhového vzoru) a dané věc se řešila např. v jiné části kódu (wraperem nebo podobně).

Dále bych přihlédnul i k povaze dotazu, kdy je autor patche zmatený z reakce jaderného týmu a hledá vysvětlení. Je dobré k němu přistupovat vstřícně, aby nebyl odrazen od dalšího přispívání do kódu. Nemyslím, že by si prostřednictvím patche nějak honil ego nebo tak něco. Tolik můj postoj.

[Petr Tomášek]

Jo, měl jsem tam dát nějaké smajlíky, aby bylo jasné, že prudím. Vomlouvám se, náčelníku... :-P

[Kju]

Tak znova a mozna vice po lopatisticky: Funkce zacina takhle:

static int cxd2820r_tuner_i2c_xfer(struct i2c_adapter *i2c_adap,
        struct i2c_msg msg[], int num)
{
        struct cxd2820r_priv *priv = i2c_get_adapdata(i2c_adap);
        u8 obuf[msg[0].len + 2];
        struct i2c_msg msg2[2] = {
                {
                        .addr = priv->cfg.i2c_address,
                        .flags = 0,
                        .len = sizeof(obuf),
                        .buf = obuf,
                }, {
                        .addr = priv->cfg.i2c_address,
                        .flags = I2C_M_RD,
                }
        };

Problem nastava, kdyz je tato funkce volana z ovladace tda18271
s parametrem msg[] delky = 1 (jen jeden prvek pole)
a to takhle:

int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
{
        struct tda18271_priv *priv = fe->tuner_priv;
        unsigned char *regs = priv->tda18271_regs;
        unsigned char buf[TDA18271_NUM_REGS + 1];
        struct i2c_msg msg = { .addr = priv->i2c_props.addr, .flags = 0,
                               .buf = buf };
        int i, ret = 1, max;

        BUG_ON((len == 0) || (idx + len > sizeof(buf)));


        switch (priv->small_i2c) {
        case TDA18271_03_BYTE_CHUNK_INIT:
                max = 3;
                break;
        case TDA18271_08_BYTE_CHUNK_INIT:
                max = 8;
                break;
        case TDA18271_16_BYTE_CHUNK_INIT:
                max = 16;
                break;
        case TDA18271_39_BYTE_CHUNK_INIT:
        default:
                max = 39;
        }

        tda18271_i2c_gate_ctrl(fe, 1);
        while (len) {
                if (max > len)
                        max = len;

                buf[0] = idx;
                for (i = 1; i <= max; i++)
                        buf[i] = regs[idx - 1 + i];

                msg.len = max + 1;

                /* write registers */
                ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);

Protoze prave tady se realne vola cxd2820r_tuner_i2c_xfer().
Jak je vidno, tak msg[] je vytvoreno na zasobniku, takze
to je nejspis duvod, proc to autorovi nepada. Ale
spatne to je.

Honza

[Kju]

Omlouvam se, omylem jsem prvni kod vlozil spatnou verzi.
Tady je ten autoruv puvodni:

static int cxd2820r_tuner_i2c_xfer(struct i2c_adapter *i2c_adap,
        struct i2c_msg msg[], int num)
{
        struct cxd2820r_priv *priv = i2c_get_adapdata(i2c_adap);
        u8 obuf[msg[0].len + 2];
        struct i2c_msg msg2[2] = {
                {
                        .addr = priv->cfg.i2c_address,
                        .flags = 0,
                        .len = sizeof(obuf),
                        .buf = obuf,
                }, {
                        .addr = priv->cfg.i2c_address,
                        .flags = I2C_M_RD,
                        .len = msg[1].len,
                        .buf = msg[1].buf,
                }
        };

[pc2005]

Dost zajímavá chyba. Podle tý ukázky mě celej ten kód připadá na nějaké větší upravení.

P.S. Chtělo to hodit sem větší kontext. Ze začátku jsem se v tom zamotal.

[Luboš Doležel (Doli)]

Tak prece jen zmena - podarilo se mi podiskutovat s Anttim pres IRC a nakonec ten patch uznal. Takze ho udelam proti release 3.0 a poslu mu ho znova.

Fakt je to ale hodne odrazujici chovani. Takova hloupost a clovek si pak pripada, jako by chtel otravovat.

Honza

[karlosko]

Asi je treba byt pripraveny na to, ze patch resici realne nenastavajici problem ten spravce moc detailne nezkouma. Dela to ve volnem case a jestli ma takovych deset za den... Kdyby te mela odradit trocha dohadovani, tak toho na kernelu moc neudelas - take it easy

[dustin]

Díky, žes to nevzdal a dotáhl do konce.

[vain]

Hele to mě zajímá, když jste o tom diskutovali a tedy "nakonec uznal". O čem jste mluvili? Vždyť ten patch je dost triviální záležitostí, tak co mu prvně vadilo?

[Kju]

Rekl bych, ze z nejakeho (mne neznameho) duvodu se nad tim proste nechtel pozastavovat.

A tak jsme diskutovali, jestli kernel spadne, kdyz se provede takovy lookup, ja mu psal, ze na lokalnich promennych (a tudiz na zasobniku) na 99% ne, ale kdyby se napr. pouzilo staticke pole, tak by mohlo atd atd. Nakonec jsem ho musel utnout a zeptal jsem se ho, jestli mu ten jeho kod prijde korektni, kdyz vstupni pole muze byt jen jednoprvkove. Az pak obratil.

No proste na muj vkus jsme si u tak primitivni zaplaty vymenili az neskutecne moc mailu a pak jsem to s nim jeste musel probrat na irc, protoze na maily prestal reagovat.

Uz to tu zaznelo, ze je to bezne, ze se musi dost casto bojovat. Me to normalni neprijde, ale jsem schopen si zvyknout. Obvykle se nenecham tak rychle odradit :)

[Ivan]

Musim priznat, ze presne tohle me odradilo od nejakyho hrabani se v kernelu. Ted kdyz vydelavam a mam rodinu, tak nemam tolik casu. Jeste par lety pro me nebyl problem projit kus kodu neco tam porochat a upravit podle svy potreby. Dostat to do stavu kdy by to bylo formalne akceptovatelny a pak to nekam protlacit tak to vyzaduje, aby se tomu clovek venoval opravdu naplno. Aby si vybudoval nejaky jmeno. Jinak to opravdu dopada tak, ze venujes ctyrem radkum kodu dva tydny zivota.

[Filip Jirsák]

Jenže možná právě to je jeden z důvodů úspěchu Linuxu. Nedostane se tam každý kód, který někdo splácá za deset minut a za půl hodiny už o něj nejeví zájem.

[Kju]

Splacat kod za deset minut mi prijde dost nerealne.

Uz jenom to, ze je ho treba prohnat validacnimi nastroji (minimalne checkpatch.pl) zabere vic nez onech deset minut.

Navic, to co jsem provedl ja, bylo jen pouhe presunuti kodu do bloku, kde je to bezpecne (patricny if() s podminkou uz ten zdrojak stejne obsahoval), takze je mozne rict, ze jsem splacal 0 radku kodu.

A jeste jednou k casovemu udaji - ja ten kod studoval skoro 2 dny, jelikoz se ho snazim vyuzit pro vlastni potrebu. Myslim, ze jinak se clovek k detekci takoveto chyby tezko dostane. Funkce je pouzivana neprimo a to z jineho ovladace.

Ja se na to mohl uplne vykaslat, opravit si to u sebe a vesele vyuzivat kod. Prijde mi to ale nefer a navic je to jedna z mala moznosti, jak neco vratit komunite.

[Filip Jirsák]

To nebylo myšleno proti vám. Jde o to, že to, co autory (jako vás) otravuje, je ale zároveň mechanismus, který udržuje určitou kvalitu kódu. Samozřejmě se to nesmí přehánět, ale jisté nepohodlí pro autory není nic špatného.

[Kju]

Tak s tim, samozrejme, souhlasim.

Je pravda, ze ja mam zkusenosti pouze s linux-media subsystemem, takze vubec nemuzu generalizovat, ale po predchozim odmitnuti virtualizacniho ovladace jsem na ne jeste stale napruzeny.

Jinak souhlas - formalizace je nutna a i ta zvysena komunikace pri prijimani sebemensi zaplaty neni na skodu. Hlavne v pripade, kdy autor je pro mainatainera neznamy a tak je vlastne nucen cely kod peclive zanalyzovat.

[Ivan]

Jo to uznavam. Na druhou stranu na necem podobme je postavena vetsina opensource. V mem pripade to bylo o tom, ze jsem mel - narozdil od core vyvojaru - pristup k velice drahemu HW a jeste jsem k nemu mel vice-mene kompletni SCSI command reference. No a stavajici genericky ovladac se k tomu zerizeni choval "hloupe".

[dustin]

Je pravda, že i sebemenší patch vyžaduje formální náležitosti (např. u alsy výstup z git-format-patch, zkontrolované checkpatch.pl, atd.). Samozřejmě chvíli trvá, než se to člověk naučí. Na druhou stranu kdyby správci subsystémů akceptovali všechny patche, co jim přijdou, tak by jim z toho hráblo a kvalita by šla do kytek. Takže jsem rád za stávající striktnější požadavky.

Samozřejmě když ješitný autor nechce akceptovat opravu své jasné chyby, je to o něčem jiném...

[R]

Mala otazka: otestoval si ten patch? Ak ano, tak si to mal hned do prveho mailu napisat. Ak nie, tak sa nediv, ze nebol prijaty hned, pretoze takto sa najlepsie rozbijaju funkcne drivery. Nic nie je horsie ako ked nejaky HW po upgrade jadra prestane fungovat (podobne naposledy niekto rozbil floppy v 2.6.39).

Dalej patch nebol trivialny, takze mal byt k nemu lepsi popis, aby z neho bolo jasne, aky problem patch riesi a akym sposobom.

[Kju]

Jak to myslis otestoval? Samozrejme jsem ho u sebe napred aplikoval.

A ze nebyl trivialni? To si delas srandu. Co je netrivialniho na presunu inicializace druheho prvku struktury (tj, ctyri radky prirazujici hodnoty) do mista, kde je osetreno, ze dat jsou ctena z existujiciho prvku a ne odnekud "ze vzduchu". Navic presun snad jenom o 5 radku niz.

A co je spatneho na subjektu "[PATCH] cxd2820r: fix possible out-of-array lookup"? A kapanek podrobnejsi popis nasledoval v tele zpravy. Samozrejme jsem tam mohl uvest konkretni backtrace, kde by bylo videt, kdy presne k tomu dojde. Ale u ctyrradkove zaplaty? Vsimni si, ze jsem ani nemenil logiku a zadny vypocet. Fakt jsem jenom posunul nastaveni do bloku, ktery se provadi prave jenom kdyz je jasne, ze tam ten druhy prvek opravdu je.

To jsem fakt tak zaslepeny, ze to prijde jenom mi, ze se jedna o primitivni patch?

No, ve finale je to uz jedno - dostal jsem (a nebo ziskal? ACK, takze je vyhrano.

[pc2005]

Imho je ten patch jednoduchý, ale ten kód okolo je dost humus. Takže na první pohled uznat přínos není tak jednoduchý. Určitě kdybys mu dal nějakej krokovanej příklad, tak by ti to uznal hned napoprvé. Výsledek by byl stejnej, jenom byste nad tím oba strávili méně mailů a tedy byste měli víc ušetřenýho času.

[Petr Tomášek]

A ze nebyl trivialni? To si delas srandu.

I triviální patch může mít netriviální důsledky :-O