After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 743181 - Add unpartitioned drive read-write support
Add unpartitioned drive read-write support
Status: RESOLVED FIXED
Product: gparted
Classification: Other
Component: application
GIT HEAD
Other Linux
: Normal normal
: ---
Assigned To: gparted maintainers alias
gparted maintainers alias
: 683643 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-01-19 13:41 UTC by Mike Fleetwood
Modified: 2015-03-23 17:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
read-write whole disk device support (draft 1) (65.58 KB, patch)
2015-01-21 14:21 UTC, Mike Fleetwood
none Details | Review
read-write whole disk device support (draft 2) (92.17 KB, patch)
2015-01-27 21:57 UTC, Mike Fleetwood
none Details | Review
read-write whole disk device support (draft 3) (100.67 KB, patch)
2015-02-06 20:57 UTC, Mike Fleetwood
none Details | Review
read-write whole disk device support (v1) (63.10 KB, patch)
2015-02-15 14:36 UTC, Mike Fleetwood
none Details | Review
Correct format cleared preview (draft 1) (4.01 KB, patch)
2015-02-19 09:46 UTC, Mike Fleetwood
none Details | Review
read-write whole disk device support (v2) (74.90 KB, patch)
2015-02-19 17:17 UTC, Mike Fleetwood
none Details | Review
read-write whole disk device support (v3) (88.64 KB, patch)
2015-02-28 16:47 UTC, Curtis Gedak
none Details | Review
read-write whole disk device support (v4) (92.10 KB, patch)
2015-03-08 14:20 UTC, Mike Fleetwood
none Details | Review
Unpartitioned disk R/W support help manual update (v1) (2.82 KB, patch)
2015-03-09 21:34 UTC, Curtis Gedak
none Details | Review
Unpartitioned disk R/W support help manual update (v2) (3.68 KB, patch)
2015-03-10 15:54 UTC, Curtis Gedak
none Details | Review

Description Mike Fleetwood 2015-01-19 13:41:26 UTC
Separate bug number to track adding read-write support of unpartitioned
drives.  The code change and therefore review process will be big enough
it will be best separated from read-only support.

Read-only support of unpartitioned drives is being / was added under
bug 741430.
Comment 1 Mike Fleetwood 2015-01-21 14:21:32 UTC
Created attachment 295104 [details] [review]
read-write whole disk device support (draft 1)

Hi Curtis,

Here's the first draft of read-write whole disk device support.

It shows the approach I have taken for the UI, making it fit with the
current way of working with the partition and file system conflation.
Therefore:
1) Uses the existing partition menu.
2) Create a new whole disk device on an empty unrecognised drive.
   New dialog to support this.  Analogous to, but simpler than, the new
   partition dialog.
3) Can format existing whole disk file systems.
4) Delete whole disk file system simple erases the signatures.
   Gets back to an unrecognised drive.
5) Resize/move isn't implemented.
   This is not easily to explain on a whole disk.
6) Other operations are as expected.
7) No partition flags.

When you have time, play with GParted and see what you think.

There are still a few bits I know are missing:
* Graphic of a disk in the new create whole disk file system dialog.
* Paste into whole disk device hasn't been done yet.
* There's should be no partition overhead creating or pasting a new
  whole disk file system.
  Ref: Win_GParted::set_valid_operations() if TYPE_UNALLOCATED
* Need to check erase signatures clears old partition tables
  (GPT backup).
* No documentation updates.
* Anything else I've forgotten.

No need to review any code at this time.  Especially any commits tagged
with WIP (Work In Progress).

You'll need to apply the latest patchsets from the following bugs, in
order:
  bug 741430 - read-only whole disk support
  bug 742741 - RHEL6 support of nilfs2       (optional)
  bug 743181 - read-write whole disk support (this bug)
  
Thanks,
Mike
Comment 2 Mike Fleetwood 2015-01-27 21:55:33 UTC
Hi Curtis,

Here's the second draft of read-write whole disk device support.

Compared to draft 1 it also does:
* Working graphic of a disk in the new create whole device file system
  dialog.
* Working paste into whole disk device.
* Forces creation of required file systems to succeed on whole disk
  devices where necessary.

This makes the UI feature complete.  Still need to tweak the layout of
the widgets a little though.

There's even more WIP (Work In Progress) code and FIXMEs so don't review
it.  Just play with it so that you can feel how it works.

Thanks,
Mike
Comment 3 Mike Fleetwood 2015-01-27 21:57:04 UTC
Created attachment 295600 [details] [review]
read-write whole disk device support (draft 2)

Whoops.  This time with patchset draft 2.
Comment 4 Curtis Gedak 2015-01-29 17:42:04 UTC
Hi Mike,

This is just a heads up that I am looking into the three bug reports listed at the end of comment #1 (includes this bug).

Since you are working on the code improvements, I offer to do the help manual updates once we've arrived at a solution.

Curtis
Comment 5 Mike Fleetwood 2015-01-30 15:02:12 UTC
Hi Curtis,

Yes I would appreciate you updating the GParted Manual when we finalise
how GParted should work.

I think what I have coded is the right way of working with
parititionless drives for GParted.  However I'm not sure about the
warning messages in the dialogs.  Could get rid of the warnings.  Could
have an advanced mode toggle in the GParted menu to enable partitionless
support.  Or something else.

Thoughts welcome,
Mike
Comment 6 Curtis Gedak 2015-01-31 16:40:16 UTC
Hi Mike,

I'm working my way through these bug reports.  I just arrived at:

[11/23] WIP: Add create new whole device file system dialog


    Food for thought:

    Can we use the existing Create Partition Table dialog to wipe out
    the partitition table signatures resulting in a NONE partition
    table.  Then use the existing New Partition dialog to format a
    file system that spans the entire disk?

    This might save us the need to write and maintain dialogs for
    unpartitioned devices.


I'm continuing on with my review and will post more comments once I finish my first pass of the code.

Curtis
Comment 7 Curtis Gedak 2015-01-31 17:22:57 UTC
Hi Mike,

Following is a my review of the read-write whole disk device support
draft2 patch in comment #3.  Note that I plan to play around with the
new functionality more to get familiar with it.  Some UI changes are
big, such as instead of forcing creation of a partition table this
patch set now permits "New" formatting with caution.


[01/23] Correct dialog title displaying libparted exceptions

    I like the expanded message handling for libparted exceptions.


[03/23] Enable operations on whole disk device virtual partitions

    For initial read/write whole device support I agree that we should
    ignore the move and resize cases.

    I can think of one more case where resize would be useful for
    whole device support.  This is when the size of a RAID is grown
    and a user wishes to now grow the file system to use the entire
    RAID.

    In the commit message point [2] says the calibrate step is only
    needed for Check, Label, and New UUID.
    What about Copy/Paste?


[10/23] Make deletion work on whole disk device file systems

    This is a question on consistency.  In other areas the handling of
    partition.whole_device has been added directly into the original
    method, for example create() and format().  However in the case of
    Delete(), the method has been split into two new methods
    delete_op() and delete_partition().

    Why the difference?  The change is minor and for consistency might
    be better added to the original Delete() method.


[11/23] WIP: Add create new whole device file system dialog

    Food for thought:

    Can we use the existing Create Partition Table dialog to wipe out
    the partition table signatures resulting in a NONE partition
    table.  Then use the New Partition dialog to format a file system
    that spans the entire disk?

    UPDATE:  I'm beginning to see the wisdom of your approach of
             keeping whole device dialogs separate from partition
             device dialogs.  It keeps the logic simpler.


    Typo  "decices" should be "decides":
      <snip>This decices the order of the items<snip>


    I agree that we should scrutinize the warning text carefully.
    Rather than saying "for experts only", we should try to indicate
    why having a partition table is a good idea.  Then caution against
    formatting without a partition table.


    When calling a method and passing fixed values like "1", or
    "false", I think it is helpful to place a comment beside the value
    indicating what the value means.  For example:

         false /* Is device busy? */

    I know that we haven't done this throughout the code, so to check
    I have to lookup the method parameters.  Then again that isn't a
    bad thing to do....


    // FIXME: Why does the .sector_size get passed to Get_New_Partition()?

    .sector_size is needed to calculate the exact sector for MiB
    boundaries.  Ideally the graphical representation would be
    completely separate from the sector values, but this is not
    currently the case.  Instead the graphical representation
    determines the sector values.


[12/23] WIP: Make new work on whole unallocated disk device

    Typos "acheive" should be "achieve":
      <snip>doesn't acheive anything<snip>


[16/23] WIP: Make pasting into unallocated whole disk device work

    Typo "releated" should be "related"
      <snip>releated code.<snip>



[22/23] WIP: Add partition graphic - not fully working

    Typo "streatched" should be "stretched"
      <snip>is getting streatched wider than<snip>


Curtis
Comment 8 Mike Fleetwood 2015-02-01 12:10:09 UTC
Hi Curtis,

> ...  Some UI changes are
> big, such as instead of forcing creation of a partition table this
> patch set now permits "New" formatting with caution.

There are definitely pros and cons.  The UI is different to how the
other partition table types work, but then the reality of how file
systems on whole drives without a partition table is different.  I think
it is better to reflect that difference in the UI.

Also an unpartitioned drive looks virtually identical to a partitioned
drive with no partitions.  Previously GParted showed a error dialog
informing the user that there was no partition table when trying to
create a new partition on an unpartitioned drive.  Now we show a create
new whole device file system dialog with a similar warning.

I think having the warning message in the operation dialog is ugly, but
don't have a better way.  Perhaps enclosing in an etched frame may
reduce it's ugliness.

If instead users had to create a "none" partition table first, then
create a partition, we would have to write a "none" partition table
signature to the disk and recognise it later.  This is how libparted's
"loop" table works.  On creation of a "loop" table, GParted would then
show an empty partition filling the drive.  This then leads to using
format existing partition, rather then create new partition dialog.  To
try this use current GParted with libparted 3.2.


> ...  Note that I plan to play around with the
> new functionality more to get familiar with it.

Please do.


> [01/23] Correct dialog title displaying libparted exceptions
>
>   I like the expanded message handling for libparted exceptions.

Thanks.  When I first started trying to copy from whole disk devices I
was getting the libparted bug found, invalid argument during seek for
read.  What I found was that libparted was actually reporting an error
trying to read from sector -1, which lead to me discovering bug 742920
"GParted internal copy is overstepping partition boundaries".  Hence
needing to fix this too.


> [10/23] Make deletion work on whole disk device file systems
>
>     This is a question on consistency.  In other areas the handling of
>     partition.whole_device has been added directly into the original
>     method, for example create() and format().  However in the case of
>     Delete(), the method has been split into two new methods
>     delete_op() and delete_partition().
>
>     Why the difference?  The change is minor and for consistency might
>     be better added to the original Delete() method.

The existing create() and format() functions already only make choices
and call more functions such as create_partition(), set_partition_type()
and create_filesystem() etc to implement those changes.  Now delete_op()
does this too and calls delete_partition() when needed.  I don't see a
consistency issue here.


>   // FIXME: Why does the .sector_size get passed to Get_New_Partition()?
>
>   .sector_size is needed to calculate the exact sector for MiB
>   boundaries.  Ideally the graphical representation would be
>   completely separate from the sector values, but this is not
>   currently the case.  Instead the graphical representation
>   determines the sector values.

But the New Partition dialog and the resize/move widgets got the
sector_size from the selected_partition passed to .Set_Data().

Dialog_Partition_New.cc
35  void Dialog_Partition_New::Set_Data( const Partition & partition,
...
43          this ->selected_partition = partition;
...
143       START = partition.sector_start ;
144       total_length = partition.sector_end - partition.sector_start ;
145       TOTAL_MB = Utils::round( Utils::sector_to_unit( this ->selected_partition .get_sector_length(), this ->selected_partition .sector_size, UNIT_MIB ) ) ;
146       MB_PER_PIXEL = TOTAL_MB / 500.00 ;

And I don't see any reason why .Get_New_Partition() couldn't also do the
same by using this->selected_partition.sector_size rather than requiring
it to be passed as a parameter.  Perhaps there is an historical reason
why it was coded that way.  Git blame research required.


Other points accepted.


Thanks,
Mike
Comment 9 Curtis Gedak 2015-02-02 18:04:11 UTC
Hi Mike,

When you get a chance would you please rebase the patch set(s) against
the current master?

Following are responses to your comments:


> > ...  Some UI changes are
> > big, such as instead of forcing creation of a partition table this
> > patch set now permits "New" formatting with caution.
>
> There are definitely pros and cons.  The UI is different to how the
> other partition table types work, but then the reality of how file
> systems on whole drives without a partition table is different.  I
> think it is better to reflect that difference in the UI.
>
> Also an unpartitioned drive looks virtually identical to a
> partitioned drive with no partitions.  Previously GParted showed a
> error dialog informing the user that there was no partition table
> when trying to create a new partition on an unpartitioned drive.
> Now we show a create new whole device file system dialog with a
> similar warning.
>
> I think having the warning message in the operation dialog is ugly,
> but don't have a better way.  Perhaps enclosing in an etched frame
> may reduce it's ugliness.

I think we need to discourage the use of disk space without a
partition table.

The key benefit of a partition table is:

a)  Other operating systems and tools usually respect commonly used
    partition tables like MSDOS and GPT.  This can be considered an
    added layer of protection for data within a partition.

The main disadvantage of not using a partition table is:

b)  Other operating systems often do not recognize non-native file
    systems and will assume the disk device is unformatted or empty.
    Some might even prompt to partition the device or format using an
    OS native file system.  This leads to a higher likelihood of
    overwriting data.

I'm not yet sure how we can get this message across in a short caution
message so that non-expert users will understand the longer term
implications of not using a partition table.

> If instead users had to create a "none" partition table first, then
> create a partition, we would have to write a "none" partition table
> signature to the disk and recognise it later.  This is how
> libparted's "loop" table works.  On creation of a "loop" table,
> GParted would then show an empty partition filling the drive.  This
> then leads to using format existing partition, rather then create
> new partition dialog.  To try this use current GParted with
> libparted 3.2.

The extra step of creating a "loop" partition table has the advantage
that it already displays a warning about data loss.  This part I like
about it because it forces two steps to format an entire disk device:
(1) Create a new partition table "loop" or "none", and (2) Format the
"loop" or "none" device.

Unfortunately the "loop" functionality does not work across all
previous libparted versions, and is limited in recognizing file
systems.  :-(


> > [10/23] Make deletion work on whole disk device file systems
> >
> >     This is a question on consistency.  In other areas the
> >     handling of partition.whole_device has been added directly
> >     into the original method, for example create() and format().
> >     However in the case of Delete(), the method has been split
> >     into two new methods delete_op() and delete_partition().
> >
> >     Why the difference?  The change is minor and for consistency
> >     might be better added to the original Delete() method.
>
> The existing create() and format() functions already only make
> choices and call more functions such as create_partition(),
> set_partition_type() and create_filesystem() etc to implement those
> changes.  Now delete_op() does this too and calls delete_partition()
> when needed.  I don't see a consistency issue here.

I guess what I see as inconsistent is the introduction of delete_op()
which introduces the fragment "op".  We use the term operation in other
specific methods, such as OperationCreate, and OperationDelete.

Why not continue to call this method "Delete", or "delete" for
consistency with "create" and "format"?


> > // FIXME: Why does the .sector_size get passed to Get_New_Partition()?
> > <snip>
>
> But the New Partition dialog and the resize/move widgets got the
> sector_size from the selected_partition passed to .Set_Data().
>
> <snip>
>
> And I don't see any reason why .Get_New_Partition() couldn't also do
> the same by using this->selected_partition.sector_size rather than
> requiring it to be passed as a parameter.  Perhaps there is an
> historical reason why it was coded that way.  Git blame research
> required.

The addition of sector_size comes from a change I made starting in
2010-04-19 preparing for devices with sector sizes > 512 bytes, and
handling copying between devices with different sector sizes.  This
was a large change set.  I believe that passing the parameter was
necessary to ensure that the correct destination sector size was used
for the partition in Dialog_Partition_Copy::Get_New_Partition()

In the copy logic, I believe the selected_partition is the new
destination partition.  The copied_partition is the original source
partition.

The selected_partition is set equal to the copied partition
Dialog_Partition_Copy::Set_Data():

https://git.gnome.org/browse/gparted/tree/src/Dialog_Partition_Copy.cc?id=GPARTED_0_21_0#n103

I believe we need the override of the destination sector size from
Dialog_Partition_Copy::Get_New_Partition():

https://git.gnome.org/browse/gparted/tree/src/Dialog_Partition_Copy.cc?id=GPARTED_0_21_0#n121

It's not readily apparent how all this code works.  Complicating
matters is that there is inheritance of classes, and many object values
are not explicitly passed from method to method.

That's not a great explanation, but I do believe that passing sector
size is required when copying between devices with different sector
sizes.  Of course if you discover that passing sector size is not
needed, then please do remove redundant code.  I know that I am not
perfect and neither is my code.  :-)


Thanks,
Curtis
Comment 10 Mike Fleetwood 2015-02-02 21:16:20 UTC
Hi Curtis,


Will rebase this patchset in a while.


The important stuff.  How we present the capabilities to the users
------------------------------------------------------------------

> I think we need to discourage the use of disk space without a
> partition table.
...
> I'm not yet sure how we can get this message across in a short caution
> message so that non-expert users will understand the longer term
> implications of not using a partition table.

Could use a warning message similar to that which I have already added,
with just a reference or possibly even something like a clickable link
to the right section in the help (GParted Manual) as to why using
partition tables are recommended.

Other options:
* We could just decide that shouldn't dictate to users what they should
  do in the app.
* Just provide read-only support.
* Don't provide a way to create a new whole disk file system, just
  everything else but the New operation.
* Have an expert mode toggled in the GParted menu to enable creation of
  new whole disk file systems.  Outside expert mode the old dialog
  saying there is no partition table could still be displayed.

I quite like this last idea.


> The extra step of creating a "loop" partition table has the advantage
> that it already displays a warning about data loss.  This part I like
> about it because it forces two steps to format an entire disk device:
> (1) Create a new partition table "loop" or "none", and (2) Format the
> "loop" or "none" device.

Note that you can only create a whole disk file system when GParted
reports the whole disk as unrecognised (contains no recognised partition
table or file system).  The warning message when creating a whole disk
file system could be beefed to be similar the write partition table one.

There is an inconsistency with this, clearing a whole disk device empty
"loop" or "none" partition table clears the marker and makes it go back
to an unrecognised disk.  This could be worked around in GParted making
it re-write the "loop" or "none" partition table marker after erase.


> Unfortunately the "loop" functionality does not work across all
> previous libparted versions, and is limited in recognizing file
> systems.  :-(

Don't be concerned with whether "loop" works or not.  I can write
GParted to read and write fake "loop" or "none" partition table markers
if necessary to make GParted more consistent, although I'd really sooner
not.


The minor stuff.  Patch discussion
----------------------------------

> > > [10/23] Make deletion work on whole disk device file systems
...
> I guess what I see as inconsistent is the introduction of delete_op()
> which introduces the fragment "op".  We use the term operation in other
> specific methods, such as OperationCreate, and OperationDelete.
> 
> Why not continue to call this method "Delete", or "delete" for
> consistency with "create" and "format"?

Can't be called "delete" as that's a C++ key word.  I think having it
called "Delete" with only case difference in 1 letter from something
else is bad practice, hence I renamed it to "delete_op".  I don't mind
so I'll call it "Delete" if you want.


> > > // FIXME: Why does the .sector_size get passed to Get_New_Partition()?
...
> The addition of sector_size comes from a change I made starting in
> 2010-04-19 preparing for devices with sector sizes > 512 bytes, and
> handling copying between devices with different sector sizes.  This
> was a large change set.  I believe that passing the parameter was
> necessary to ensure that the correct destination sector size was used
> for the partition in Dialog_Partition_Copy::Get_New_Partition()
...
> It's not readily apparent how all this code works.  Complicating
> matters is that there is inheritance of classes, and many object values
> are not explicitly passed from method to method.
> 
> That's not a great explanation, but I do believe that passing sector
> size is required when copying between devices with different sector
> sizes.  Of course if you discover that passing sector size is not
> needed, then please do remove redundant code.
...

I'll leave if as a FIXME in the code for later, if that's OK.
Do you know of a way to simulate a disk with a different sector size?
Then I can printf debug the code on different sector size and confirm
what is needed.


Thanks,
Mike
Comment 11 Curtis Gedak 2015-02-03 02:30:29 UTC
Hi Mike,

Thanks for answering my questions, even though some seem ridiculous in
hindsight (I'd forgotten that "delete" was a reserved word).  :-)

Responses follow in-line:

> The important stuff.  How we present the capabilities to the users
> ------------------------------------------------------------------
>
> Could use a warning message similar to that which I have already
> added, with just a reference or possibly even something like a
> clickable link to the right section in the help (GParted Manual) as
> to why using partition tables are recommended.
>
> Other options:
> * We could just decide that shouldn't dictate to users what they
>   should do in the app.

This has some merit.  I don't like to restrict use, but I think we owe
it to our users to protect them from unknowingly causing data loss.
If we provide a warning then that should suffice.  If the warning is
ignored, then the fault lies with user.  At least those are my
thoughts.

> * Just provide read-only support.

If we chose this path, then I think it would be preferable to at
least permit libparted 3.2 to create a loop partition that could later
be formatted.  This would be similar to how it works from a user
perspective when your patches are applied from:

  Bug 743181 - Add unpartitioned drive read-write support.

> * Don't provide a way to create a new whole disk file system, just
>   everything else but the New operation.

Is this the same as what I described in my comments under "just
provide read-only support"?

> * Have an expert mode toggled in the GParted menu to enable creation
>   of new whole disk file systems.  Outside expert mode the old
>   dialog saying there is no partition table could still be
>   displayed.
>
> I quite like this last idea.

I recall the idea of an expert mode being suggested in at least one
other bug report.  This has some merit for protecting users from
accidentally shooting themselves in the foot.  And yet it still
permits experts to knowingly access unpartitioned device support.

<snip>

> Don't be concerned with whether "loop" works or not.  I can write
> GParted to read and write fake "loop" or "none" partition table
> markers if necessary to make GParted more consistent, although I'd
> really sooner not.

If you are not keen on this, then let's not work further on this
fake "loop" or "none" option.


> The minor stuff.  Patch discussion
> ----------------------------------
> 
> > > > [10/23] Make deletion work on whole disk device file systems
> > ...
> > Why not continue to call this method "Delete", or "delete" for
> > consistency with "create" and "format"?
>
> Can't be called "delete" as that's a C++ key word.  I think having
> it called "Delete" with only case difference in 1 letter from
> something else is bad practice, hence I renamed it to "delete_op".
> I don't mind so I'll call it "Delete" if you want.

Doh!  Now I feel silly.  I had forgot that "delete" was a reserved
word.  That must be why Bart called the method "Delete".  Based on
this I am okay with either "Delete" or "delete_op".  Your choice.  :-)

Perhaps if you add a comment about "delete" being a reserved word then
that might prevent me from making the same silly nit-pick again.  ;-)


> > > // FIXME: Why does the .sector_size get passed to Get_New_Partition()?
>
> I'll leave if as a FIXME in the code for later, if that's OK.

That would be all right with me.

> Do you know of a way to simulate a disk with a different sector
> size?

To simulate a disk with different sector sizes I use the scsi_debug
module.  For example,

To create a RAM disk using scsi_debug:

  sudo modprobe scsi_debug dev_size_mb=48 sector_size=4096

    I've used values up to 1024 mb for the device size, but depending on
    the amount of free RAM available the command can sometimes fail.

To remove the RAM disk:

  sudo rmmod scsi_debug

'Hope that helps.

Thanks,
Curtis
Comment 12 Mike Fleetwood 2015-02-03 22:28:21 UTC
Hi Curtis,


So I think we have narrowed down most of what we want to provide.  That
is for read-write support of whole disk file systems to just work using
the existing partition menu options.

What we haven't finalised yet is whether to and how to provide the
capability to create new whole disk file systems.  The options we have
are:

1) Don't provide a way to create new whole disk file system.
   (Users can still copy from, paste to, format over the top of, and all
   the other operations on whole disk file systems.  They just can't
   create a new whole disk device in a whole disk device).
   --
   This fits with the agenda of don't let the uses shoot themselves in
   the foot.

2) Have an expert mode which enables new whole disk file system dialog.
   (As option (1) but have an expert mode toggle in the GParted menu).
   --
   This fits with the agenda of as long as you say you know what you are
   doing we won't stop you shooting yourself in the foot.

3) Use libparted "loop" or GParted "none" partition table types.
   (As option (1) but used Device --> Create Partition Table "loop"/
   "none" which would end up "creating" a virtual unknown partition
   table spanning the whole disk device.  May be a libparted >= 3.2 only
   feature if using libparted "loop" type.
   --
   This fits with the agenda of here's a big, bad warning sign about
   being careful not shooting yourself in the foot.

All options will use the draft code in this patchset with the choice of
how the new operation is achieved an open question.  As option (1) is
a major subset of all options, I'll tidy up the current patchset to do
this.  I'll think about bashing together some code to implement options
(2) and (3) on top of this so we can have a play and decide what we
want.  Option (3) may require changes to, or removal of the last patch
in bug 741430, so we should push it upstream yet.
  p9/9 - Display libparted "loop" partition tables via "none" instead


Hopefully this answered your question:
> Is this the same as what I described in my comments under "just
> provide read-only support"?

Thanks,
Mike
Comment 13 Curtis Gedak 2015-02-04 17:04:22 UTC
Hi Mike,

Thank you for summarizing the options, and yes you did indeed answer my question.  :-)

In my mind option 3 seems to fit with the way GParted internally merges/handles partitions and file systems.

One concern I have with option 2 is that we might have to deal with more support questions from users who are not aware that expert mode would need to be enabled.  This comes from my experience answering questions in the forum.  Many times it would appear that users have not read the help manual and just expect things to work.  This is not to say that I'm against option 2.

I think we should try to strike a balance between making tasks easy to do while at the same time avoiding *accidental* data loss.  In my mind, when a person writes a new partition table to a device then they have *intentionally* chosen to overwrite any useful data on said device.


> Option (3) may require changes to, or removal of the last patch
> in bug 741430, so we should push it upstream yet.

Did you mean "we should *not* push it upstream yet"?

If so, then I'm glad you mentioned it because I was going to do one more review with an eye towards committing the patch set in bug 741430.

Thanks,
Curtis
Comment 14 Mike Fleetwood 2015-02-04 17:23:31 UTC
> > Option (3) may require changes to, or removal of the last patch
> > in bug 741430, so we should push it upstream yet.
> 
> Did you mean "we should *not* push it upstream yet"?

Yes.  I did mean "we should *not* push it upstream yet".

Mike
Comment 15 Mike Fleetwood 2015-02-04 20:03:42 UTC
Hi Curtis,

So I'm thinking of implementing your preferred option (3), get the user
to create a libparted "loop" partition table first.

If this is the option then the "none" partition type should be removed
and "loop" used instead.  This will change some of the comments in the
patchset for bug 741430 and a little bit of the code too.


Also this is how GParted 0.21.0 with libparted 3.2 works now:

1) Write a "loop" partition table:
   On refresh a virtual partition of unknown content spans the whole
   device.

2) Whole drive file system, formatted to cleared:
   Previews as virtual partition of cleared content.
   On refresh it become an unrecognised device.

3) Delete whole drive file system:
   Previews as becoming unallocated space.
   On refresh it becomes a virtual partition spanning the whole drive of
   unknown content.

I assume we would want to change actions (2) and (3) so that GParted
performs suitable actions so that after the refresh it actually appears
as the preview.  Therefore:

(2) Whole drive file system, formatted to cleared:
    a) Erases the file system signatures,
    b) Gets libparted to re-write the "loop" partition table signature.
    So that after refresh there is a virtual partition spanning the
    whole drive of unknown content.

(3) Delete whole drive file system:
    a) Erases the file system signatures.
    So that after refresh the drive is unrecognised.


As I have said before I would prefer that GParted closer reflects the
reality of writing whole drive file systems.  Anyway, if this is what
you really want then this is what I will implement.

Mike
Comment 16 Curtis Gedak 2015-02-04 20:32:35 UTC
Hi Mike,

My apologies if I have indicated that I only like option (3) and not option (2).

In an ideal world, GParted would already model devices, partitions, and file systems in a fashion that directly supports working with unpartitioned disk devices.  Unfortunately this is not the case.

Last year we began discussion on how GParted should use classes to better model the real world of storage systems.  At that time I believe we agreed that there is only so much we can undertake as volunteers in our spare time.  Hence we can work slowly towards a better model, but only as makes sense and as we have time.

If you have plans longer term that require that GParted closer reflects the reality of writing whole drive file systems, then that could be the better path forward.

What I want to make clear is that I do not feel strongly one way or the other on options 2 and 3.  I'm sure that there are pros and cons to both paths.

How about we proceed with your preference, especially since you are closer to the code at the moment.  Hence you likely can more clearly see future road blocks we might encounter.

Curtis
Comment 17 Mike Fleetwood 2015-02-06 20:57:02 UTC
Created attachment 296303 [details] [review]
read-write whole disk device support (draft 3)

Hi Curtis,

Here's the third draft of read-write whole disk device support.

It does what I suggested I might, and implements all 3 options outlined
in comment 12.

For option (1) everything bug creation of whole disk file systems,
checkout and compile at commit:
    Remove unused device parameter from a few GParted_Core methods

For option (3), as option (1) but also using format to "loop" as a way
to an unknown whole disk device file system which can then be formatted
to the required file system, checkout and compile at commit:
    WIP: Make format as "loop" create an unknown spanning partition (#743181)

For option (2), as option (1) but also new partition and paste into
unallocated whole disk device.  This is what you have already seen in
draft 2.  Checkout and compile all commits.
(Actually also includes option (3)).


Also implements whole disk device file system resizing.  The partition
resize UI is a bit odd when it is really just resizing a file system,
but if you remember that it's actually resizing a virtual partition
filling the whole disk device it's doing the right thing.  We might want
to change it to make it better fit resizing just a file system.


As before the code is not for review as the commits still contain WIP
(Work In Progress) and quite a few FIXMEs.  It's a vehicle for testing
the UI to decide what we want.

Thanks,
Mike
Comment 18 Curtis Gedak 2015-02-11 18:36:38 UTC
Hi Mike,

My apologies for not testing the v3 patch set in comment #17 earlier.  I've been busy with many other things.

First off, I like having all the methods available to format a complete disk device.  Being able to immediately format a device with a file system should be the most intuitive to our users.  It is also handy that the libparted loop device also works (though I think this would be a lesser used option).

I also liked that resizing a file system on a whole device works. And that if a person shrinks the partion, then when GParted refreshes it will detect that the file system does not use the whole device.

I haven't looked at the code yet, so I'm not yet familiar with how complex the changes are, or how easy this will be to maintain in the future.

Regarding the wording for the "Create new wholde device file system" dialog, it seems better.  If you did not make any changes then perhaps it is slowly becoming more acceptable in my mind.  :-)

What are your thoughts regarding supporting all methods, or forcing one way only?

Curtis
Comment 19 Curtis Gedak 2015-02-12 16:37:21 UTC
Hi Mike,

After thinking on this longer, I think we should strongly discourage formatting whole disk devices without a partition table.  As such I think that the warning must be the first sentence in the message.

Following is a suggestion for different wording for the message when formatting a whole disk device:

   *Warning:  This action is not recommended.*

   It is not recommended to create a file system on the entire disk
   without using a partition table.

   If you proceed with this action, all data on device </dev/sdb>
   will be lost.

   It is recommended to cancel this action and create a
   partition table, even for a sinle partition.  To create a new
   partition table choose the menu item:
   Device --> Create Partition Table.


The message in patch set v3 is currently:

   *Device unallocted does not contain a partition table.*
   Proceeding with theis action will create a file system on the 
   entire disk and is for experts only.

   It is recommended to cancel this operation and create a
   partition table, even for a single partition.  To create a
   new partition table choose the menu item:
   Device --> Create Partition Table.

Curtis
Comment 20 Curtis Gedak 2015-02-12 17:00:51 UTC
We might even include the reason why the action is not recommended. 


   *Warning:  This action is not recommended.*

   This action will create a file system on the entire disk.
   This action is not recommended because operating systems
   recognize common partition tables, but do not recognize
   all file system types.

   If you proceed with this action, all data on device </dev/sdb>
   will be lost.

   It is recommended to cancel this action and create a
   partition table, even for a single partition.  To create a new
   partition table choose the menu item:
   Device --> Create Partition Table.

Curtis
Comment 21 Mike Fleetwood 2015-02-12 18:42:32 UTC
Hi Curtis,

I've just had a friend do some user testing of all 3 options.  The
feedback is that he thinks option 3, create "loop" partition table
followed by formatting to the required file system is the more natural
way.  As you has a slight preference for this too, that is what I will
do.

It means we don't have to have new dialogs with lots of ugly warning
text which option 2 required although it did provide a direct method of
creating a whole disk device file system.

He also  suggested that delete of whole disk device file systems
shouldn't be allowed as it's not the same as deleting a partition which
doesn't overwrite the file system data.  Can still format to unformatted
to erase the whole disk device file system or create a new partition
table of any type.

I'll work on tidying up the patchset and make it commit ready assuming
this is OK.

Thanks,
Mike
Comment 22 Curtis Gedak 2015-02-12 20:31:43 UTC
Thanks Mike and thank your friend for the additional testing. It helps to have another perspective.  :-)

Implementing option 3, create "loop" partition table followed by formatting with a file system, works for me,

Curtis
Comment 23 Mike Fleetwood 2015-02-15 14:36:41 UTC
Created attachment 296875 [details] [review]
read-write whole disk device support (v1)

Hi Curtis,

Here's v1 of the patchset.  Finally ready for full review.  Implements
option 3 as agreed above.

Thanks,
Mike
Comment 24 Curtis Gedak 2015-02-17 17:02:01 UTC
Hi Mike,

Thank you for the updated patch set.  So far my testing is going well.

Before testing with a variety of libparted versions I just wanted to confirm that with patch set v1 in comment #23, we only expect it to work properly with libparted-3.2.  Is that right?

Curtis
Comment 25 Mike Fleetwood 2015-02-17 18:12:57 UTC
Hi Curtis,


GParted will fully work with any version of libparted.  Patch num 13/15
includes this paragraph in the commit message:

So with this patchset GParted can recognise and manipulate file systems
on whole disk devices and only requires libparted to be able to write
and recognise "loop" partition table signatures.  With this minimal
requirement all operations are supported on all distributions, at least
as far back as RedHat/CentOS 5 with libparted 1.8.1.


I have a question too.  Create a whole disk file system, then format to
cleared.  What do you think about the preview being different to what
actually happens when applied?  When your familiar with this we can
discuss the options and their implications.


Thanks,
Mike
Comment 26 Curtis Gedak 2015-02-18 17:10:01 UTC
Hi Mike,

My testing on ubuntu 14.10 with libparted 3.2 has gone well.  All operations on unpartitioned drives work as I would expect.


Testing on ubuntu 12.04 with libparted 2.3 and blkid 2.20.1 has revealed some minor anomalies that suggest to me that this patchset works best with libparted 3.2.

A)  Whole dev format with FAT16 results in empty MSDOS partition table

Create a loop partition table on device /dev/sdb and format it to FAT16.  When gparted refreshes it shows unallocated space and View -> Device -> Information shows an msdos partition table.

Note that the blkid command recognizes the fat format on device /dev/sdb, but parted sees it as an empty partition /dev/sdb1 in an msdos partition table.

B)  Whole dev format with ext2 results in unknown filesystem.

Create a loop partition table on device /dev/sdb and format it to ext2.  When gparted refreses it shows an unknown file system and View -> Device -> Information shows a loop partition table.

Note that blkid does not recognize any file system on device /dev/sdb, and parted sees a loop partition table with an ext2 file system.


Next I will visually review the patch set.

Curtis
Comment 27 Curtis Gedak 2015-02-18 17:59:23 UTC
Hi Mike,

Patch set v1 in comment 23 looks pretty good to me.  I like that you
also picked up Phillip's force creation of ext2/3/4 and ntfs on whole
disk devices patch.


Following is one question on whether or not we should have the word
"none" as translatable.


In 13/15 of the patch set, the word "none" is translatable and the

   /* TO TRANSLATORS:  none
    * means that the disk device is not partitioned
    * and that there is a file system covering the
    * whole disk.
    */
   temp_device.disktype = _("none");

Should we enable the word "none" as translatable when it is
specifically referring to a disk device that is not partitioned?

From my understanding of translation, only one copy of the word "none"
is made in the .po file for the translator to translate.  If
additional meaning beyond the word is needed (for example none
referring to a partition table versus none referring to something
else) then we might need something more specific like "no partition
table".

If I recall, the names of partition tables types in libparted are not
translatable, so I would be okay if "none" was also not translatable
in this situation.


Regarding your earlier question in comment #25 on "Create a whole disk
file system, then format to cleared.  What do you think about the
preview being different to what actually happens when applied?", I'm
okay with how this works.  The preview is indeed different (shows file
system "cleared" spanning disk) compared to the applied result (shows
unallocated disk with unrecognized disk label.)

Curtis
Comment 28 Mike Fleetwood 2015-02-18 19:49:58 UTC
Hi Curtis,

(In reply to Curtis Gedak from comment #26)
> B)  Whole dev format with ext2 results in unknown filesystem.

This Sounds exactly like creating the ext2 file system didn't happen and
the libparted "loop" signature "GNU Parted Loopback 0" is still on the
disk.  I definitely get an ext2 file system spanning the whole drive
even with CentOS 5 and libparted 1.8.1.

Can you re-try this test and if it's reproducible examine the operation
results in GParted and also look at the content of the disk with
hexdump.  When I *JUST* create partition table "loop" this is what I see
at the start of the drive:

# dd if=/dev/md126 count=1 | hexdump -C
1+0 records in
1+0 records out
512 bytes (512 B) copied, 0.000662262 s, 773 kB/s
00000000  47 4e 55 20 50 61 72 74  65 64 20 4c 6f 6f 70 62  |GNU Parted Loopb|
00000010  61 63 6b 20 30 00 00 00  00 00 00 00 00 00 00 00  |ack 0...........|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000200

Thanks,
Mike
Comment 29 Curtis Gedak 2015-02-18 20:16:22 UTC
Hi Mike,

It's starting to look like a some type of caching or not clearing out of the previous partition table problem.

If I start the VM and the device (/dev/sdb in my case) already has an msdos partition table with at least one partition then I experience the problem.

If the device has no partition table then the following steps demonstrate the problem:

1)  Create "loop" partition table and apply.

2)  Format device to "fat16" and apply.

    Note that gparted thinks we have an msdos partition table with
    unallocated space.

3)  Create new partition as "ext4" (spans disk) and apply.

    At this point we also have an ext4 partition on /dev/sdb1.
    Device paths exist for /dev/sdb and /dev/sdb1.

4)  Create "loop" partition table and apply.

    I think this is where things go further sideways.
    GParted shows "loop" partition table and "unknown" file system.
    Device paths exist for /dev/sdb and */dev/sdb1*!

5)  Format device to "ext2" and apply.

    Gparted incorrectly shows "loop" partition table and "unknown" FS.
    Device paths exist for /dev/sdb and */dev/sdb1*!   
    Command "dd if=/dev/sdb count=1 | hexdump -C" shows all zeroes.

I can test this on other VMs if that would help.

Curtis
Comment 30 Curtis Gedak 2015-02-18 20:19:54 UTC
'Forgot to say that when I reboot the above Ubuntu 12.04 VM, then all is displayed as it should have been.  Specifically an "ext2" file system spanning disk device /dev/sdb.

The fact that everything is okay after a reboot indicates to me that somehow things are getting out of sync.
Comment 31 Curtis Gedak 2015-02-18 20:59:32 UTC
Results from using the steps from comment #29 in different VMs.

Distro         Parted   Result
------------   ------   ----------------------------------------------
Fedora 12       1.9.0   Works - displays ext2 FS spanning disk
Ubuntu 10.04    2.2     Broken - Same as comment 29, okay after reboot
Ubuntu 12.04    2.3     Broken - Same as comment 29, okay after reboot
Fedora 18       3.1     Broken - Same as comment 29, okay after reboot
Fedora 20       3.1     Broken - Same as comment 29, okay after reboot
Fedora 21       3.2     Works - displays ext2 FS spanning disk
Ubuntu 14.10    3.2     Works - displays ext2 FS spanning disk

This leads me to believe that the problem started sometime after parted 1.9.0 and was fixed for parted 3.2.
Comment 32 Mike Fleetwood 2015-02-18 21:59:46 UTC
(In reply to Curtis Gedak from comment #26)
> A)  Whole dev format with FAT16 results in empty MSDOS partition table

From the parted NEWS file against 2.4:
  libparted once again recognizes a whole-disk FAT partition
  [bug introduced in parted-1.9.0]

The fixing commit:
libparted: avoid regression when processing a whole-disk FAT partition
http://git.savannah.gnu.org/cgit/parted.git/commit/?id=616a2a1659d89ff90f9834016a451da8722df509

So effects libparted 1.9.0 to 2.3 inclusive.

This effects Ubuntu 12.04 LTS, CentOS 6 and other older but still
supported distros.  The only way I see to fix this would be to use blkid
to detect whole disk file systems before using libparted to recognise
any partition tables.

But blkid is so old on CentOS 5 it can't distinguish between FAT16/32.
FS_Info uses existence SEC_TYPE="msdos" to detect FAT16 and it's absence
to detect FAT32.  On CentOS 5 it reports SEC_TYPE="msdos" for both
FAT16 and FAT32, so this would detect everything FAT16.  Double but;
libparted 1.8.1 correctly detects this as loop partition table with
either FAT16 or FAT32.

I will have to fix this along the above lines.
Comment 33 Curtis Gedak 2015-02-18 22:08:42 UTC
Hi Mike,

(In reply to Curtis Gedak from comment #26)
> B)  Whole dev format with ext2 results in unknown filesystem.

I just wanted to clarify that this problem occurs if there is a pre-existing partition table (I tested with both MSDOS and GPT).  It doesn't rely on formatting a whole disk with fat16.

The problem does not occur with libparted 1.9.0 or 3.2.
I'm not sure of the exact range of libparted versions that are faulty, but can say that the problem does occur with libparted 2.2 through 3.1.

Curtis
Comment 34 Mike Fleetwood 2015-02-18 22:47:28 UTC
(In reply to Curtis Gedak from comment #33)
> I just wanted to clarify that this problem occurs if there is a pre-existing
> partition table (I tested with both MSDOS and GPT).  It doesn't rely on
> formatting a whole disk with fat16.
> 
> The problem does not occur with libparted 1.9.0 or 3.2.
> I'm not sure of the exact range of libparted versions that are faulty, but
> can say that the problem does occur with libparted 2.2 through 3.1.

This is sounding like a good match for the libparted cache coherency
issue.  It affected libparted 2.0 to 3.1 inclusive.

Parted 3.2 NEWS fragment:
  Fix cache coherency issue by flushing partition block devices.
  This had been mistakenly disabled in parted 2.0, and resulted
  in parted sometimes identifying the previous filesystem type
  after running an mkfs to format a partition to a new type.

Fixing commit:
Revert "linux-commit: do not unnecessarily open partition device nodes"
http://git.savannah.gnu.org/cgit/parted.git/commit/?id=fb99ba5ebd0dc34204fc9f1014131d5d494805bc

I previously worked around it by calling ped_device_sync() when erasing
file system signatures:
Flush device after wiping a file system (#688882)
https://git.gnome.org/browse/gparted/commit/?id=797f0b8eeb61f81da1791c3cf048b5c0abf9a550

and in:
Flush devices when scanning to prevent reading stale signatures (#723842)
https://git.gnome.org/browse/gparted/commit/?id=3bea067596e3e2d6513cda2a66df1b3e4fa432fb

so my initial reaction is that we can probably work around it in this
situation too.  More investigation required to confirm.

Mike
Comment 35 Mike Fleetwood 2015-02-19 09:46:26 UTC
Created attachment 297250 [details] [review]
Correct format cleared preview (draft 1)

Hi Curtis,

(In reply to Curtis Gedak from comment #27)
> Regarding your earlier question in comment #25 on "Create a whole disk
> file system, then format to cleared.  What do you think about the
> preview being different to what actually happens when applied?", I'm
> okay with how this works.  The preview is indeed different (shows file
> system "cleared" spanning disk) compared to the applied result (shows
> unallocated disk with unrecognized disk label.)

Can you try the attached patch on top of patchset v1 and let me know
what you think.

Thanks,
Mike
Comment 36 Mike Fleetwood 2015-02-19 17:17:14 UTC
Created attachment 297287 [details] [review]
read-write whole disk device support (v2)

Hi Curtis,


Here's patchset v2.  Compared to patchset v1:

* Adds patch "Avoid whole disk FAT being detected as MSDOS partition
  table" to resolve issue (A).

* Includes patch "Correct format cleared preview (draft 1)" from
  comment 35.

Doesn't try to fix:
* CentOS 5 not being able to distinguish between whole disk file
  systems FAT16 and FAT32.
* Issue (B) format whole dev with ext2 results in unknown file system


> B)  Whole dev format with ext2 results in unknown filesystem.

Using this patchset, can you produce a minimal test case.  I'm not
totally clear what steps I need to do to reproduce this.


Thanks,
Mike
Comment 37 Mike Fleetwood 2015-02-19 19:46:38 UTC
Hi Curtis,

I think I have worked out what's happening with:
> B)  Whole dev format with ext2 results in unknown filesystem.

Steps to reproduce:
1) Create any real partition table (e.g. MSDOS or GPT)
2) Create any partition anywhere with any content
3) Create "loop" partition table
4) Format whole device with any file system

Result: GParted shows whole disk device with unknown file system

What's going wrong in the above steps:
3) Libparted is not informing the kernel to remove the old partition(s)
   with the removed partition table.
4) Because the blkid cache has stale data doesn't report the newly
   created file system.
     blkid | fgrep $MYDEV
   produces nothing.  But
     blkid /dev/$MYDEV does show the signature.

Libparted 2.4, 2.1 get this wrong as described above.
Libparted 1.8.1, 3.2 get this right.
Libparted 3.1 (on CentOS 7) gets this right.  Might be a distro specific
patch.

Fix ideas:
1) If have old (broken) libparted, or perhaps always, create an MSDOS or
   GPT partition table to get libparted to delete any old partitions
   before then creating the required "loop" partition table.  This will
   then inform the kernel the partitions have been removed and trigger
   blkid to update it's cache.
2) If have old (broken) libparted, or perhaps always, just delete all
   existing partitions before creating a new "loop" partition table.

Now to see if I can find the commit in parted which fixed and broke
this.

Mike
Comment 38 Curtis Gedak 2015-02-20 16:55:51 UTC
Hi Mike,

Initial testing of patch set v2 in comment #36 on Fedora 12 with libparted 1.9.0 looks good.

The patch for issue (A) "Avoid whole disk FAT being detected as MSDOS partition table" worked.  In addition the preview for an unformatted "loop" partition looks the same in the preview as it does after apply.  :-)

(In reply to Mike Fleetwood from comment #37)
> Using this patchset, can you produce a minimal test case.  I'm not
> totally clear what steps I need to do to reproduce this.

It seems that you determined the minimal case already.  :-)

Your fix ideas in comment 37 sound like a good approach too.  I think approach (2) might have the advantage of being slightly cleaner as we would not introduce a potentially different temporary partition table.

Curtis
Comment 39 Mike Fleetwood 2015-02-22 13:10:53 UTC
Re:
> B)  Whole dev format with ext2 results in unknown filesystem.

(In reply to Curtis Gedak from comment #31)
> Results from using the steps from comment #29 in different VMs.
> 
> Distro         Parted   Result
> ------------   ------   ----------------------------------------------
> Fedora 12       1.9.0   Works - displays ext2 FS spanning disk
> Ubuntu 10.04    2.2     Broken - Same as comment 29, okay after reboot
> Ubuntu 12.04    2.3     Broken - Same as comment 29, okay after reboot
> Fedora 18       3.1     Broken - Same as comment 29, okay after reboot
> Fedora 20       3.1     Broken - Same as comment 29, okay after reboot
> Fedora 21       3.2     Works - displays ext2 FS spanning disk
> Ubuntu 14.10    3.2     Works - displays ext2 FS spanning disk
> 
> This leads me to believe that the problem started sometime after parted
> 1.9.0 and was fixed for parted 3.2.

I've tested this on:

Distro      Parted      Results
---------   ---------   ----------------------------
Fedora 21   3.1         Works - displays FS spanning disk
CentOS 7    3.1         Works - displays FS spanning disk
CentOS 6    built 3.1   Works - displays FS spanning disk


Curtis,

Can you re-test your above cases involving parted 3.1 please.  I find
that 3.1 works correctly.  (Minimal steps in comment 37).

Thanks,
Mike
Comment 40 Curtis Gedak 2015-02-22 17:26:00 UTC
Hi Mike,

Re:
> B)  Whole dev format with ext2 results in unknown filesystem.

Following are the results of retesting with patch set v2 from
comment #36 using the test steps in comment #37
(msdos PT, fat16 Part1 FS, loop PT, ext2 FS).

Distro         Parted   Result
------------   ------   ----------------------------------------------
Fedora 12       1.9.0   Works - displays ext2 FS spanning disk
Ubuntu 10.04    2.2     Broken - Same as comment 29, okay after reboot
Ubuntu 12.04    2.3     Broken - Same as comment 29, okay after reboot
Fedora 18       3.1     Works - displays ext2 FS spanning disk
Fedora 20       3.1     Works - displays ext2 FS spanning disk
Fedora 21       3.2     Works - displays ext2 FS spanning disk
Ubuntu 14.10    3.2     Works - displays ext2 FS spanning disk

Based on the above test results it appears all is okay with
patch set v2 and libparted 3.1.

I no longer have patch set v1 in case your intention was for me to
retest with patch set v1.

Curtis
Comment 41 Curtis Gedak 2015-02-28 16:47:10 UTC
Created attachment 298169 [details] [review]
read-write whole disk device support (v3)

Due to technical issues I am making this post on Mike Fleetwood's behalf.

Mikes post follows....

Here's patchset v3.  This seems to have turned into an exercise in side
stepping and working around various bugs with libparted and blkid.
Anyway, changes compared to patchset v2:

* Adds patch "Skip reading existing partition table before creating a
  new one (#743181)"

* Adds patch "Make older blkid distinguish between FAT16 and FAT32
  (#743181)".  Turns out what I initially saw on CentOS 5 effects blkid
  quite widly.

* Adds patch "Fix failure to recognise whole disk file systems in
  certain cases (#743181)" to resolve issue (B) Whole dev format with
  ext2 results in unknown filesystem.

* Re-orders patches num 13 onwards.

Thanks,
Mike
Comment 42 Curtis Gedak 2015-03-01 18:55:11 UTC
Hi Mike,

Re:
> B)  Whole dev format with ext2 results in unknown filesystem.

Following are the results of retesting with patch set v3 from
comment #41 using the four test steps in comment #37
(msdos PT, fat16 Part1 FS, loop PT, ext2 FS).

Distro         Parted   Result
------------   ------   ----------------------------------------------
Fedora 12       1.9.0   Works - displays ext2 FS spanning disk
Ubuntu 10.04    2.2     Works - displays ext2 FS spanning disk
Ubuntu 12.04    2.3     Works - displays ext2 FS spanning disk
Fedora 18       3.1     Works - displays ext2 FS spanning disk
Fedora 20       3.1     Works - displays ext2 FS spanning disk
Fedora 21       3.2     Works - displays ext2 FS spanning disk
Ubuntu 14.10    3.2     Works - displays ext2 FS spanning disk

Based on the above test results it appears that you resolved
problem (B) with patch set v3.  :-)

My next tasks are to review the code, and to perform some regression
testing.

Curtis
Comment 43 Curtis Gedak 2015-03-07 16:56:19 UTC
Hi Mike,

'Just finished reviewing the patch set v3 code in comment #41.  Most all looks good to me.  There is only one area that might cause some issues, and this is centered around blkid caching and systems with incorrectly configured BIOS that indicates a floppy drive is present when no actual device exists.

PROBLEM

Patch set v3 changes from "blkid" to "blkid -c /dev/null".  This can cause an excessive delay on device scans.  The code change in the patch set is:

  P18/20 Make older blkid distinguish between FAT16 and FAT32 (#743181)

Back in 2012-01-06, we changed from "blkid -c /dev/null" to "blkid" to avoid excessive delay on device scans.  See:

  Fix long scan problem when BIOS floppy setting incorrect
https://git.gnome.org/browse/gparted/commit/?id=18f863151c82934fe0a980853cc3deb1e439bec2


POSSIBLE SOLUTION

Perhaps we might be able to use your new file system detection method GParted_Core::recognise_filesystem_signature to distinguish between FAT16 and FAT32?

Curtis
Comment 44 Mike Fleetwood 2015-03-08 08:17:58 UTC
Hi Curtis,

I've looked at the source in blkid.  Detection of FAT is complicated and
easily mixed up with MSDOS partition tables (as libparted has previously
done).  I'd sooner not try to do that in GParted.
https://git.kernel.org/cgit/utils/util-linux/util-linux.git/tree/libblkid/src/superblocks/vfat.c

I see a few options:
1) Remove P18/20 and don't distinguish when changing between FAT* file
   systems on whole disk devices with old versions of blkid.
2) Keep P18/20 and force users to correctly set their BIOS to only
   report a floppy drive only when one exists.  Floppies are legacy
   devices anyway.
3) Use libparted to detect the difference between FAT*.  Only partially
   helps because libparted 1.9.0 to 2.3 inclusive detect it as a MSDOS
   partition table instead.
4) Only when blkid reports a FAT* file system on a whole disk device,
   and optionally when blkid is old enough, query blkid again for just
   that whole disk device bypassing the cache.

I will code option (4) as I have a plan of how to keep all the changes
within the FS_Info module.

Thanks,
Mike
Comment 45 Mike Fleetwood 2015-03-08 14:20:44 UTC
Created attachment 298813 [details] [review]
read-write whole disk device support (v4)

Hi Curtis,

Here's patchset v4.  Compared to patchset v3, the only change is:

* P18/20 Re-written to fix as outligned above.  Only with old blkid and
  when vfat is reported, query blkid again bypassing the cache.

Thanks,
Mike
Comment 46 Curtis Gedak 2015-03-08 19:39:22 UTC
Hi Mike,

Thank you for continuing to improve this patch set.  Adding read-write
support for whole devices has been a significant undertaking.

I like your innovative approach to resolving the "blkid -c /dev/null"
cache and misconfigured BIOS excessive floppy scan problem.

Following are my test results using the four test steps in comment
#37, in addition to running each supported operation for FAT16 and
EXT4 file systems in partition tables.  More specifically the create,
delete, grow, shrink, move, copy, check, label, and new UUID
operations.

Distro           Parted   Result
---------------  ------   ----------------------------------------------
Fedora   12       1.9.0   Success - all operations worked
Debian    7       2.3     Success - all operations worked
openSUSE 13.2     3.1     Success - all operations worked
Ubuntu   14.10    3.2     Success - with One issue [1]

[1] When resizing FAT16 partition from 540MB to 520MB, libparted indicated:

      Would you like to use FAT32?  If you leave your file system as
      FAT16, then you will have no problems.  If you convert to FAT32,
      and MS Windows is installed on this partition, then you must
      re-install the MS Windows boot loader.  If you want to do this,
      you should consult the Parted manual (or your distribution's
      manual).  Also, converting to FAT32 will make the file system
      unreadable by MS DOS, MS Windows 95a, and MS Windows NT.

    I choose "No" and GParted proceeded to crash.

    This is a known problem with fat16 resizing with libparted-3.2.
    The problem was reported earlier and a patch for parted has been
    created.

      Bug 735669 - GParted crashes resizing fat16 file system

    The version of libparted in use was:

    $ dpkg -l | grep libparted
    ii  libparted-dev:i386                                  3.2-6 \
         i386         disk partition manipulator - development files
    ii  libparted-fs-resize0:i386                           3.2-6 \
         i386         disk partition manipulator - shared FS resizing library
    ii  libparted2:i386                                     3.2-6 \
         i386         disk partition manipulator - shared library

    It appears that the patch has not made it into Ubuntu 14.10.
    Since this problem is separate from patch set v4, I consider the
    testing on Ubuntu 14.10 a success.


I am now ready to commit this patch set to the git master.

If I don't hear any objections, on Monday I will begin committing the
necessary patches in the following order:

  bug 741430 - read-only whole disk support
  bug 742741 - RHEL6 support of nilfs2
  bug 743181 - read-write whole disk support

After that I will begin work on the documentation updates.  My
thoughts are to indicate whole disk device format support in a "loop"
section of "Create Partition Table" and indicate which partition
operations will work on whole device formats.

Thanks again Mike for all your work on this enhancement.  :-)

Curtis
Comment 47 Mike Fleetwood 2015-03-09 13:54:45 UTC
Hi Curtis,

(In reply to Curtis Gedak from comment #46)
>
> After that I will begin work on the documentation updates.  My
> thoughts are to indicate whole disk device format support in a "loop"
> section of "Create Partition Table" and indicate which partition
> operations will work on whole device formats.

Yes, that looks like the right place to inform the user to create a
"loop" label and then format with the required file system, which
changes the table type to "none".  (Include shortcut to Formatting a
Partition).  Not sure you really need to list which operations are
supported though.

Probably have a note in the Managing Partition Flags section that whole
disk device file system, shown as "loop" and "none" partition table
types, don't support partition flags because there is no actual
partition table.

Mike
Comment 48 Curtis Gedak 2015-03-09 17:22:11 UTC
Thank you Mike for the documentation suggestions.  I will work towards incorporating your suggestions into patches for your review.

The patch set v4 in comment #45 has been committed to the git repository for inclusion in the next release of GParted.

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

Add whole_device flag to the partition object (#743181)
https://git.gnome.org/browse/gparted/commit/?id=5098744f9aa958ba18d2a4657ea4345e275c885b

Enable operations on whole disk device virtual partitions (#743181)
https://git.gnome.org/browse/gparted/commit/?id=6c333f845c723c640db780f056a6dc930718e157

Document workings of calibrate_partition() method
https://git.gnome.org/browse/gparted/commit/?id=b293a464cbcaf446eed204bf9e84183e518f58e0

Split get_device_and_disk() into two (#743181)
https://git.gnome.org/browse/gparted/commit/?id=51ac4d56489653854cd22787238a14bfa66a6ad4

Make calibrate work with whole disk devices (#743181)
https://git.gnome.org/browse/gparted/commit/?id=c4229c99f880891759f1e7ed726fc0d6c7590146

Report either partition or device in operational results (#743181)
https://git.gnome.org/browse/gparted/commit/?id=84eb35b7eb15964a188cbdd1f17a33143ea0ec21

Make clearing signatures work with whole disk devices (#743181)
https://git.gnome.org/browse/gparted/commit/?id=dc4e69136c3595f3c28253915753b65352004274

Make format work with whole disk devices (#743181)
https://git.gnome.org/browse/gparted/commit/?id=db8d964fba799dd986a43714e5ad7bcf0b3b6cee

Force creation of ext2/3/4 and ntfs on whole disk devices (#683643)
https://git.gnome.org/browse/gparted/commit/?id=70e17e938860702fd929a4ff2ad93ef89f8a086b

Force creation of reiserfs on whole disk devices (#743181)
https://git.gnome.org/browse/gparted/commit/?id=fb9653fd8efaebba09033db181ca359b87fa76cf

Make copy into existing whole disk device file systems work (#743181)
https://git.gnome.org/browse/gparted/commit/?id=85a6c88eee59bb0f36a79c0d6450d133e4a03883

Make resize of whole disk file systems work (#743181)
https://git.gnome.org/browse/gparted/commit/?id=c01106c54e98fa77c899adffa5797b7d173ab982

Make "loop" table appear as unknown whole device file system (#743181)
https://git.gnome.org/browse/gparted/commit/?id=8607717b7b756d38f6e6716c71f8973306217081

Correct whole disk device file system format to cleared preview (#743181)
https://git.gnome.org/browse/gparted/commit/?id=63f701033ebd30d948ce3c885f3475f6ef210013

Skip reading existing partition table before creating a new one (#743181)
https://git.gnome.org/browse/gparted/commit/?id=e9a5cf2843000d5d4cff5648e3f79b2a547e17fa

Erase file system signatures before creating a partition table (#743181)
https://git.gnome.org/browse/gparted/commit/?id=e7ed209020331aa26cb4b2f60bfa984a78d7fdf0

Avoid whole disk FAT being detected as MSDOS partition table (#743181)
https://git.gnome.org/browse/gparted/commit/?id=f8faee637787329c07771e495c9b26abc9ac1603

Workaround older blkid not distinguishing between FAT16 and FAT32 (#743181)
https://git.gnome.org/browse/gparted/commit/?id=4087cb2e2ba3e5b43e0118f6364f3b00f44961d9

Fix failure to recognise whole disk file systems in certain cases (#743181)
https://git.gnome.org/browse/gparted/commit/?id=9c1a833a0d654d697420780f452517aa0b4925eb

Remove unused device parameter from a few GParted_Core methods
https://git.gnome.org/browse/gparted/commit/?id=4d83d3723d1ff40c93bc2f167058b836e06f267c
Comment 49 Curtis Gedak 2015-03-09 17:27:38 UTC
*** Bug 683643 has been marked as a duplicate of this bug. ***
Comment 50 Curtis Gedak 2015-03-09 21:34:06 UTC
Created attachment 298918 [details] [review]
Unpartitioned disk R/W support help manual update (v1)

Hi Mike,

Attached is patch v1 to add support of unpartitioned devices to the help manual.  With this update I am trying to *not encourage* the use of devices without partition tables.  As such I have kept the mention of unpartioned disks to a minimum.

If I have missed any key areas, then please let me know.

Thanks,
Curtis
Comment 51 Mike Fleetwood 2015-03-10 09:55:13 UTC
Hi Curtis,

Please make it a separate note containing "To use a disk without a
partition table, choose loop ..." rather than incorporating it into the
existing note.  They are about different aspects, existing note is about
the default partition table and this new one is about how to use loop
type.

Otherwise OK.

Thanks,
Mike
Comment 52 Curtis Gedak 2015-03-10 15:54:37 UTC
Created attachment 299016 [details] [review]
Unpartitioned disk R/W support help manual update (v2)

Hi Mike,

Thanks for the suggestion.  I have placed the "loop" section in it's own note.

This patch set contains an additional change.  I have added a note in the delete partition section to indicated that deleting a file system on an unpartitioned disk requires choosing format to cleared.

Curtis
Comment 53 Mike Fleetwood 2015-03-10 20:16:26 UTC
Hi Curtis,

I've pushed the documentation update v2 from comment 52 to the public
GIT repo.

Update help manual for support of devices without partition tables (#743181)
https://git.gnome.org/browse/gparted/commit/?id=91e197ac8b9a4f728bf8974a3b5863e6b3bcd8df

Thanks,
Mike
Comment 54 Curtis Gedak 2015-03-23 17:48:33 UTC
This enhancement was included in the GParted 0.22.0 release on March 23, 2015.