Blkid output using the cache missing from issue (1) test case details.

    # blkid /dev/sda12
    /dev/sda12: UUID="8C80-7618" TYPE="vfat"

Hi Curtis,

While working on this fix I have encountered some unusual behaviour with
parted/libparted and I wondered if you could confirm my findings to make
sure I'm not doing something wrong.  It seems that parted/libparted are
remembering file system types even after the partition has been wiped.
At the moment it's making me unsure how to proceed with this patch.

Thanks,
Mike

Test case:
1) Create a new partition with file system using GParted.  Use a file
system which all versions of libparted support: ext2/3, jfs, xfs.
2) Use wipefs to erase the signature.  Parted and GParted still report
the file system, although corrupted.
3) Fill the partition with zeros.  Parted and GParted still report the
file system, although corrupted.

Results:
1) One XFS file system created.
    # blkid /dev/sda12
    /dev/sda12: UUID="af6a8447-949b-4394-94d3-1960c4e138ba" TYPE="xfs"
    # wipefs /dev/sda12
    offset               type
    ----------------------------------------------------------------
    0x0                  xfs   [filesystem]
                         UUID:  af6a8447-949b-4394-94d3-1960c4e138ba
    # parted /dev/sda print
    Model: ATA SAMSUNG SSD 830 (scsi)
    Disk /dev/sda: 128GB
    Sector size (logical/physical): 512B/512B
    Partition Table: msdos
    Disk Flags:

    Number  Start   End     Size    Type      File system     Flags
     1      1049kB  525MB   524MB   primary   ext4            boot
     2      525MB   2673MB  2147MB  primary   linux-swap(v1)
     3      2673MB  13.4GB  10.7GB  primary   ext4
     4      13.4GB  121GB   107GB   extended
     5      13.4GB  77.8GB  64.4GB  logical   ext3
     6      77.8GB  78.4GB  524MB   logical   ext4
     7      78.4GB  80.5GB  2147MB  logical   linux-swap(v1)
     8      80.5GB  91.2GB  10.7GB  logical   ext4
     9      91.2GB  91.8GB  524MB   logical   ext4
    10      91.8GB  93.9GB  2147MB  logical   linux-swap(v1)
    11      93.9GB  105GB   10.7GB  logical   ext4
    12      105GB   106GB   1074MB  logical   xfs

2) Wipe file system results.  Not recognised by blkid, but still
recognised by parted and GParted.  GParted reports a warnings from
XFS tools saying "/dev/sda12 is not a valid XFS filesystem (unexpected
SB magic number 0x00000000)".
    # wipefs -a /dev/sda12
    4 bytes were erased at offset 0x00000000 (xfs): 58 46 53 42
    # blkid /dev/sda12
    # parted /dev/sda print
    Model: ATA SAMSUNG SSD 830 (scsi)
    Disk /dev/sda: 128GB
    Sector size (logical/physical): 512B/512B
    Partition Table: msdos
    Disk Flags:

    Number  Start   End     Size    Type      File system     Flags
     1      1049kB  525MB   524MB   primary   ext4            boot
     2      525MB   2673MB  2147MB  primary   linux-swap(v1)
     3      2673MB  13.4GB  10.7GB  primary   ext4
     4      13.4GB  121GB   107GB   extended
     5      13.4GB  77.8GB  64.4GB  logical   ext3
     6      77.8GB  78.4GB  524MB   logical   ext4
     7      78.4GB  80.5GB  2147MB  logical   linux-swap(v1)
     8      80.5GB  91.2GB  10.7GB  logical   ext4
     9      91.2GB  91.8GB  524MB   logical   ext4
    10      91.8GB  93.9GB  2147MB  logical   linux-swap(v1)
    11      93.9GB  105GB   10.7GB  logical   ext4
    12      105GB   106GB   1074MB  logical   xfs
    # xfs_db -r -c label /dev/sda12
    xfs_db: /dev/sda12 is not a valid XFS filesystem (unexpected SB magic number 0x00000000)

3) File whole partition with zeros.  Parted still recognises it.
    # dd if=/dev/zero of=/dev/sda12 bs=1M
    dd: writing `/dev/sda12': No space left on device
    1025+0 records in
    1024+0 records oud /dev/sda12
    1073741824 bytes (1.1 GB) copied, 9.45353 s, 114 MB/s
    # od -Ax -tx1z /dev/sda12
    000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  >................<
    *
    # parted /dev/sda print
    Model: ATA SAMSUNG SSD 830 (scsi)
    Disk /dev/sda: 128GB
    Sector size (logical/physical): 512B/512B
    Partition Table: msdos
    Disk Flags:

    Number  Start   End     Size    Type      File system     Flags
     1      1049kB  525MB   524MB   primary   ext4            boot
     2      525MB   2673MB  2147MB  primary   linux-swap(v1)
     3      2673MB  13.4GB  10.7GB  primary   ext4
     4      13.4GB  121GB   107GB   extended
     5      13.4GB  77.8GB  64.4GB  logical   ext3
     6      77.8GB  78.4GB  524MB   logical   ext4
     7      78.4GB  80.5GB  2147MB  logical   linux-swap(v1)
     8      80.5GB  91.2GB  10.7GB  logical   ext4
     9      91.2GB  91.8GB  524MB   logical   ext4
    10      91.8GB  93.9GB  2147MB  logical   linux-swap(v1)
    11      93.9GB  105GB   10.7GB  logical   ext4
    12      105GB   106GB   1074MB  logical   xfs

Hi Mike,

Interesting.  I wonder if you've found a bug in one of the versions of either parted or wipefs.

What version of parted/libparted are you using?

What version of wipefs (from util-linux) are you using?

Since parted-2.x includes code for wiping file system signatures, I tried using parted-3.1 on Ubuntu 12.10.  With this setup the "wipefs -a /path-to-partition" command seems to completely erase the file system signatures.

My output is as follows:

1)  Create 1024 MiB partition formatted with XFS on /dev/sdb using GParted.


2)  Confirm that blkid and parted recognize XFS.

root@quantal:~# blkid /dev/sdb1
/dev/sdb1: UUID="4fb6849f-3789-46cb-aab7-801f0f5e92b1" TYPE="xfs" 
root@quantal:~# parted /dev/sdb print
Model: VMware, VMware Virtual S (scsi)
Disk /dev/sdb: 2147MB
Sector size (logical/physical): 512B/512B
Partition Table: msdos
Disk Flags: 

Number  Start   End     Size    Type     File system  Flags
 1      1049kB  1075MB  1074MB  primary  xfs

root@quantal:~# 


3)  Remove the file system signatures using "wipefs -a /path-to-partition":

root@quantal:~# wipefs -a /dev/sdb1
4 bytes were erased at offset 0x0 (xfs)
they were: 58 46 53 42
root@quantal:~# 


4)  Confirm that blkid and parted _do not_ recognize XFS.

root@quantal:~# blkid /dev/sdb1
root@quantal:~# parted /dev/sdb print
Model: VMware, VMware Virtual S (scsi)
Disk /dev/sdb: 2147MB
Sector size (logical/physical): 512B/512B
Partition Table: msdos
Disk Flags: 

Number  Start   End     Size    Type     File system  Flags
 1      1049kB  1075MB  1074MB  primary

root@quantal:~# 


5)  Show the version of the wipefs and parted commands:

root@quantal:~# wipefs --version
wipefs from util-linux 2.20.1
root@quantal:~# parted --version
parted (GNU parted) 3.1
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by <http://git.debian.org/?p=parted/parted.git;a=blob_plain;f=AUTHORS>.
root@quantal:~# 


Hopefully this output will help to isolate where the problem originates.  If you need me to do any more testing then please do not hesitate to ask.

Cheers,
Curtis

Hi Curtis,

I got my results in comment #2 with:
1) Physical    Fedora 14         parted 2.3  util-linux 2.20.1    Broken
2) Physical    Fedora 17         parted 3.0  util-linux 2.21.2    Broken
3) Physical    Ubuntu 10.04 LTS  parted 2.2  util-linux 2.17.2    Broken

Your result from comment #3:
4) VMware      Ubuntu 12.10      parted 3.1  util-linux 2.20.1    Works

Further testing:
5) Physical    Fedora 17         parted 3.1  util-linux 2.12.2    Broken
6) VirtualBox  Debian 6.0        parted 2.3  util-linux 2.17.2    Works

(5) I installed parted 3.1 (from Fedora 18 beta) on my Fedora 17 netbook
and got the same results.

I don't think that it can be a problem with wipefs.  Checking the
partition contents shows it wiped exactly the bytes it said it was going
to.  (Details below).

Would you be able to test a couple more combinations for me please.
Ubuntu 10.04 LTS and Fedora any version.  (I think I have seen you
mention these before).  Can you also say whether they are virtualised or
not, just in case.

Thanks,
Mike

--
Confirming wipefs works.  (Fedora 17, util-linux 2.21.2).

    # dd if=/dev/zero of=/dev/sda12 bs=1M
    # mkfs.xfs /dev/sda12
    # od -Ax -tx1z /dev/sda12 > before
    # wipefs /dev/sda12
    offset               type
    ----------------------------------------------------------------
    0x0                  xfs   [filesystem]
                         UUID:  1d3d7111-86a7-428e-96a0-cfb705a87b00
    # wipefs -a /dev/sda12
    4 bytes were erased at offset 0x00000000 (xfs): 58 46 53 42
    # od -Ax -tx1z /dev/sda12 > after
    # diff before after
    1c1
    < 000000 58 46 53 42 00 00 10 00 00 00 00 00 00 04 00 00  >XFSB............<
    ---
    > 000000 00 00 00 00 00 00 10 00 00 00 00 00 00 04 00 00  >................<

Hi Mike,

(In reply to comment #4)
> Would you be able to test a couple more combinations for me please.
> Ubuntu 10.04 LTS and Fedora any version.

'Sure can do.  I have several virtual machines already built for testing.

Both XFS and EXT file systems were tested with the following setups:

7)  VMWare    Fedora 13       parted 2.1  util-linux 2.17.2    Broken *
8)  VMWare    Fedora 17       parted 3.0  util-linux 2.21.2    Broken
9)  VMWare    Ubuntu 10.04    parted 2.3  util-linux 2.17.2    Broken
10) Physical  Kubuntu 12.04   parted 2.3  util-linux 2.20.1    Works
11) VMWare    Ubuntu 12.04**  parted 2.3  util-linux 2.20.1    Broken
12) VMWare    Ubuntu 12.04*** parted 2.3  util-linux 2.20.1    Broken
13) VMWare    Kubuntu 12.04   parted 2.3  util-linux 2.20.1    Works

*   wipefs -a did not erase any bytes for XFS.  Both parted and blkid broken.
    wipefs -a did erase 2 bytes for EXT2.  Both parted and blkid worked.

    The result with Fedora 13 was unusual.  As such I retested it, but
    the same results occurred.  Strange....

**  Ubuntu 12.04 unpatched.

*** Ubuntu 12.04 patched.

The discrepancy between kubuntu 12.04 (up-to-date) and Ubuntu 12.04
(unpatched) led me to investigate further by applying patches to
Ubuntu 12.04.  With patches applied, Ubuntu 12.04 is still broken.

These results seemed unusual.  My thought was that perhaps the results
depended upon whether a physical or virtual machine was used.
However, when I tried using kubuntu 12.04 on a virtual machine it
worked properly.  These discrepancies have me puzzled too.

Currently seeking help via the parted development mailing list.  Thread starts here:

  [parted-devel] parted reporting old file system names
  http://lists.alioth.debian.org/pipermail/parted-devel/2012-December/004289.html

For the record we worked out what was happening shortly afterwards.  The
Linux kernel was returning old data when parted reads via the whole disk
device after a new file system was created on a partition.

More details in this post:
http://lists.alioth.debian.org/pipermail/parted-devel/2012-December/004296.html

Worked around by calling ped_device_sync() to provide cache coherency.
Patch set development continuing well.

Created attachment 237647 [details] [review]
File system signature erasure (draft 1)

Hi Curtis,


Do you have any techniques you use to trigger errors in libparted during
testing?  Particularly that would affect any of these function:
  ped_device_open(), ped_geometry_write(), ped_device_sync()

I'm trying to test the error handling in my erase file system signature
code but there is only so far you can go by changing the code to pretend
a function returned an error.  If not I'll try reading parted's test
cases and source code.


Patchset status:
* Code is feature complete, but error handling may change a little.
* Documentation is outstanding, but something will need writing to
  explain:
** what formatting as "cleared" does
** recovering from accidental partition deletion by re-creating the
   partition


Don't do any reviewing yet, but if your interested in a quick test the
patchset is attached or you can get it from my repo in the usual manner,
branch erase-draft1.
    git clone https://rockover.homeip.net/cgit/gparted gparted-erase-d1
    cd gparted-erase-d1
    git checkout erase-draft1


Thanks,
Mike

Hi Mike,

(In reply to comment #8)
> Do you have any techniques you use to trigger errors in libparted during
> testing?  Particularly that would affect any of these function:
>   ped_device_open(), ped_geometry_write(), ped_device_sync()

Some things that I can think of are the following:

a)  Pass an invalid device path -- ped_device_open() should fail.

b)  Use a read-only disk device such as an SD card with the write-protect
    switch enabled -- ped_geometry_write() should fail.

    Another idea would be to create a loop device and alter the
    permissions so that even root does not have write access.

c)  I'm not sure how to test ped_device_sync().


Phillip,

Do you have some testing ideas from your time working with the parted code?

Thanks that worked.  Most cases cause libparted to throw exceptions
which GParted shows to the user and records in the operation details.

Examples being applied at different points in the code:

//#1 Pass unknown path
ped_device_get( "/dev/sdX" );

//#2 Pass invalid sector
ped_disk_get_partition_by_sector( lp_disk, -1LL );

//#3 Overwrite with unknown path after ped_device_get()
memset( lp_device->path, "/dev/sdX", 9 );
ped_device_open( lp_device );

//#4 Overwrite with invalid geom after ped_disk_get_partition_by_sector()
lp_partition->geom.start  = -1LL;
lp_partition->geom.end    = -1LL;
lp_partition->geom.length = 0LL;
ped_geometry_write( & lp_partition ->geom, ... );

//#5 Overwrite with invalid file descriptor after ped_device_open()
typedef struct _LinuxSpecific LinuxSpecific;
struct _LinuxSpecific {
	int fd;
	int major;
	int minor;
	char* dmtype;
};
#define LINUX_SPECIFIC(dev) ((LinuxSpecific*) (dev)->arch_specific)
LINUX_SPECIFIC(lp_device)->fd=876;
ped_device_sync( lp_device );

Created attachment 237942 [details] [review]
File system signature erasure (v1)

Hi Curtis,

Here's the patchset ready for review.  Applies on top of the current
master:
    Updated German translation
http://git.gnome.org/browse/gparted/commit/?id=ff1612cce54a21679bf6d77671a6d7af71c11f70

Let me know if by the time you review this it doesn't apply cleanly and
I've update it.  Also available from my repository as branch erase-v1:
    git clone https://rockover.homeip.net/cgit/gparted gparted-erase-v1
    cd gparted-erase-v1
    git checkout erase-v1

Turns out that I had to code around a few limitations with the wipefs
command.  (See all the patches changing erase_filesystem_signatures()).
I'm tempted to remove wipefs altogether.  Will do if you want?

Also minimally updated the help documentation.  There should probably
be a "Recovering a Deleted Partition" sub-section some where, depending
on what you think.  It might just say re-create the partition using
unformatted file system, refer to 5.2.1 "Create a New Partition" and
5.2.6.2 "Specifying Partition File System".

Test on Fedora 14 & 17, Debian 6.  Tested with wipefs, without wipefs
and erroring wipefs.  Tested with techniques from comment #10 above
inserted into erase_filesystem_signatures() to break it.  GParted
displays error dialogs with libparted error messages and includes them
in the operation details.

Thanks,
Mike

Created attachment 239340 [details] [review]
File system signature erasure (v2)

Hi Curtis,

Rebased this patch to apply cleanly on top of the current master,
commit 5b737b224de9f868ecf91a54994b35b0c78abebc.

The only change was to update the numbering of the FILESYSTEM
enumeration in Utils.h after Patrick's patched added FS_F2FS and this
patch set adds FS_CLEARED.

Thanks,
Mike

Hi Mike,

Thanks for your patience.

Wow!  I would never have guessed that there would be so much work involved to ensure that file system signatures were cleared.  You've been busy!

(In reply to comment #11)
> Turns out that I had to code around a few limitations with the wipefs
> command.  (See all the patches changing erase_filesystem_signatures()).
> I'm tempted to remove wipefs altogether.  Will do if you want?

I agree with you regarding removing wipefs altogether.  Having to run wipefs 3 times to remove VFAT signatures is ridiculous.  Also in cases where wipefs does not exist, or does not remove all the signatures, then we would have to handle the signature erasure ourselves anyway.  I think the GParted code will be cleaner and easier to understand if we remove wipefs and all it's strangeness.

On the other hand, one possible advantage to keeping wipefs is that in the future it might clear a signature for a file system that GParted does not yet clear.  However, the added complexity for wipefs seems unjustified for this potential reason for keeping wipefs.


> Also minimally updated the help documentation.  There should probably
> be a "Recovering a Deleted Partition" sub-section some where, depending
> on what you think.  It might just say re-create the partition using
> unformatted file system, refer to 5.2.1 "Create a New Partition" and
> 5.2.6.2 "Specifying Partition File System".

Unfortunately there is a potential mismatch between the MSDOS partition type (as seen in fdisk) and the actual file system if a person tries to create an unformatted partition in an attempt to recover a file system.  An unformatted partition is created as type 83 which works for many GNU/Linux file systems, but not for file systems such as NTFS, and VFAT.  As such I do not think we should mention "Recovering a Deleted Partition" in this way.

Another rescue option is available in parted 2.x.  However with support for file systems removed in 3.x, I think it best to avoid this rescue option as well.
https://www.gnu.org/software/parted/manual/html_chapter/parted_2.html#SEC24

For now, I think the best recovery option for a deleted partition is to use testdisk.

Created attachment 239632 [details] [review]
File system signature erasure (v3)

Hi Curtis,

Changes from "File system signature erasure (v2)" in comment #12:

1)  Added patch which, as per your recommendation, does:
    Remove use of wipefs to clear file system signatures (#688882)

2)  Updated this patch so that file system names are marked up as GUI
    menu items:
    Add "cleared" file system format details to the GParted Manual (#688882)

3)  Added another tidy-up patch:
    Remove unused function copy_filesystem_simulation()


(In reply to comment #13)
> I agree with you regarding removing wipefs altogether.  Having to run wipefs 3
> times to remove VFAT signatures is ridiculous.  Also in cases where wipefs does
> not exist, or does not remove all the signatures, then we would have to handle
> the signature erasure ourselves anyway.  I think the GParted code will be
> cleaner and easier to understand if we remove wipefs and all it's strangeness.

Wipefs removed.
Keeping all the small commits as it documents understanding of wipefs
and reasoning for the code being the way it is which helps review.  Also
when git bisecting, finding a fault in a smaller commit is usually
easier.


> On the other hand, one possible advantage to keeping wipefs is that in the
> future it might clear a signature for a file system that GParted does not yet
> clear.  However, the added complexity for wipefs seems unjustified for this
> potential reason for keeping wipefs.

Writing zeros over the first 68K will cope with clearing most future
file system signatures.  It handles the recently added F2FS (signature
at offset 1024 bytes).


> (In reply to comment #11)
> > Also minimally updated the help documentation.  There should probably
> > be a "Recovering a Deleted Partition" sub-section some where, depending
> > on what you think.  It might just say re-create the partition using
> > unformatted file system, refer to 5.2.1 "Create a New Partition" and
> > 5.2.6.2 "Specifying Partition File System".
> 
> Unfortunately there is a potential mismatch between the MSDOS partition type
> (as seen in fdisk) and the actual file system if a person tries to create an
> unformatted partition in an attempt to recover a file system.  An unformatted
> partition is created as type 83 which works for many GNU/Linux file systems,
> but not for file systems such as NTFS, and VFAT.

Good catch.  I hadn't though about that.


> ...  As such I do not think we
> should mention "Recovering a Deleted Partition" in this way.

Agreed.  The manual still says this though to describe unformatted:

    unformatted can be used when re-creating a partition to recover the
    previous contents.

To dig a hole for myself labelled more coding required; should GParted
even have unformatted any more now that it has cleared.  From a less
technical user's point of view choosing between cleared and unformatted
may not be obvious.  Again from a less technical user point of view why
would they need either.  To quickly fill in that hole; I think that the
descriptions for cleared and unallocated in this patch set are clear.
Also use of unallocated to recover a deleted partition can become a
technique we keep under our hats just in case some one needs it.  For
fat file systems they would have to use fdisk or similar to reset the
partition type too.


> For now, I think the best recovery option for a deleted partition is to use
> testdisk.

There's also gpart which is built in GParted but it doesn't handle
logical partitions.


Thanks,
Mike

Hi Mike,

Thank you for the updated patch, for finding and removing some unused code, and making a good point about gpart for recovery.  :-)

Overall the patch set looks pretty good.  Following are some comments on the patch set that I think are worth considering.

a)  In patch 11/15 I am still uncomfortable with stating in the manual
    that creating an unformatted partition is useful for re-creating
    a partition to recover it's contents.  Currently it says:

    "<guimenuitem>unformatted</guimenuitem> can be used
     when re-creating a partition to recover the previous
     contents."

    This can work for primary partitions, if and only if the starting sector
    exactly matches the start of the previously deleted partition.  The end
    sector is not as critical and can be larger than the end of the previous
    partition.

    However, with logical partitions, the action of deleting the partition
    and later recreating the partition can result in the Extended Boot
    Record being written to different locations.  The fact that the EBR
    can move means that a portion of the previous partition contents _can_
    be overwritten by the EBR.

    I first encountered this problem with the EBR location changing when
    debugging a problem with moving logical partitions.  That is why the
    move code now makes an all-encompassing partition first, and then moves
    the partition contents within the all-encompassing partition.  At the
    end of the move the partition boundaries are shrunk to the new
    partition location.  This problem is most noticable when changing
    from cylinder to MiB alignment.

    The way I see it, the real use for creating an unformatted
    partition is so that a user can then format it with a new "type 83"
    file system that GParted does not currently support.  With
    GUID Partition tables, I think we default to MSFT DATA.


b)  In patch 11/15 I think that the description of "clear" is
    misleading.  Currently it says:

    "<guimenuitem>cleared</guimenuitem> can be used to
     clear any existing file system signatures and ensure
     that the partition is empty."

    To me, this implies that no data can be recovered from the partition.
    However, only the file system signatures have been zeroed.  The
    content of some data files is still available and can be recovered
    using tools like photorec.

    My thoughts are that with clear we need to expressly state that only
    the file system signatures are zeroed.  Either that, or perhaps
    we should go all the way and implement a format type of "zero" or
    "clear" to zero out the entire partition.  This would likely be
    a solution to the following report:

    Bug #619348 - GParted should have option to shred (secure partition
                  deletion)


c)  In patch 13/15 I do not think there is any harm in setting pointers to
    NULL upon creation.  In fact by doing so it removes the chance of
    dereferencing a pointer that has not been set and thus avoids
    segmentation faults that can occur if this happens.  For example,
    referencing memory outside of that owned by the process.

    With this in mind, I think that patch 13/15 which removes the setting
    of pointers to NULL upon creation is not adding value.

    If I am missing something here, then please let me know.  I am thinking
    back to my early teachings which instructed to always initialize
    memory pointers and variables.


d)  Minor clarification about gpart and GParted.

    The gpart application can find up to 4 partitions whether these are
    primary or logical.  I have tested this with a single cylinder
    aligned logical ntfs partition.  Where gpart breaks down is that
    it can only restore up to 4 partition entries in the primary/extended
    slots in the MBR.

    With GParted, we use gpart to find up to 4 partitions, but instead of
    restoring partitions to the partition table, we instead use the
    partition boundaries from gpart to temporary mount the file system.
    That way a user can copy data from the temporary mount to another
    location.  This works with both primary and logical partitions.


Please feel free to indicate if you disagree with my view point.

Regards,
Curtis

Hi Curtis,

For a) and b) is the following wording OK?

unformatted can be used to just create a partition without writing a
file system.

cleared can be used to clear any existing file system signatures and
ensure that the partition is recognised as empty.


(In reply to comment #15)
> b)  ...
>     My thoughts are that with clear we need to expressly state that only
>     the file system signatures are zeroed.  Either that, or perhaps
>     we should go all the way and implement a format type of "zero" or
>     "clear" to zero out the entire partition.  This would likely be
>     a solution to the following report:
>
>     Bug #619348 - GParted should have option to shred (secure partition
>                   deletion)

Cleared does explicitly state that the file system signatures are
cleared.  Added "... recognised as ..." this time.

Zeroing the whole partition is potentially a very long operation (likely
take multiple hours for a 1 TiB partition) where as just "cleared"
format takes less than 1 second.  I think that zeroing a whole partition
or disk should be separate to just clearing file system signatures.
(Possibly a separate operation in the partition menu).


c) [13/15] not setting lp_device and lp_disk to NULL

The pointers lp_device and lp_disk are always passed to the function
get_device_and_disk() to be updated.  Now the function also ensures that
both pointers are always assigned a valid reference.  They will either
be set to NULL or a valid pointer returned by ped_device_get() and
ped_disk_new().  Therefore there is no chance that an undefined pointer
is dereferenced.

Thanks,
Mike

Hi Mike,

(In reply to comment #16)
> For a) and b) is the following wording OK?
> 
> unformatted can be used to just create a partition without writing a
> file system.
> 
> cleared can be used to clear any existing file system signatures and
> ensure that the partition is recognised as empty.

Perfect!  I like this wording much better.  :-)


> (In reply to comment #15)
> > b)  ...
> >     My thoughts are that with clear we need to expressly state that only
> >     the file system signatures are zeroed.  Either that, or perhaps
> >     we should go all the way and implement a format type of "zero" or
> >     "clear" to zero out the entire partition.  This would likely be
> >     a solution to the following report:
> >
> >     Bug #619348 - GParted should have option to shred (secure partition
> >                   deletion)
> 
> Cleared does explicitly state that the file system signatures are
> cleared.  Added "... recognised as ..." this time.
> 
> Zeroing the whole partition is potentially a very long operation (likely
> take multiple hours for a 1 TiB partition) where as just "cleared"
> format takes less than 1 second.  I think that zeroing a whole partition
> or disk should be separate to just clearing file system signatures.
> (Possibly a separate operation in the partition menu).

Agreed.  A separate operating for shredding or zeroing an entire partition is probably the better direction to take.


> c) [13/15] not setting lp_device and lp_disk to NULL
> 
> The pointers lp_device and lp_disk are always passed to the function
> get_device_and_disk() to be updated.  Now the function also ensures that
> both pointers are always assigned a valid reference.  They will either
> be set to NULL or a valid pointer returned by ped_device_get() and
> ped_disk_new().  Therefore there is no chance that an undefined pointer
> is dereferenced.

My apologies for any confusion I have caused.  The code is not wrong by any definition.

I did not mean to indicate that the ged_device_and_disk() function would be the problem.  I meant to say that using the pointers between when they are declared, and when they are set is the potential area for a segmentation fault.  If the pointer is set to NULL at declaration time, then this potential problem is avoided.

Currently in the patch the pointers are declared, and then the get_device_and_disk() function is called which will initialize the pointers.  Over time changes to the code can move this declaration and function call further apart, and at some point in the future someone might try to dereference the unset pointers.

Overall this is a minor point and I am probably getting to be too picky.  ;-)

Created attachment 239701 [details] [review]
File system signature erasure (v4)

Hi Curtis,

Changes from previous v3 patch set in comment #14 are:

1)  Update wording for the Manual as agreed in patch:
    Add "cleared" file system format details to the GParted Manual

2)  Dropped tidy-up patch:
    Further refactor GParted_Core::get_device_and_disk()

Thanks,
Mike

Hi Mike,

Thanks for these last minor changes, my testing has gone well and the code looks good to me.  As such the patch set contained in comment #18 has been committed to the git repository for inclusion in the next release of GParted.

The relevant git commits can be viewed at the following links:

Use wipefs to clear old signatures before creating new file systems (#688882)
https://git.gnome.org/browse/gparted/commit/?id=3c75f3f5b103bc158b0f70ea63459aa362b0bab8

Refactor Win_GParted::create_format_menu() (#688882)
https://git.gnome.org/browse/gparted/commit/?id=bc5b57ab35fdad3cde221e2d9725cc61681d78ef

Add new "cleared" file system format (#688882)
https://git.gnome.org/browse/gparted/commit/?id=d4f68eb730ae2d2046c23b733c8ff44ceb7c9f0a

Flush device after wiping a file system (#688882)
https://git.gnome.org/browse/gparted/commit/?id=797f0b8eeb61f81da1791c3cf048b5c0abf9a550

Workaround not so old wipefs only erasing 1 of 3 vfat signatures (#688882)
https://git.gnome.org/browse/gparted/commit/?id=6982f68e21b5c1cfdd0c4f61ace83772152ad5d2

Implement fallback if wipefs is not available or fails (#688882)
https://git.gnome.org/browse/gparted/commit/?id=7a75148a7b8f455c27a584ddc277823be0a90659

Display failure from wipefs as an error not a warning (#688882)
https://git.gnome.org/browse/gparted/commit/?id=b2b51ad424b5b76c9bf25aa80d54e3ad21dab762

Make flush OS cache a reported step (#688882)
https://git.gnome.org/browse/gparted/commit/?id=0b164b168d28b63fb8dc6fcee4e16b1b6b2552c4

Clear nilfs2 secondary super block (#688882)
https://git.gnome.org/browse/gparted/commit/?id=38ca0db3434653c5890d2bec21ad8cd4ddd4bfc2

Remove use of wipefs to clear file system signatures (#688882)
https://git.gnome.org/browse/gparted/commit/?id=2b7e4694737230fde0c9b841e03e0f3eb3699285

Add "cleared" file system format details to the GParted Manual (#688882)
https://git.gnome.org/browse/gparted/commit/?id=17a349e28bf4974a9a8664a069034d47ba0d4633

Refactor and rename GParted_Core::open/close_device_and_disk()
https://git.gnome.org/browse/gparted/commit/?id=e218ba3358eac27447b67dea6792987a30ac0a19

Don't hard code reiser4 max label length when reading the label
https://git.gnome.org/browse/gparted/commit/?id=a39079211b9726dffe727572f47cb7a5ce7d48b6

Remove unused function copy_filesystem_simulation()
https://git.gnome.org/browse/gparted/commit/?id=6c33a8f5ca5448461e9d698d3e5fa13c8da46449

The code change to address this report has been included in GParted 0.16.0 released on April 24, 2013.