dcantrell / pyparted

Python bindings for GNU parted (libparted)
GNU General Public License v2.0
86 stars 43 forks source link

pyparted should support fixing GPT labels #61

Closed rlaager closed 5 years ago

rlaager commented 5 years ago

With GPT, there is a second copy of the partition table stored at the end of the disk. So when you expand the disk, you need to adjust the GPT label. Parted prompts you to fix the label when run interactively:

$ sudo parted example.img
...
(parted) p                                                               
Warning: Not all of the space available to example.img
appears to be used, you can fix the GPT to use all of the space (an extra 9765625 blocks) or continue
with the current setting?
Fix/Ignore?

Unfortunately, from what I can see, it is not possible to invoke this via pyparted. When this situation is detected, libparted throws an exception, which the parted UI catches and presents to the user. If they choose to fix it, then libparted fixes the GPT: https://github.com/bcl/parted/blob/master/libparted/labels/gpt.c#L768

pyparted registers its exception handler here: https://github.com/dcantrell/pyparted/blob/master/src/_pedmodule.c#L770

and here is that handler: https://github.com/dcantrell/pyparted/blob/master/src/_pedmodule.c#L421

From the first link (the libparted code), we can see the exception in question is a PED_EXCEPTION_WARNING. Unless the exception has PED_EXCEPTION_YES_NO set in the options (which this one doesn't, as it offers PED_EXCEPTION_FIX), the handler returns PED_EXCEPTION_IGNORE.

It would be nice if pyparted had some way to deal with this.

dcantrell commented 5 years ago

This is entirely reasonable. I will see what I can do.

dcantrell commented 5 years ago

I've pushed a commit to try and address this limitation. The Fix/Ignore exception will be pushed back up in a way similar to yes/no. Can you try out master and see if it works for you?

rlaager commented 5 years ago

Thanks! I am on vacation. I will retest when I get home and let you know. This will be a nice improvement as we can avoid a workaround of shelling out to sgdisk.

rlaager commented 5 years ago

This works with the following change:

--- a/src/_pedmodule.c
+++ b/src/_pedmodule.c
@@ -428,7 +428,7 @@
         case PED_EXCEPTION_WARNING:
             if (e->options == PED_EXCEPTION_YES_NO) {
                 ret = PED_EXCEPTION_NO;
-            } else if (e->options == PED_EXCEPTION_FIX) {
+            } else if (e->options & PED_EXCEPTION_IGNORE) {
                 ret = PED_EXCEPTION_IGNORE;
             } else {
                 partedExnRaised = 0;

There are two changes here:

  1. You were comparing for direct equality against PED_EXCEPTION_FIX, but e->options is actually PED_EXCEPTION_FIX | PED_EXCEPTION_IGNORE, so it needs to be a bitwise AND check.
  2. I changed PED_EXCEPTION_FIX to PED_EXCEPTION_IGNORE. In practice, I would expect that PED_EXCEPTION_IGNORE will always be available with PED_EXCEPTION_FIX, but it seemed more technically correct in theory to only set PED_EXCEPTION_IGNORE if PED_EXCEPTION_IGNORE was a choice. If you disagree, this part of the change is optional.

For reference, here is our example exception handler that accepts all fixes from libparted:

def parted_exception_handler(exception_type, options, message):
    if options & parted.EXCEPTION_RESOLVE_FIX:
        print("Fixing")
        return parted.EXCEPTION_RESOLVE_FIX
    return parted.EXCEPTION_RESOLVE_UNHANDLED

parted.register_exn_handler(parted_exception_handler)

To help with my planning (e.g. for backporting a fix), do you intend to release a 3.11.3 with this fix soon? (If not, I'll just backport from master, which is fine too.)

dcantrell commented 5 years ago

Very nice feedback, thank you. Good catch on the bitwise and failure. I also agree with your change to PED_EXCEPTION_IGNORE given the existing library behavior. I don't want to overthink this too much and just follow what libparted is more or less doing. I had used PED_EXCEPTION_FIX because of my incorrect reading that it was a constant rather than a set of flags.

Commit coming soon. I am waiting on feedback for some other changes on master before I do a new release. Possibly a few more weeks.

rlaager commented 5 years ago

That change fixes fixing GPT labels when the disk has been expanded. Unfortunately, when a disk is shrunk, I can't seem to get it to fix anything. Unfortunately, that seems to be an issue in parted too:

$ sudo parted /mnt/krls1/virtualmachines/os/rabbit.wiktel.com-os.img
GNU Parted 3.2
Using /mnt/krls1/virtualmachines/os/rabbit.wiktel.com-os.img
Welcome to GNU Parted! Type 'help' to view a list of commands.
(parted) p                                                                
Error: end of file while reading
/mnt/krls1/virtualmachines/os/rabbit.wiktel.com-os.img
Retry/Ignore/Cancel? I                                                    
Error: The backup GPT table is corrupt, but the primary appears OK, so that will
be used.
OK/Cancel? O                                                              
Model:  (file)
Disk /mnt/krls1/virtualmachines/os/rabbit.wiktel.com-os.img: 10.0GB
Sector size (logical/physical): 512B/512B
Partition Table: unknown
Disk Flags: 
(parted) p                                                                
Error: end of file while reading
/mnt/krls1/virtualmachines/os/rabbit.wiktel.com-os.img
Retry/Ignore/Cancel? I                                                    
Error: The backup GPT table is corrupt, but the primary appears OK, so that will
be used.
OK/Cancel? O                                                              
Model:  (file)
Disk /mnt/krls1/virtualmachines/os/rabbit.wiktel.com-os.img: 10.0GB
Sector size (logical/physical): 512B/512B
Partition Table: unknown
Disk Flags: 
(parted) q