Project

General

Profile

Actions
Copy link

Submit #3099

closed

disklabel64: Fix partition 1MiB phsycial alignment; with other minor updates and cleanups

Added by liweitianux about 7 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
dillon
Category:
Kernel
Target version:
-
Start date:
11/08/2017
Due date:
% Done:

0%

Estimated time:

Description

Hello,

According to disklabel64(8), the partitions within a slice are physically aligned to 1MiB (PALIGN_SIZE). However, there is a mistake in l64_makevirginlabel() in kern/subr_disklabel64.c, which causes the partitions are actually only 32KiB aligned. The proposed patches here fix this issue, and introduce some more updates and cleanups:

  • Calculate d_pbase and d_pstop to make them both physically aligned to 1MiB;
  • Defined BOOT2SIZE64 in sys/disklabel64.h to replace the use of 32768;
  • Reserve space for the backup label at the slice end (after d_pstop), though the backup functionality not implemented;
  • Make the "auto" disk type optional, since the disk type support is not implement;
  • Update several comments, displayed disklabel descriptions;
  • Fix two compilation warnings due to the mismatched type in strncpy();
  • Add "static" keyword; cleanup unused variables and definitions.

I also pushed these patches to my GitHub at:
https://github.com/liweitianux/dragonflybsd/tree/disklabel

Here is a comparison of the virgin labels generated:

diskinfo:
/dev/ad4s1 blksize=512 offset=0x000000007e00 size=0x002543158200 149.05 GB

Before:
------------------------------------------------------------------------------
  1. boot space: 1044992 bytes
  2. data space: 156287323 blocks # 152624.34 MB (160038219264 bytes)

boot2 data base: 0x000000001000
partitions data base: 0x000000100200
partitions data stop: 0x002543157000
backup label: 0x002543157000
total size: 0x002543158200 # 152625.34 MB
------------------------------------------------------------------------------

After:
------------------------------------------------------------------------------
  1. boot space: 1012224 bytes
  2. data space: 156286976 blocks # 152624.00 MB (160037863424 bytes)

boot2 data base: 0x000000001000
partitions data base: 0x0000000f8200
partitions data stop: 0x0025430f8200
backup label: 0x0025430f8200
total size: 0x002543158200 # 152625.34 MB
------------------------------------------------------------------------------

Thanks for reviewing these patches.

Aly

Actions
Copy link
 #1

Updated by liweitianux about 7 years ago

Hello,

I should note that there are still several remaining issues:

1. In sbin/disklabel64/disklabel64.c, The "boot1buf" and "boot2buf" are malloc'ed but seems not properly freed on errors and normal exit.

2. The disklabel64(8) man page is not update against the proposed option change ("auto" is optional), and the examples also needs update.

3. If the disk type (i.e., disktab(5)) support doesn't make sense to disklabel64, then the code and man page need further cleanups.

Cheers,
Aly

Actions
Copy link
 #2

Updated by dillon about 7 years ago

  • Status changed from New to In Progress
  • Assignee set to dillon

Nice cleanups. I see disklabel64 is still using strncpy, too... and incorrectly in fact. Those probably should be changed to snprintf() to properly guarantee zero termination of the string. Nobody uses type or packid any more but we have it in there for historical reasons.

Your work looks like it is ready to be committed. Is there anything else you would like to add before I cherry-pick it into master?

-Matt

Actions
Copy link
 #3

Updated by dillon about 7 years ago

The malloc/free stuff is less critical, since program execution is short lived. In fact its reasonable to not bother to free() things in such situations as it burns cpu cycles for no reason (not that anyone would notice). One of those things that just doesn't matter a whole lot. But to be correct, adding missing free()'s is fine.

I'll clarify my last remark. We should leave the disk type (and packname) support in, even though nobody uses it any more. There's a historical context there that might trip a program up here and there if it is missing, and it doesn't hurt anything. Well, I guess the disk type might actually be used for anyone who still has a floppy drive connected (which is hopefully nobody) <grin>.

If you want to clean up the manual page please go ahead. You don't have to submit patches via the bug system, just commit them to your tree and I will cherry pick from your GIT repo when you think it is all ready for commit. Always easier to work with GIT repos rather than manual patches :-).

-Matt

Actions
Copy link
 #4

Updated by liweitianux about 7 years ago

Hi Dillon,

Thank you for looking at my patches. I have tested these patches (full build and install), and they should be fine to be committed (except for the incorrect use of "strncpy()").

I will try to update the man page today or tomorrow, and will append a comment here if it's ready.

Cheers,
Aly

Actions
Copy link
 #5

Updated by liweitianux about 7 years ago

Hi Dillon,

I've updated the disklabel64.8 man page with some other updates/cleanups, please take a review.

I also removed the "auto" argument from several places that use the disklabel64 utility, since the "auto" is now make default and optional.

I haven't fix the "strncpy()" problem that you pointed out above.

Please check my updated GitHub branch. Thank you.

Cheers,
Aly

Actions
Copy link
 #6

Updated by liweitianux almost 7 years ago

  • File deleted (0009-diskinfo-8-Print-0x-prefix-for-hex-numbers-Change-of.patch)
Actions
Copy link
 #7

Updated by liweitianux almost 7 years ago

  • File deleted (0007-disklabel64-8-Add-various-static-keyword-Fix-compila.patch)
Actions
Copy link
 #8

Updated by liweitianux almost 7 years ago

  • File deleted (0008-disklabel64-8-free-allocated-boot1buf-on-error.patch)
Actions
Copy link
 #9

Updated by liweitianux almost 7 years ago

  • File deleted (0006-disklabel64-8-Update-displayed-label-comments-and-an.patch)
Actions
Copy link
 #10

Updated by liweitianux almost 7 years ago

  • File deleted (0005-disklabel64-8-Make-the-auto-disk-type-argument-optio.patch)
Actions
Copy link
 #11

Updated by liweitianux almost 7 years ago

  • File deleted (0004-disklabel64-8-Fix-error-message-to-indicate-no-disk-.patch)
Actions
Copy link
 #12

Updated by liweitianux almost 7 years ago

  • File deleted (0003-disklabel64-8-Clean-up-unused-definitions-and-variab.patch)
Actions
Copy link
 #13

Updated by liweitianux almost 7 years ago

  • File deleted (0001-sys-disklabel64.h-Define-BOOT2SIZE64-to-32-KB-for-bo.patch)
Actions
Copy link
 #14

Updated by liweitianux almost 7 years ago

  • File deleted (0002-kern-disklabel64-Fix-physically-alignment-to-1-MiB-f.patch)
Actions
Copy link
 #15

Updated by liweitianux almost 7 years ago

Hi Matt Dillon,

I've added 2 more commits to my branch:

1. Replace "strncpy()" with "strlcpy()", and update the packname print;
2. Improve the error message to say that disktab(5) is not yet supported, and only "auto" allowed.

Here is my GitHub branch:
https://github.com/liweitianux/dragonflybsd/tree/disklabel

It's ready to be reviewed and merged by you, thanks.

Happy New Year!

Aly

Actions
Copy link
 #16

Updated by liweitianux over 6 years ago

  • Status changed from In Progress to Resolved

Committed in 0a31961570a4f5b0eff99d4c2e54d7bc722c8c2f

Actions
Copy link

Also available in: Atom PDF