Skip to content

Conversation

@jjmcdn
Copy link

@jjmcdn jjmcdn commented Sep 25, 2020

I found these when I was working with a USB stick with multiple FAT32 partitions and I wanted to resize the last one to fill the remainder of the space on the flash drive. Once merged, these allow calls like the following to work:

# fatresize -n 3 -s 23415733760 /dev/sda
# fatresize -n 3 -s 23416MB /dev/sda
# fatresize -s 23415733760 /dev/sda3

The first two would previously fail with the assertion noted in the commit log while the third would complain about overlapping partitions because it would incorrectly deduce that the first partition was the one to be resized since it also happened to match the other conditions (it is also a fat32 partition and devname was modified during use).

By the time get_partnum() is called here, devname may have been modified
to it no longer ends with the partition number but instead is a
null-terminated device name.  That is, dev may still be '/dev/sda3' but
devname is now '/dev/sda'.

Additionally, the '-n' parameter hasn't been respected, with get_partnum()
consistently returning '1' regardless of the actual partition specified on
the command line.

Only attempt to deduce the partition number if it wasn't already
explicitly stated on the command line.  This would also be essential if
mmc devices should be supported in the future as they're frequently named
mmcblk0p1 where a partition name isn't an int but a string.

Signed-off-by: Joe MacDonald <joe_macdonald@mentor.com>
Under certain circumstances we cannot accommodate the size request:  the
specified size is either greater than or exactly equal to the available
length.  This is very likely to happen if the user is specifying less
precise size values such as MiB, where the number argument is multiplied
by a factor or three.  Limit this to the actual partition length.

It also happens that if you specify the precise number of bytes free, you
also, you'll hit an assertion in ped_geometry_test_sector_inside() so
we'll bump the request down by one sector and run with that.

Signed-off-by: Joe MacDonald <joe_macdonald@mentor.com>
opts.size = constraint->max_size * dev->sector_size;
ped_constraint_destroy(constraint);
}
if (opts.size >= dev->sector_size * part_geom.length) {
Copy link

@crass crass Sep 3, 2021

Choose a reason for hiding this comment

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

Currently fatresize has two thing that it does, one is obvious, resizing a FAT filesystem, and the other less obvious, resizing the partition on whch a FAT filesystem resides. This would effectively remove the capability to grow the partition, which I actually think would be a good thing.

@crass
Copy link

crass commented Sep 3, 2021

I'm currently hitting some bugs that both these patches solve. The first is that fatresize -s max -p -n3 /dev/mmcblk0p3 fails because the -n3 is ignored and the partition num set to 0. This can be worked around by changing the order of the options, eg. fatresize -s max -p /dev/mmcblk0p3 -n3, which resets the opts.pnum after erroneously setting it in get_device.

The second is that ped_file_system_get_resize_constraint in libparted isn't very smart and will always return a constraint where max_size doesn't account for partitions after the partition that the PedFileSystem resides on.

As noted though, the second commit removes a not well documented "feature" of fatresize, namely the ability to grow the partition that the FAT filesystem resides on. This feature is not without its bugs though. For instance, using -s max to grow a partition that is not the last partition on disk will fail with a message about overlapping partitions (due to my second issue above).

Can we at least get the first patch merged?

@wdlkmpx
Copy link

wdlkmpx commented Apr 14, 2023

Do you know a fork where pending patches known to fix and improve stuff are already integrated?

@intrigeri
Copy link

Hi, FWIW at Tails we've been applying a workaround since a few years, that we could ditch once the bug fixed by this MR is fixed.

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.

4 participants