On Wed, Sep 06, 2017 at 12:31:38PM +0200, Claudius Heine wrote:
>
>
> On 09/05/2017 10:27 AM, [ext] Andreas Reichel wrote:
> > From: Reichel Andreas <[email protected]>
> >
> > - the introduction of a special marker byte which tells the algorithm
> > - to not touch the original content. Deletion of user variables led to
> > - another special case, where *negative* variables had to be defined by
> > - a special `DELETED` type to tell the algorithm that the specific user
> > - variable has to be deleted. This is rather complicated and a better
> > - aproach has already been discussed using a journal with actions
> > - instead of a prebuilt state.
>
> I am not sure if TODO files in a repo is the best way forward. Maybe rather
> use issues on github for this?
>
If Jan agrees, we can move to github issues after this file is empty.
> > +
> > +struct stailhead *headp;
> > +struct env_action {
> > + char *key;
> > + char *type;
> > + uint8_t *data;
> > + BGENV_TASK task;
> > + STAILQ_ENTRY(env_action) journal;
> > +};
> > +
> > +STAILQ_HEAD(stailhead, env_action) head = STAILQ_HEAD_INITIALIZER(head);
>
> This is a global variable. I would prefer to put all of those into a
> context, just because its cleaner and you might at some point move those
> functions into multiple files. Maybe the library can profit from this
> journaling approach to changing the environment as well.
>
> I have seen a lot of applications where the 'main' file contains only
> command line parsing and stdout/stderr printing while everything else is in
> a library of sorts. I liked this approach, because it separates cleanly code
> for cli from the rest.
Yes, good idea, but not in this patch set anymore. It is already too
huge and should really be merged before even more deep changes to any
API. Furthermore, the underlying API is the API this tool uses and it
should not define a new API just to abstract things even more. If the
tool is extended to a lot new functionality then one can think of
separating the bg_setenv.c into several object files. At the moment I
really don't see this is necessary. While splitting logical units and
abstracting might be or is good practise in theory, it may also increase
difficulties in reading code and/or maintaining it - especially if it is
only one stand-alone tool that does not provide any functions for
external utilization others than commandline usage. For such a small
tool I prefer KISS principle.
>
> > +
> > +static void journal_add_action(BGENV_TASK task, char *key, char *type,
> > + uint8_t *data, size_t datalen)
> > +{
> > + struct env_action *new_action;
> > +
> > + new_action = calloc(1, sizeof(struct env_action));
>
> Is there a reason why you use calloc here instead of malloc?
>
Yes, because calloc zeroes memory and malloc does not.
> Claudius
>
Andreas
--
Andreas Reichel
Dipl.-Phys. (Univ.)
Software Consultant
[email protected], +49-174-3180074
TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterfoehring
Geschaeftsfuehrer: Henrik Klagges, Dr. Robert Dahlke, Gerhard Mueller
Sitz: Unterfoehring * Amtsgericht Muenchen * HRB 135082
--
You received this message because you are subscribed to the Google Groups "EFI
Boot Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/20170906151744.GA914%40iiotirae.
For more options, visit https://groups.google.com/d/optout.
