Fix wrong fs label as partition label #316

Itxaka · 2022-05-05T13:46:17Z

The label entry in the Partition struct should correspond to the
partition label, but we were getting the FS label instead.

This patch fixes that by getting the actual partition label for the
label value and adding an extra field on the Partition struct to add the
fs label, so they are separated and clear on which is which.

Signed-off-by: Itxaka [email protected]

The label entry in the Partition struct should correspond to the partition label, but we were getting the FS label instead. This patch fixes that by getting the actual partition label for the label value and adding an extra field on the Partition struct to add the fs label, so they are separated and clear on which is which. Signed-off-by: Itxaka <[email protected]>

Itxaka · 2022-05-05T13:49:25Z

For me it makes sense that the entry at Partition.Label refers to the actual label of the partition itself, if it has. And FsLabel is for the FS that the partition has and can change if the partition is reformatted. The partition label wont change in that case.

Sorry about this :)

Itxaka · 2022-05-05T14:18:51Z

This breaks the API by changing the underlying value, unfortunately.

Another proposal could be adding 2 new fields, FSLabel and PartLabel and filling those correctly while deprecating the Label field but keeping it like it currently is, so it can be dropped in a couple of versions while we transition.

Itxaka · 2022-05-05T17:38:06Z

Although one could argue that the API is not really broken, we are fixing the value that it returns because its not really correct and providing another way of getting the older value provided by it.

It would be good to get a look at this asap, as this was recently released (the new Partition.Label entry) so the time to fix it is now, before it can be used generally by other projects cc @jaypipes @fromanirh :)

ffromani · 2022-05-06T14:18:12Z

Although one could argue that the API is not really broken, we are fixing the value that it returns because its not really correct and providing another way of getting the older value provided by it.

It would be good to get a look at this asap, as this was recently released (the new Partition.Label entry) so the time to fix it is now, before it can be used generally by other projects cc @jaypipes @fromanirh :)

I'll review shortly (ETA worst case 20220509)

jaypipes · 2022-05-06T14:20:12Z

pkg/block/block_linux.go

+		return util.UNKNOWN
+	}
+
+	if label, ok := info["ID_PART_ENTRY_NAME"]; ok {


@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?

@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.

@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.

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

ffromani · 2022-05-06T14:19:45Z

pkg/block/block.go

@@ -183,6 +183,7 @@ type Partition struct {
 	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
+	FSLabel    string


missing json tags - should probably add json:"fs_label" or perhaps we want to go as far as json:"filesystem_label"? (I think the former is good)

yep, didnt want to add anything here yet as the values can change. Will make sure to add the json tags once we agreed what fields goes where 👍

Fine for me, thanks!

@Itxaka @fromanirh OK, good with me to have a separate field then :) Can we name it FilesystemLabel and json:"filesystem_label" though?

you wish is my command, behold a new commit!

jaypipes · 2022-05-10T13:37:48Z

pkg/block/block.go

@@ -183,6 +183,7 @@ type Partition struct {
 	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
+	FSLabel    string


@Itxaka @fromanirh OK, good with me to have a separate field then :) Can we name it FilesystemLabel and json:"filesystem_label" though?

Signed-off-by: Itxaka <[email protected]>

ffromani · 2022-05-10T14:12:23Z

LGTM! thanks a lot!

jaypipes

perfect :) thanks @Itxaka!

Itxaka mentioned this pull request May 5, 2022

ghw does not fetch partition labels rancher/elemental-cli#254

Closed

jaypipes requested review from ffromani and jaypipes May 6, 2022 14:16

jaypipes reviewed May 6, 2022

View reviewed changes

ffromani reviewed May 6, 2022

View reviewed changes

Itxaka requested review from jaypipes and ffromani May 9, 2022 08:04

jaypipes requested changes May 10, 2022

View reviewed changes

Address feedback

3072245

Signed-off-by: Itxaka <[email protected]>

Itxaka requested a review from jaypipes May 10, 2022 14:01

jaypipes approved these changes May 10, 2022

View reviewed changes

jaypipes merged commit 94dcba5 into jaypipes:main May 10, 2022

Itxaka mentioned this pull request Mar 9, 2023

Bump ghw and fix state kairos-io/kairos#1073

Merged

Itxaka mentioned this pull request May 5, 2023

QA kcrypt on kairos 1.5 and 1.6 kairos-io/kairos#1248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong fs label as partition label #316

Fix wrong fs label as partition label #316

Itxaka commented May 5, 2022

Itxaka commented May 5, 2022

Itxaka commented May 5, 2022

Itxaka commented May 5, 2022

ffromani commented May 6, 2022

jaypipes May 6, 2022

ffromani May 6, 2022

Itxaka May 9, 2022

ffromani May 9, 2022

ffromani May 6, 2022

Itxaka May 9, 2022

ffromani May 9, 2022

jaypipes May 10, 2022

Itxaka May 10, 2022

jaypipes May 10, 2022

ffromani commented May 10, 2022

jaypipes left a comment

Fix wrong fs label as partition label #316

Fix wrong fs label as partition label #316

Conversation

Itxaka commented May 5, 2022

Itxaka commented May 5, 2022

Itxaka commented May 5, 2022

Itxaka commented May 5, 2022

ffromani commented May 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani commented May 10, 2022

jaypipes left a comment

Choose a reason for hiding this comment