I think implementing flashrom_set_loglevel() is a good idea. Sorry I didn't respond earlier.
Just to confirm, is the idea to set a log level, and then invoke print callback only for the prints which have log level higher than that? > Also, what was the original idea behind struct flashrom_programmer? I am not sure, I wasn't here in 2017 when it was introduced, and currently it is [still] unused. It seems the natural development went by the path of storing info in flashrom_flashctx and struct flashrom_programmer has never been used. It is passed only into init/shutdown/probe, so might not be enough for logging anyway? > But where should it be stored? Another global variable... yeah :( I don't know how to avoid another global variable log callback was relying on global state from the very beginning (unlike progress callback for example), and setting log callback does not have flash context, neither print function has flash context. And log callback v2 is partially using existing logging "infrastructure", so now it's bound to global state too. So, I guess, yes, log level will be another global variable On Tue, Mar 18, 2025 at 10:30 AM Dmitry Zhadinets <dzhadin...@gmail.com> wrote: > > Probably this is a better place than Gerrit to discuss my question about this > line > https://github.com/flashrom/flashrom/blob/c3b89597fc61e14c4bc3a1e57e949483325c1e76/libflashrom.c#L47 > What if we implement this in the logger APIg patch? It would prevent many > calls, unnecessary allocations, and redundant formatting. Flashrom would > handle and cut them in advance. > But where should it be stored? Another global variable... > Or is it finally time to introduce struct flashrom_programmer? > Also, what was the original idea behind struct flashrom_programmer? > I'm a bit confused about its purpose, especially since you already have > struct flashrom_flashctx and looks like flashrom is like a single thread > library > > > On Mon, Mar 17, 2025 at 4:48 PM Anastasia Klimchuk <a...@chromium.org> wrote: >> >> Dmitry, thank you for the patch! I was so happy to see you made a patch >> already, that I started reviewing and forgot to post here :) >> >> So, we have moved to code review for #2, if anyone is interested, have a >> look in Gerrit. >> >> Anastasia >> >> On Mon, 17 Mar 2025, 13:39 Dmitry Zhadinets, <dzhadin...@gmail.com> wrote: >>> >>> Sorry for a late reply >>> >>> Yes, definitely I will be interested. Moreover, today I've implemented an >>> idea for your review. it is a bit more than needed. But probably you will >>> like something from it. I'm cleaning it and will push it soon. >>> I've pushed a patch about logs. So any comments are welcome. Please take a >>> look. https://review.coreboot.org/c/flashrom/+/86875 >>> >>> On Fri, Mar 14, 2025 at 9:21 AM Anastasia Klimchuk <a...@chromium.org> >>> wrote: >>>> >>>> Hello Dmitry, >>>> >>>> Thank you for your work to create a UI for flashrom! >>>> >>>> Overall question to all your 1,2,3 points: if we get to implementing >>>> these, would you be interested to get involved, for example test a >>>> patch or involved in any other way? Especially that you would be using >>>> the new API functions. >>>> >>>> On Fri, Mar 14, 2025 at 10:18 AM Peter Marheine <pmarhe...@chromium.org> >>>> wrote: >>>> > >>>> > Hey Dmitry, >>>> > >>>> > Thanks for the notes! There aren't many general-purpose users of the >>>> > library API so I'm sure it does currently have some rough edges- it's >>>> > nice to have somebody interested in improving it. >>>> > >>>> >> - how to obtain the list of programmers , as far as I see there is no >>>> >> any way to do it for now. Maybe using the log routine but it is >>>> >> definitely not the best way >>>> > >>>> > >>>> > There isn't currently an API for this, but it would be nice to add one. >>>> > I imagine a function returning strings would be sufficient, and that >>>> > could possibly be expanded later with a way to discover the parameters >>>> > supported by each programmer (a function taking a programmer name and >>>> > returning some kind of parameter info struct). >>>> > >>>> >> >>>> >> - could you add userdata to the callbacks you are calling . just a >>>> >> void* without any obligations >>>> > >>>> > >>>> > https://review.coreboot.org/c/flashrom/+/86031 added that to the >>>> > progress callback, but it looks like we should also have one for log >>>> > callbacks; is that right? >>>> > >>>> >> - probing: in case of multiple chips it returns just an error about >>>> >> it. but there is no option to enumerate what was found. BTW relating to >>>> >> some previous thread. It would be great that libflasrom probe works >>>> >> like cli version (to probe everything but not only first 2) >>>> > >>>> > >>>> > That doesn't seem too difficult either, and I agree it would be good to >>>> > have. I imagine adding a new API similar to the internal probe_flash >>>> > function allowing the user to iterate through detected chips (rather >>>> > than detecting all of them and returning a list). >>>> > _______________________________________________ >>>> > flashrom mailing list -- flashrom@flashrom.org >>>> > To unsubscribe send an email to flashrom-le...@flashrom.org >>>> >>>> >>>> >>>> -- >>>> Anastasia. -- Anastasia. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
