On Thu, Feb 8, 2018 at 4:59 PM, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > On Thu, 2018-02-08 at 16:48 +0100, Rafael J. Wysocki wrote: >> On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko >> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: >> > On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote: >> > > > >> > >> > > Also the return value here means "success", so why is an error the >> > > right choice? >> > >> > Because we need to return something which is not NULL. Naturally >> > feels >> > the error code, esp. ENODATA, is quite suitable. We indeed have no >> > data >> > in this case, and it's not a NULL case (not found / not match) — we >> > have >> > a match. >> >> But this is an error code that means "success". May I call it rather >> confusing? > > This function AFAICS does two things at once: > - matches device against ID > - returns matched ID entry in the table > > Return value combines those two into actually ternary option: > - no match > - match with ID > - match without ID Right. And the caller knows when to expect the third case which is the whole point. >> > > Overall, this really looks like a preparation for a future patch, >> > > so >> > > why not just say that straight away in the changelog? >> > >> > It's not _just_ a preparation, it mitigates the trick used in >> > mentioned >> > by Fixes tag commit. >> > >> > I would rather update comment here, and add explanation to the >> > commit >> > message to be sure it covers tricks mitigation and preparation >> > purposes. >> >> This is not mitigation, sorry. It just replaces one possibly >> confusing thing with another. > > I would agree here... > >> The code as is works as I said and this patch doesn't make it any >> better as far as I'm concerned. > > ...but not here. Instead of returning pointer to *something* (from > caller point of view), we explicitly tell caller what of the above > happened. We don't rely on the organization of ID table or its life > time (though it's forever). > > I can say that is *slightly* better. But agree that is not cleanest > solution I can come up with. > > I'm all ears on other possibilities how to get rid of that trick. Please see my reply to the second patch, then. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
