[parted-devel] [PATCH 1/2] gpt_read: reintroduce restoring the primary/backup GPT when corruption

[Phillip Susi]

On 11/13/2018 9:22 PM, Hatayama, Daisuke wrote:
> The purpose of this patch set as a whole is to deal with the regression of
> recovery of primary/backup GPTs when their corruption is detected, specifically,
> in the following path:
> 
>   else if (primary_gpt && !backup_gpt)
>     {
>       /* The primary header is ok, but backup is corrupt.  */
>       if (ped_exception_throw
>           (PED_EXCEPTION_ERROR, PED_EXCEPTION_OK_CANCEL,
>            _("The backup GPT table is corrupt, but the "
>              "primary appears OK, so that will be used."))
>           == PED_EXCEPTION_CANCEL)
>         goto error_free_gpt;
> 
>       gpt = primary_gpt;
>     }
>   else /* !primary_gpt && backup_gpt */
>     {
>       /* primary GPT corrupt, backup is ok.  */
>       if (ped_exception_throw
>           (PED_EXCEPTION_ERROR, PED_EXCEPTION_OK_CANCEL,
>            _("The primary GPT table is corrupt, but the "
>              "backup appears OK, so that will be used."))
>           == PED_EXCEPTION_CANCEL)
>         goto error_free_gpt;
> 
>       gpt = backup_gpt;
>     }
> 
> Currently, there is a question in this path but no recovery is performed even if
> we responds as yes.

You mean if no other modification is made then the gpt is not written
back out later?  I'm not sure that's a bad thing.  You don't expect a
parted print operation to modify the disk.

> On the other hand, there is no issue in another point of recovery below, where
> recovery is performed when and only when user responds as yes:

An important difference here is that the user must respond yes to cause
the disk to be modified.  In the case above, in scripting mode the lack
of any response will still use the backup.

> This patch 1/2 is essentially a revert of the commit 432a33115c50005bbe96a09d55edc7d034715ec8.
> 
> Only the concern of this patch I think is that whether or not the issue mentioned
> in the commit about unintentionally clobbering pMBR has been fixed or not. I guess
> the clobbering would be done in ped_disk_clobber() in the following path:
> 
>     gpt_read
>       ped_disk_commit_to_dev
>         ped_disk_clobber

I thought ped_disk_clobber was called when you mklabel, not every time
the table is written.

> and it looks to me that ped_disk_clobber() now avoids pMBR to clobber if it exists:
> 
>     int
>     ped_disk_clobber (PedDevice* dev)
>     {
>             PED_ASSERT (dev != NULL);
> 
>             if (!ped_device_open (dev))
>                     goto error;
> 
>             PedDiskType const *gpt = find_disk_type ("gpt");
>             PED_ASSERT (gpt != NULL);
> 
>             /* If there is a GPT table, don't clobber the protective MBR.  */
>             bool is_gpt = gpt->ops->probe (dev);
>             PedSector first_sector = (is_gpt ? 1 : 0);

That definitely looks like a dirty hack.

I seem to remember there being a patch a while back to have gpt save and
restore the existing pbmr when it writes out instead of generating a new
one each time.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://alioth-lists.debian.net/pipermail/parted-devel/attachments/20181114/4ef5d44e/attachment.sig>