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.

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

actions/image-partition: check label length in verify stage #269

Conversation

vigneshraman
Copy link
Contributor

@vigneshraman vigneshraman commented Jul 23, 2021

The partition name (label) can be up to 11 characters long for vfat
and 16 characters long for ext3/4.

When partition name length is longer than 11 characters for vfat,
it fails with below message,
==== image-partition ====
cmdline: [mkfs.vfat -F32 -n efi456789012 /dev/vda1]
Formatting partition 1 | mkfs.vfat: Label can be no longer than 11 characters
Formatting partition 1 | mkfs.fat 4.2 (2021-01-31)
Action image-partition failed at stage Run, error: exit status 1

When partition name length is longer than 16 characters for ext2/3/4,
it shows the below warning message,
cmdline: [mkfs.ext4 -L root567890123456789 /dev/vda2]
Formatting partition 2 | Warning: label too long; will be truncated to 'root567890123456'

In case the partition name is longer than 11 for vfat, name is truncated
to 11 characters and a warning message is reported. In case of ext3/4,
it is truncated to 16 characters.

Originally-by: Christopher Obbard [email protected]
Signed-off-by: Vignesh Raman [email protected]

@obbardc
Copy link
Member

obbardc commented Jul 23, 2021

rather than exiting, would it be better to trim the label and report a warning?
also, the docs will probably need to be changed.

@vigneshraman
Copy link
Contributor Author

vigneshraman commented Jul 23, 2021

rather than exiting, would it be better to trim the label and report a warning?

@obbardc, yes it would be better to trim and report warning in case of vfat. For ext3/4, the label will be truncated to 16 characters by mkfs.ext4 and a warning is reported. So we can ignore ext3/4 case ?

also, the docs will probably need to be changed.

Sure, will check this. Thanks.

@vigneshraman vigneshraman force-pushed the vigneshraman/debos/issues/251 branch from 3e00257 to d55b3a9 Compare July 23, 2021 09:56
@@ -669,6 +670,13 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error {
for pidx, _ := range i.Partitions {
p := &i.Partitions[pidx]
if m.Partition == p.Name {
if p.FS == "vfat" {
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't check that here, you should check it after: for idx, _ := range i.Partitions {

you should probably also determine the label there, something like (untested):

for idx, _ := range i.Partitions {
		if p.Name == "" {
			return fmt.Errorf("Partition without a name")
		}

		// moved this code from the Run() function...
		if p.PartitionType == "gpt" {
			if p.PartLabel == "" {
				p.PartLabel = p.Name
			}

			var maxLength int = 0
			switch p.FS {
				case "vfat":
					maxLength = 11
				case "ext2":
					maxLength = 16
				case "ext3":
					maxLength = 16
			}
			if maxLength > 0 {
					truncated := p.PartLabel[0:maxLength]
					log.Printf("Warning: partition label for %s '%s' is too long; truncated to '%s'", p.Name, p.PartLabel, truncated)
					p.PartLabel = truncated
			}
		}

		// check for duplicate partition names
		for j := idx + 1; j < len(i.Partitions); j++ {
			if i.Partitions[j].Name == p.Name {
				return fmt.Errorf("Partition %s already exists", p.Name)
			}
		}
}

Copy link
Contributor Author

@vigneshraman vigneshraman Jul 26, 2021

Choose a reason for hiding this comment

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

@obbardc, this was added here since we need to check both m.Partition and p.Name. Can we do the above check before if m.Partition == p.Name check ? Do we need to truncate m.Partition also ?

The partition name (label) can be up to 11 characters long for vfat
and 16 characters long for ext3/4.

When partition name length is longer than 11 characters for vfat,
it fails with below message,
==== image-partition ====
cmdline: [mkfs.vfat -F32 -n efi456789012 /dev/vda1]
Formatting partition 1 | mkfs.vfat: Label can be no longer than 11 characters
Formatting partition 1 | mkfs.fat 4.2 (2021-01-31)
Action `image-partition` failed at stage Run, error: exit status 1

When partition name length is longer than 16 characters for ext2/3/4,
it shows the below warning message,
cmdline: [mkfs.ext4 -L root567890123456789 /dev/vda2]
Formatting partition 2 | Warning: label too long; will be truncated to 'root567890123456'

In case the partition name is longer than 11 for vfat, name is truncated
to 11 characters and a warning message is reported. In case of ext3/4,
it is truncated to 16 characters.

Originally-by: Christopher Obbard <[email protected]>
Signed-off-by: Vignesh Raman <[email protected]>
@vigneshraman vigneshraman force-pushed the vigneshraman/debos/issues/251 branch from d55b3a9 to efe796f Compare July 26, 2021 08:59
@@ -668,6 +690,7 @@ func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error {

for pidx, _ := range i.Partitions {
p := &i.Partitions[pidx]

Copy link
Member

Choose a reason for hiding this comment

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

spurious change

p.PartLabel = p.Name
}

switch p.FS {
Copy link
Member

Choose a reason for hiding this comment

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

also, can we have a look at the maximum lengths for the other filesystems ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will check for btrfs, xfs, reiserfs, jfs.

@@ -54,7 +54,8 @@ Mandatory properties:

- name -- is used for referencing named partition for mount points
configuration (below) and label the filesystem located on this partition. Must be
unique.
unique. The partition name can be up to 11 characters long for vfat and 16 characters
long for ext2/3/4. Name should be same as partition name in mount points.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
long for ext2/3/4. Name should be same as partition name in mount points.
long for ext2/3/4. Any longer labels will be automatically truncated.

also; the length should probably be moved to the partlabel documentation

@@ -54,7 +54,8 @@ Mandatory properties:

- name -- is used for referencing named partition for mount points
configuration (below) and label the filesystem located on this partition. Must be
unique.
unique. The partition name can be up to 11 characters long for vfat and 16 characters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
unique. The partition name can be up to 11 characters long for vfat and 16 characters
unique. The partition label can be up to 11 characters long for vfat and 16 characters

Copy link
Member

@obbardc obbardc left a comment

Choose a reason for hiding this comment

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

you can also remove duplicate code in Run():

		if p.PartLabel == "" {
			p.PartLabel = p.Name
		}

@vigneshraman
Copy link
Contributor Author

Closing this PR. New proposed changes are in #271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants