As far as I can see, first thing I need is to get the VG names in lvm2_vg_cache into the drop down currently showing devices. Writing a function std::vector<Glib::ustring> LVM2_PV_Info::get_vg_names() is not a biggie, not sure how to add it into GParted_Core::set_devices_thread().

Any ideas anyone?

Hi Wrolf,

As general guidance advice I would say make sure you understand the
data representations and existing code inside and out.  Understand
everything it is doing and why.  Then how to adapt it to handle the new
or changing needs it usually clear.  Sometimes it still takes trying a
few different ways to find the one which works best.

My first idea is ... roughly ...
Refactor the existing code mostly into several new functions:
probe_disk_devices(), confirm_disk_devices() and
populate_disk_devices().  Only example names to suggest where to split
the code.  Then add probe_volume_groups() and populate_volume_groups().
Don't see a need for confirm_volume_groups().  Then have to make it
accept either nothing or disk devices and/or volume groups specified on
the command line.  Call all the probe_*() functions if none specified.
Have to identify what are disk devices and what are VG names so that the
populate_*() functions operation on the correct lists.

All of this needs to be a number of fully working patches incrementally
adapting the code.  This is for git bisectability and review purposes.

As some point these questions (and many more) will need answering so
that GParted is changed in a coherent way:
1) Does the Device class need to adapt in any way to represent a Volume
   Group?
2) How will disk devices be distinguished from Volume Groups?
3) What will all the Device member variables be set to for a Volume
   Group?
4) How will all the existing code handle this?
Same sort of questions about Partition class too.

There is a lot of code which will have to be adapted as until now
GParted only knew about disk devices and partitions.

Mike

Hi Wrolf,

After reading GParted_Core::set_devices_thread() to answer your question
yesterday I found this new bug 755495 - GParted allowing partitioning of
large sector devices specified on the command line, when built with old
libparted which doesn't support it.

So, I too need to modify GParted_Core::set_devices_thread() to fix this
bug.

If you like I can do the refactoring of the set_devices_thread()
function into the separate functions as I discussed above.  Only
offering to split the existing code into separate functions to make it
easier to work with.  Won't be writing any of the new code you need to
handle VGs and LVs.  Realistically it will take me maybe 2 or 3 weeks to
code, test and for Curtis to review before it appears upstream.

In the interim you can code the probe_volume_groups() and
populate_volume_groups() functionality without bothering with command
line handling and refactoring set_devices_thread().  For now just call
whatever you code from near the end of the existing
set_devices_thread().  That will take you a while and then you can
continue with the rest of the challenge.  Can merge with the refactored
set_devices_thread() later.

Please let me know if this is useful.

Thanks,
Mike

Go ahead, factor GParted_Core::set_devices_thread(). I have quite a bit of work to do just figuring out where to add the VG code, I will merge my changes after you factor.

Wrolf

Created attachment 313057 [details] [review]
Add probing and command line handling of VG names (v1)

Hi Worlf,

For bug 755495 I didn't need to do as much refactoring as I suggested I
would.  So here are 2 patches for you to base your work on top of, which
add probing and command line handling of VG names.

Thanks,
Mike

Hi Mike and Wrolf,

I reviewed and tested patch v1 in comment #5.  As Mike mentioned the last patch in the set currently demonstrates how VG Names might be handled.  As such it is not ready for production.

However, the first patch titled:
   Create new method GParted_Core::set_device_from_disk() (#755429)
looks production worthy.

Mike, if you think it would be of help, I could commit the first patch only.

Otherwise I am okay with holding off until more work can be done on this report.

Thanks,
Curtis

Hi Curtis,

Thank you for reviewing and testing the code.  I would prefer that we
wait to commit this code until Wrolf is ready with his patches based on
top of these; including my second patch as is.

(P2 adds working handling of VG names either probed or named on the
command line.  It is just left for one of Wrolf's following patches to
replace the debug line with code to populate the Device object as
needed.  As part of a larger patchset committed together it is perfectly
OK.  Fully working bisectable code incrementally adding functionality.
Just not releasable to the users after P2).

Thanks,
Mike

Created attachment 313418 [details] [review]
Add probing and command line handling of VG names (v2)

Hi All,

Here is patchset v2.  The only difference is that it has been updated to
apply cleanly on top of current git master after the changes for
bug 756434.

Thanks,
Mike

Created attachment 330395 [details] [review]
Show LVM2 VGs and LVs (draft 1)

Hi Curtis,

Yesterday I replied to someone in the GParted IM channel asking how come
GParted still didn't support LVM.  I replied that this bug was open to
add read-only support but little progress had been made so far.  Anyway
I decided to see what it would take to display VG and LV information in
GParted.

The code is here to see if the direction looks reasonable and to prompt
questions and issues from a UI point of view.  What the code does:
1) Add VG names to the top of the list of disks in the drop down;
2) Display LVs when the VG is selected, including LV content (when the
   VG is active).  There are path issues because of different names
   /dev/mapper/VGNAME-LVNAME and /dev/VGNAME/LVNAME and some stuff
   doesn't work.  This is what bug 767842 needs to be making possible.

Probably lots of labels and menus need renaming / amending when
displaying VGs LVs.  Thoughts welcome.

Thanks,
Mike

Hi Mike and Wrolf,

Thanks Mike for creating a draft patch (d1) for discussion.

Patch (d1) in comment #9 compiled and ran for me.

I agree with adding the VG Names to the top of the list of devices.
On systems using LVM2, the user is likely to wish to view information
on LVs within a VG because that is where the user data and OS is
likely stored.

I also like the way LVM2 information is placed within the same UI that
GParted users are already familiar with for disk information.

Following is a question to help my understanding.

Q1) Why remove *device_paths* and replace with *disk_paths*?

    Are you thinking ahead to implement other types such as
    *raid_paths* for future operations such as create RAID?

    The original string vector *device_paths* could contain physical
    disk paths such as /dev/sda, in addition to RAID devices such as
    /dev/mapper/isw_efjbbijhh_Vol0 or /dev/md0.  Because these latter
    devices might be comprised of multiple partitions, I think it is
    more appropriate to keep the name *device_paths*.

Overall I think this draft is a good step forward in the journey to
implement read-only support of LVM2 VGs and LVs.

Thanks,
Curtis

Hi Curtis,

Q1) Why remove device_paths and replace with disk_paths?

So this is in relation to the code changes in:
  [P 3/8] Add probing and command line handling of LVM Volume Group names

Not thinking ahead to using alternate sources to get block device names,
such as SWRaid (mdadm).

The terms device and disk are often used interchangeably, and I just
needed some distinct and logical variable and function names.

The old code had:
  device_paths -> set_device_from_disk() -> devices

  str vector      Processes one str to      Device vector
                  one Device

The new code does:
              +-> disk_paths -> set_device_from_disk() -+
  user_paths -+                                         +-> devices
              +-> vg_names   -> set_device_from_vg()   -+

  str vector      str vector    Processes one str to        Device vector
                                one Device

I needed to separate the names passed on the command line into two
separate lists: those for disks/devices and those for VGs.  As each
disk/device name was then passed to set_device_from_disk() I named that
vector "disk_paths" rather than "device_paths".  It didn't seem to make
sense to name the function set_device_from_device() and I wanted the
vector name and function to go together a bit.

Thanks,
Mike

Hi both,

In relation to the UI I am thinking that when displaying a Volume Group
a whole set of subtle changes need to be made:
1) Use a separator between the VGs and devices in the dropdown;
2) Optionally add headings into the dropdowns: "Volume Groups",
   "Devices"
3) Rename "Device" menu to "Volume Group".
4) Only item in the Volume Group menu would be: activate / deactivate.
   This action is already supported via activate / deactivate on the
   PV.
5) Rename "Partition" menu to "Logical Volume".
5) Add the following items to the "Logical Volume" menu:
5.1) Unmount / mount
5.2) Information
   Read-only dictates no more.  However these might be added without
   needing to administrate LVM.
5.3) Copy
5.4) Paste
5.5) Format to >
5.6) Check
5.7) Label File System
5.8) New UUID
6) Rename View > "Device Information" to "Volume Group Information"
7) Rename and amend information displayed in "Device Information" side
   pannel to "Volume Group Information".  Display the following:
7.1) "Model: LVM2 Volume Group"
7.2) Rename "Serial" to "UUID"
7.3) Keep "Size"
7.4) Rename "Path" to "Name"
7.5) Remove "Partition table"
7.6) Remove "Heads"
7.7) Remove "Sectors/track"
7.8) Rename "Total sectors" to "Extents"
7.9) Rename "Sector size" to "Extent size"
8) In GParted > Devices radio selector add a separator between the VGs
   and the devices.
9) Optionally add headings into the radio selector "Volume Groups" and
   "Devices"

Mike

Hi Mike,

Thank you for the clear explanation for the change from device_paths
to disk_paths in comment #11.  I am now on-board.  :-)

With regards to the suggested changes in comment #12, I think we
should consider when/if it makes sense to treat VGs separate from
Devices.  By this I mean that we would consider separate dialogs for
VGs.

For example:  class Dialog_Partition_Info and a new Dialog_LV_info.

I am also wondering whether dynamically changing menu names such as
*Device* to *Volume Group* is good for the user experience.  I know we
already do this for *mount/unmount*, *activate/deactivate*,
*swapon/swapoff*.

I tried searching the GNOME HIG for guidance, and one of the closest
things I found was:

  Use progressive disclosure to show controls when they are needed [1]

  Showing every possible control all the time makes an application
  harder to use, since users have to navigate controls that are often
  not relevant. Instead, only show controls when they are needed. This
  makes applications simpler to use, even if the same amount of
  functionality is provided.

  There are various ways to progressively disclose controls, from
  using different views or modes, to showing transient or floating
  controls when particular content items are selected.

[1] https://developer.gnome.org/hig/stable/design-principles.html.en

To me this implies we only show *Volume Group* when it is applicable.
What is unclear is if it is okay to hide *Device* and show *Volume
Group* in it's place.

Thinking further on this, when a user has selected a VG then the
Device options are no longer applicable.  If we do hide *Device* and
show *Volume Group* then that could be considered alignment with "only
show controls when they are needed*.

If we assume that it is okay to dynamically change menu items on the
fly, then I am in agreement with the 9 items mentioned in comment #12.


Wrolf,

If you are following this then we'd appreciate your thoughts too.

Curtis

Created attachment 374208 [details]
Montage of screenshots displaying a VG

Hi Curtis,

What should an inactive LVM Volume Group look like?

Note that you can use LVM tools to query the names and sizes of the
Logical Volumes within an inactive VG.  It is just that none of the LVs
are accessible so there is no looking at the contained file systems or
their usage.

Attached is a screenshot montage:
1. Active VG with file systems mounted
2. Inactive VG showing LV names and sizes
3. Inactive VG showing LVs

Thanks,
Mike

P.S. The screen shots are smoke and mirrors.  I am just exploring what
needs doing and not necessarily committed to producing working code at
this point in time.

Screenshot 3 description correction:
3. Inactive VG appearing empty

Hi Mike,

Thanks for asking.  I think it is helpful to users to display more information about the LVs, even if inactive.  My vote would be for option 2.  This way the user at least knows the number of LVs, their names and sizes.

Regarding the inactive LV colour it might be better to pick a shade of grey that we have not already used because we use black for other indications (Unknown, Unformatted, Cleared).

Thanks,
Curtis

Created attachment 374248 [details] [review]
Show LVM2 VGs and LVs (draft 2)

Hi Curtis,

Here is draft 2 of the code.  It has most of the UI changes I think are
necessary.  Feel free to look and use it and provide feedback on the UI.

The only UI change it doesn't have is adding separators into
GParted > Devices radio selector and the combo box selector tool.  That
will take a bit of work because the code currently assumes that the same
integer will index Win_GParted::devices and both the afore mentioned
device selectors.  Adding a separator breaks that assumption.

The following file system only operations already work: Check, Label,
UUID and Information.  These additional operations are planned to work
inside LVs too: Format and Copy/Paste; however they don't currently
because signature erasing and block copy are coded to use libparted API
calls to read and write data from the whole disk device when working
with partitions.  Obviously this won't work on an LV.  So far I am
thinking of re-writing them to use POSIX I/O on the partition device
directly instead.

There is another significant issue too.  FS usage information breaks
down for LVs.  So far the code works by making the VG and LV "sector
size" the extent size.  However that means all FS usage figures work in
units of 4 MiB, the default VG extent size.  I think that there will
need to be a new allocation unit (extent) size to handle this.

So the UI is close to what I think it will look like, but the code has a
long way to go.  Take your time with any feedback on the UI, I have
plenty of other code to work on as outlined above.

Thanks,
Mike

bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time.

If you still use gparted and if you still see this bug / want this feature in a recent and currently supported version, then please feel free to report it at
https://gitlab.gnome.org/GNOME/gparted/-/issues/
by following the guidelines at
https://wiki.gnome.org/Community/GettingInTouch/BugReportingGuidelines

Thank you for creating this report and we are sorry it could not be implemented so far (volunteer workforce and time is limited).