Fix wrong fs label as partition label #316
Conversation
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 <igarcia@suse.com>
|
For me it makes sense that the entry at Sorry about this :) |
|
This breaks the API by changing the underlying value, unfortunately. Another proposal could be adding 2 new fields, |
|
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) |
| return util.UNKNOWN | ||
| } | ||
|
|
||
| if label, ok := info["ID_PART_ENTRY_NAME"]; ok { |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@Itxaka yes, I'm now pretty much convinced we should have two separate fields for these labels.
pkg/block/block.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
@Itxaka @fromanirh OK, good with me to have a separate field then :) Can we name it FilesystemLabel and json:"filesystem_label" though?
There was a problem hiding this comment.
you wish is my command, behold a new commit!
pkg/block/block.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
@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 <igarcia@suse.com>
|
LGTM! thanks a lot! |
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 igarcia@suse.com