Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix wrong fs label as partition label #316

Merged
merged 2 commits into from
May 10, 2022
Merged

Fix wrong fs label as partition label #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9d7888
Fix wrong fs label as partition label
May 5, 2022
3072245
Address feedback
May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions pkg/block/block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,15 @@ type Disk struct {

// Partition describes a logical division of a Disk.
type Partition struct {
Disk *Disk `json:"-"`
Name string `json:"name"`
Label string `json:"label"`
MountPoint string `json:"mount_point"`
SizeBytes uint64 `json:"size_bytes"`
Type string `json:"type"`
IsReadOnly bool `json:"read_only"`
UUID string `json:"uuid"` // This would be volume UUID on macOS, PartUUID on linux, empty on Windows
Disk *Disk `json:"-"`
Name string `json:"name"`
Label string `json:"label"`
MountPoint string `json:"mount_point"`
SizeBytes uint64 `json:"size_bytes"`
Type string `json:"type"`
IsReadOnly bool `json:"read_only"`
UUID string `json:"uuid"` // This would be volume UUID on macOS, PartUUID on linux, empty on Windows
FilesystemLabel string `json:"filesystem_label"`
}

// Info describes all disk drives and partitions in the host system.
Expand Down
30 changes: 22 additions & 8 deletions pkg/block/block_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,21 +214,23 @@ func diskPartitions(ctx *context.Context, paths *linuxpath.Paths, disk string) [
if pt == "" {
pt = diskPartTypeUdev(paths, disk, fname)
}
fsLabel := diskFSLabel(paths, disk, fname)
p := &Partition{
Name: fname,
SizeBytes: size,
MountPoint: mp,
Type: pt,
IsReadOnly: ro,
UUID: du,
Label: label,
Name: fname,
SizeBytes: size,
MountPoint: mp,
Type: pt,
IsReadOnly: ro,
UUID: du,
Label: label,
FilesystemLabel: fsLabel,
}
out = append(out, p)
}
return out
}

func diskPartLabel(paths *linuxpath.Paths, disk string, partition string) string {
func diskFSLabel(paths *linuxpath.Paths, disk string, partition string) string {
info, err := udevInfoPartition(paths, disk, partition)
if err != nil {
return util.UNKNOWN
Expand All @@ -240,6 +242,18 @@ func diskPartLabel(paths *linuxpath.Paths, disk string, partition string) string
return util.UNKNOWN
}

func diskPartLabel(paths *linuxpath.Paths, disk string, partition string) string {
info, err := udevInfoPartition(paths, disk, partition)
if err != nil {
return util.UNKNOWN
}

if label, ok := info["ID_PART_ENTRY_NAME"]; ok {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Itxaka what would you think about leaving only Partition.Label field and simply looking up ID_PART_ENTRY_NAME first and then using ID_FS_LABEL if and only if ID_PART_ENTRY_NAME is empty?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaypipes I don't really have strong feelings here, but I do have a feeble feeling ( :) ) that conflating these two different labels in the same field feels a bit funky. OTOH avoiding to expand the API surface also makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Itxaka what would you think about leaving only Partition.Label field and simply looking up ID_PART_ENTRY_NAME first and then using ID_FS_LABEL if and only if ID_PART_ENTRY_NAME is empty?

I would not do that as they are different values and can be set by different things, so it doesnt make sense to mix them together, that can lead to mistakes happening. i.e. If I format a partition with a label X and then search for the partition label I would get X which is not necessarily the label of the partition.

that conflating these two different labels in the same field feels a bit funky.

Agreed, they are different things and should be separated. They can be different and should be reported as different IMHO, as one is a partition label and the other a FS label.

Note that if your FS is formatted, the label will be erased but not the partition label, so its very helpful to have both.

OTOH avoiding to expand the API surface also makes sense.

I understand not wanting to expand the API surface, but this is a valuable information to add, especially with it already reporting the partition type, its a nice thing to also have the FS label and partition as you can identify partitions by any of those values.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Itxaka yes, I'm now pretty much convinced we should have two separate fields for these labels.

return label
}
return util.UNKNOWN
}

// diskPartTypeUdev gets the partition type from the udev database directly and its only used as fallback when
// the partition is not mounted, so we cannot get the type from paths.ProcMounts from the partitionInfo function
func diskPartTypeUdev(paths *linuxpath.Paths, disk string, partition string) string {
Expand Down
33 changes: 32 additions & 1 deletion pkg/block/block_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func TestDiskPartLabel(t *testing.T) {
_ = os.Mkdir(filepath.Join(paths.SysBlock, "sda"), 0755)
_ = os.Mkdir(filepath.Join(paths.SysBlock, "sda", "sda1"), 0755)
_ = ioutil.WriteFile(filepath.Join(paths.SysBlock, "sda", "sda1", "dev"), []byte("259:0\n"), 0644)
_ = ioutil.WriteFile(filepath.Join(paths.RunUdevData, "b259:0"), []byte(fmt.Sprintf("E:ID_FS_LABEL=%s\n", partLabel)), 0644)
_ = ioutil.WriteFile(filepath.Join(paths.RunUdevData, "b259:0"), []byte(fmt.Sprintf("E:ID_PART_ENTRY_NAME=%s\n", partLabel)), 0644)
label := diskPartLabel(paths, "sda", "sda1")
if label != partLabel {
t.Fatalf("Got label %s but expected %s", label, partLabel)
Expand All @@ -226,6 +226,37 @@ func TestDiskPartLabel(t *testing.T) {
}
}

func TestDiskFSLabel(t *testing.T) {
if _, ok := os.LookupEnv("GHW_TESTING_SKIP_BLOCK"); ok {
t.Skip("Skipping block tests.")
}
baseDir, _ := ioutil.TempDir("", "test")
defer os.RemoveAll(baseDir)
ctx := context.New()
ctx.Chroot = baseDir
paths := linuxpath.New(ctx)
fsLabel := "TEST_LABEL_GHW"

_ = os.MkdirAll(paths.SysBlock, 0755)
_ = os.MkdirAll(paths.RunUdevData, 0755)

// Emulate a disk with one partition with label TEST_LABEL_GHW
_ = os.Mkdir(filepath.Join(paths.SysBlock, "sda"), 0755)
_ = os.Mkdir(filepath.Join(paths.SysBlock, "sda", "sda1"), 0755)
_ = ioutil.WriteFile(filepath.Join(paths.SysBlock, "sda", "sda1", "dev"), []byte("259:0\n"), 0644)
_ = ioutil.WriteFile(filepath.Join(paths.RunUdevData, "b259:0"), []byte(fmt.Sprintf("E:ID_FS_LABEL=%s\n", fsLabel)), 0644)
label := diskFSLabel(paths, "sda", "sda1")
if label != fsLabel {
t.Fatalf("Got label %s but expected %s", label, fsLabel)
}

// Check empty label if not found
label = diskFSLabel(paths, "sda", "sda2")
if label != util.UNKNOWN {
t.Fatalf("Got label %s, but expected %s label", label, util.UNKNOWN)
}
}

func TestDiskTypeUdev(t *testing.T) {
if _, ok := os.LookupEnv("GHW_TESTING_SKIP_BLOCK"); ok {
t.Skip("Skipping block tests.")
Expand Down