DriveSetup: some new functions

[dsjonny]

I think It would be very useful if we can save/copy a disk/partition from DriveSetup. Just select a partition, and copy it to another disk or save it to a raw image.

And also it would be useful is DriveSetup would show more info (for example in an information window) about the disk/partition/filesystem. I think for the 100% size, used size, free space size, block size, is it a boot volume or not, readonly or not, mounted as readonly or not.

The size information should visible on the disk view too. If the volume is the boot volume, than it may shown different from the others on the disk map. And the readonly volumes may indicated with a lock icon.

Using hrev46281 nightly anyboot.

[dsjonny]

I have added a patch to the ticket.

What was changed: Added Disk image menu: register/unregister disk images, save partition to an image, write image to a partition. Added drop disk images to register images quickly. Added BitLocker/PGP/SafeBoot encryption detection. If only one disk is in the list, DriveSetup automatically select it. Added icons for the boot/readonly/shared/encrypted/virtual drives/partitions. Show with a different color if the drive is boot/BFS/encrypted. Indicate the used space on the disk map. Added more partition info to the Parameters column. Added free space and block size column.

And the result is:

I think #10141 can be closed.

Tested: work fine all function.

[stippi]

I was just looking to apply and checkout the patch, but there have been some changes to DriveSetup recently and it doesn't apply anymore. I am still going to read through the patch as is and give suggestions, but maybe its easier to make an updated patch on your end, thanks!

[stippi]

Hm, the patch has massive coding style problems. It is both violations against the style guide (names like "v" and "enc", wrong if/else formatting, and so on), as well as structural (mostly factoring out small functions which serve a singular purpose and are named accordingly).

I don't feel like pointing out all the violations and what I would do differently. Just supply me with an updated patch and I'll try to find the time to work over it. If you feel like it, you can look at what I changed, it should be in a separate commit I suppose, so you can look at the diff. Please don't take it the wrong way, I really appreciate all the work which went into this and want to apply it!

[dsjonny]

I have created again the patch. +Added LUKS encryption detection.

Mayet #10098 also can close as duplicate of this.

[korli]

Replying to dsjonny:

I have created again the patch. +Added LUKS encryption detection.

Coding style problems still apply on this new patch (names like "v" and "e", wrong if/else formatting). Also "register_file_disk_device" is not a valid method name, "RegisterFileDiskDevice" is.

I'm also wondering whether something is missing in the following code:

        // print the success message (get the device path) 
 	BDiskDevice device; 
 	BPath path; 
 	if (roster.GetDeviceWithID(id, &device) == B_OK 
 	  && device.GetPath(&path) == B_OK) { 
 	} else { 

AFAICS the path isn't used on success.

[dsjonny]

The "register_file_disk_device" and the code you pasted in are from the diskimage command's source code: ​http://cgit.haiku-os.org/haiku/tree/src/bin/diskimage.cpp

[stippi]

There are different coding style rules for function names when they are a class method or a (static) function by themselves. You should really read the coding style guide carefully when you want your patches in. Haiku developers really don't want to add code to existing applications that follows a different coding style or is simply not easy to read code. Clean code is easy to read and understand for people who have not written it. That means meaningful variable names and short methods that serve a single purpose. If you have long methods that do various things, it's called "spaghetti-code". Also, it is never useful to point out other places in Haiku code that could be improved or that doesn't follow the style guide. That is not a good excuse for degrading the code quality even more. Instead, it should gradrually improve over time, for example by accepting only nicely written patches.

[korli]

Replying to dsjonny:

The "register_file_disk_device" and the code you pasted in are from the diskimage command's source code: ​http://cgit.haiku-os.org/haiku/tree/src/bin/diskimage.cpp

Fine. The printf() call isn't here anymore in your patch, but the comment "print the success message" still is. I think you could just skip the part from the comment to the end of the method (except for the return statement) as it's just informative. Note that the partition list item should be updated with the mount path (if any)

[dsjonny]

Patch has been added. I hope I corrected all of the problems. And I have added a "Open with DiskProbe" menu item.

[korli]

Unfortunately, not all the problems:

• commit message: 1 line then a blank line, then the rest.
• DiskView.cpp: lines 93, 103, 107, 111, 115, 126, 130, 134, 138: the else if statement goes on the previous line.
• DiskView.cpp: line 143: "fUsed = 100.0;" goes on the next line. Same for lines 149-154, 185-187, 197, 216-218, 251-253, 341
• MainWindow.cpp: you can remove lines 431-441.
• MainWindow.cpp: lines 561, 568, 580, 587, 599, 605, 627, 650, 662, 672, 678, 700, 807, 834 too long.
• PartitionList.cpp: lines 279, 284, 289, 294, 364: the else if statement goes on the previous line (or the else alone).
• PartitionList.cpp: line 384: the statement after the if goes on the next line.

Maybe you could try ​http://cgit.haiku-os.org/haiku/tree/src/tools/checkstyle to help.

[dsjonny]

Patch has been added. Please check if everything is OK now.

For the commit message: I always write each sentense in a new line, but the got removed the [Enter] from the end of the lines. Now I have added 2 [Enter] at the end of the lines, but now there are an extra empty line between lines.

[korli]

Thanks, still occurrences in MainWindow.cpp lines 611, 612, 614, 689, 690, 692, 834, 876, 1127, 1128 and PartitionList.cpp lines 296-300, 309.

There is also a typo MainWindow.cpp line 591.

Functionnally, my remarks:

• You should also launch DiskProbe by signature and fallback to the path with PathFinder/find_path().
• On alerts, it seems there is no modal alert during the copy/save process, just before and after.
• In MainWindow.cpp line 604, the buffer is leaked. Also why are you using exceptions here? There is also no error alert on copy failure.
• Once the partition is written, is there any code to let the DiskDeviceManager refresh this partition status?
• In SaveDiskImage() and WriteDiskImage(), you should also consider checking all status returned by functions, and initializing pointers to NULL. These methods are critical and shouldn't misbehave.

[dsjonny]

Patch has been added.

(I think) I have corrected the style problems and the typo. The DiskProbe will be open by mime instead of path. Added a statusbar for saving/writing images to show the current status. Added nofitications when the image register/unregister/save/write complete.

The copy code (save/write images) comes from the Tracker's source code, I don't understand why there are exceptions.

For the other mentioned problems: I don't understand those. :(

[axeld]

In general, I'm not a fan of those icons. They aren't that meaningful, and don't really help.

Looking at the patch, I have a number of remarks:

• The encryption detection should not be duplicated, please move it into its own method that is called for the partition and its parent.
• You are appending a potentially not-null terminated buffer to a string. This may crash depending on the data you read. Since I doubt that the position of those strings actually change, you should just use memcmp() on the buffer directly instead.
• Also, when you read from a disk, always check the result.
• You should not trow from a thread function -- you never catch it, and the error is never reported. Instead, the application probably aborts.
• The {} after case go to the next line.
• The "save image" functionality is completely superfluous IMO.
• When creating a disk image, you should not write a buffer repeatedly (especially not a small buffer) -- this just takes ages. Instead, just seek to the position. This will also enable sparse file support for the file systems that support that.
• "Readonly" is not a word.

[dsjonny]

Patch has been added.

I have made only one function for the encryption detection. I have removed the "try" part. I have moved the { to the new line in the catch. I have replaced the "Readonly" with "Read only".

Sorry, but I don't understand the others.

For the icons: I have created a new screenshot:

You can see the readonly partition has a "cannot write" icon, the encrypted has a "cannot access" icon, and for the boot partition it has a "leaf" icon, as it is on the Desktop. I think these are logical.

[axeld]

I think the window is much to colorful with your changes, and the icons have the most part of the responsibility for that. Besides that, I don't think they are matching either: the expected icon for an encrypted disk would be a lock. I would expect the "cannot access" symbol when the system cannot access the disk (ie. because of a malfunction).

What exactly do you not understand?

• The new EncryptionType() method is still broken: you still append a potentially unterminated buffer to a string.
• There are still lots of coding style violations in that code. Just two examples: always use boolean comparison in if (not "if (fIcon)" but "if (fIcon != NULL)"), the inline IsEncypted() method has wrong parenthesis. Wrong indentation of the return type of EncryptionType(), inconsistent asterisk style, etc, etc, etc.
• If you return in an "if", there is no need for "else".
• Please make all methods const that can be const (like IsEncrypted(), or EncryptionType()).
• fIsBFS would be a better name.
• What does "Virtual" mean to you?
• Or "Mimes"?
• Line 560 in MainWindow.cpp looks broken.
• Still no error checking in things like WriteDiskImage() - try or not, this is not acceptable.
• Now that I see what does, it looks as superfluous as SaveDiskImage(), though. The only thing that would make some sense would be an option to create a disk image of a certain size, the rest can be removed.

In any case, thank you for your contribution - I just think that you at least need to try harder to follow our coding style to get the patch to a state where it could be applied before even looking at its functionality.

[dsjonny]

Thank you for your help.

I think I am not able to fix the mentioned problems. I am not a developer, I learned programing on my own, so many knowledge is missing for me, and I don't know anybody who can help me (in Hungarian). This is why I don't understand many things (and cannot make good questions).

Some answer:

• Virtual = the mounted/registered disk images
• Mimes = is the volume supports mimes (from ​http://cgit.haiku-os.org/haiku/tree/headers/os/storage/Volume.h at line 51)
• The line 560 code is from the diskimage's source code (​http://cgit.haiku-os.org/haiku/tree/src/bin/diskimage.cpp at line 169)

The save image is for making a disk image/copy/backup from an existing partition.

I also had an idea to make another one function: "Create disk image" to create a new, empty image with the given size.

If anybody can correct the affected problem, that would be fine.

[waddlesplash]

These improvements look nice. I'll get to cleaning the patch up someday.

[pulkomandy]

Patch rebased and migrated to Gerrit: ​https://review.haiku-os.org/#/c/haiku/+/330

[waddlesplash]

PulkoMandy has been the one to make the most revisions to the patch on Gerrit, reassigning to him.