[parted-devel] History patchset

[Matt Davis]

On Fri, Feb 08, 2008 at 03:01:50PM +0100, Jim Meyering wrote:
> Matt Davis <mattdavis9 at gmail.com> wrote:
> 
> > The following 3 patches implement history/undo/redo behavior for parted.
> >
> > This change captures potential disk modifications based on what commands a user
> > issues.  The list of disk modifications can be displayed/queried, undone, or
> > redone.  Eventually the changes can be commited to disk.  This prevents the user
> > from performing unwanted modifications to disk before they are ready.  Also, the
> > user can view the disk as if the modifications were taking place, even before
> > the "save" their changes.
> ...
> > diff --git a/libparted/history.c b/libparted/history.c
> ...
> > +char PedHistStr[32];
> ...
> > +PedHistManager PedHistMgr = {0};
> ...
> > +void
> > +ped_history_add (const char *name)
> ...
> > +void
> > +ped_history_clear (void)
> 
> Hi Matt,
> 
> I see at least two global variables there.
> The existing ones cause enough trouble, (and this is library code),
> so please don't add new global variables.

Jim, Otavio,
First of all I appreciate all of your responses.

I am not a fan of globals either.  I do feel a bit ashamed that they showed up,
but the reasoning was to keep the Manager as if it were a singleton.  But,
after some thinking, I do not like the singleton idea, and we can pass our
user-created (parted.c instantiated) history manager around.  Thanks for the
suggestion, it opened my mind to your vision.


> 
> As I said before, you will want to adjust each of your history-manipulation
> functions to take a pointer, probably *PedHistManager:
> 
>     void
>     ped_history_add (PedHistManager *h, const char *name)
> 
> which brings up another point:
> That function can fail (it calls malloc), hence it must not be void.

Yep.  I can attest to that.  As I mentioned above, I like your idea better.
Likewise, it can be extended to more uses, as a library should provide much
flexibility.

> 
> Please look carefully at any new function that is "void".
> Each should probably should have a return type by which
> to indicate success or failure.
> 
> Looking at ped_history_add, I see this:
> 
>     +    addme->name = ped_malloc (name_length);
>     +    strncpy (addme->name, name, name_length);
> 
> if ped_malloc fails, it returns NULL.
> If unchecked, strncpy will dereference that NULL,
> with the likely result of a segfault.
> 
> Similarly for this:
> 
>     +    addme = ped_history_alloc_obj ();
>     +    addme->id = PedHistMgr.id++;
> 
> Please audit your patch for any similar problems.

It has been a while since I have dabbled with this patchset beyond re-baseing
it.  I will try to get it done this weekend.  :-)

Thanks again!

-Matt