[parted-devel] History patchset

[Matt Davis]

On Feb 8, 2008 9:01 AM, Jim Meyering <jim at meyering.net> 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.
>
> 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.
>
> 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.

Here is my most recent implementation.  If this is not cool please let me know.
This is only implemented for the Linux architecture.  Adding the
initialization for the PedHistoryManager for other architectures
should be similar.  But I have not added them at this time, as I am
waiting to make sure this patchset is appropriate :-)

Thanks!

-Matt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Enables-undo-redo-of-disc-modifications-to-parted-co.patch
Type: application/octet-stream
Size: 15873 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20080211/392e49c7/attachment-0003.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Adds-the-history-undo-redo-functionality-to-libparte.patch
Type: application/octet-stream
Size: 16860 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20080211/392e49c7/attachment-0004.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Adds-test-script-for-the-history-undo-redo-parted-fu.patch
Type: application/octet-stream
Size: 5154 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20080211/392e49c7/attachment-0005.obj