Re: confusing message patch

[K.G.]

On Thu, 14 Jul 2005 14:01:31 -0400 (EDT) Chris Lumens <address@hidden> wrote:
> I've got a bug report where the reporter has a disk with a sector size
> other than 512 bytes and believes the message displayed is confusing.
> In particular, the "Ignore" option does ignore the message by continuing
> with the detected sector size, while "Cancel" doesn't quit at all but
> instead continues with PARTED_SECTOR_SIZE.  I tend to agree with him
> that this is pretty confusing.
> 
> My fix is something I'm not really happy with, but it's the best thing
> I've been able to come up with.  I first have an exception saying that
> parted may not work with the detected size and prompt for continue/quit.
> If they continue, I have a second exception for which sector size they
> would like to use.
> 
> Unfortunately, I can't test this since I have no disks with weird sector
> sizes.  It compiles, though.  If this looks like a good fix to everyone
> on this list, I can talk to the bug reporter to see about him testing
> it.
> 
> The bug report is:
> 
>       https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=158218
> 
> And the patch is below.

I would like to integrate this patch because I think it's obviously
better than the current code. However I've some questions.

> Index: linux.c
> ===================================================================
> RCS file: /cvsroot/parted/stable/libparted/linux.c,v
> retrieving revision 1.18
> diff -u -r1.18 linux.c
> --- linux.c   4 Jun 2005 11:14:00 -0000       1.18
> +++ linux.c   14 Jul 2005 17:55:56 -0000
> @@ -354,8 +354,9 @@
>   static int
>   _device_get_sector_size (PedDevice* dev)
>   {
> -     LinuxSpecific*  arch_specific = LINUX_SPECIFIC (dev);
> -     int             sector_size;
> +     PedExceptionOption      ex_status;
> +     LinuxSpecific*          arch_specific = LINUX_SPECIFIC (dev);
> +     int                     sector_size;
> 
>       PED_ASSERT (dev->open_count, return 0);
> 
> @@ -364,17 +365,34 @@
>       if (ioctl (arch_specific->fd, BLKSSZGET, &sector_size))
>               return PED_SECTOR_SIZE;
> 
> +     /* First ask if they even want to continue with this operation at all.
> +      * If so, ask if they want to use the device's sector size or parted's
> +      * default safe size.
> +      */
>       if (sector_size != PED_SECTOR_SIZE) {
> +             ex_status = ped_exception_throw (
> +                             PED_EXCEPTION_BUG,
> +                             PED_EXCEPTION_YES_NO,
> +                             _("The sector size on %s is %d bytes.  Parted "
> +                             "is known not to work properly with drives "
> +                             "with sector sizes other than %d bytes.  "
> +                             "Continue?"),
> +                             dev->path, sector_size, PED_SECTOR_SIZE);
> +
> +             switch (ex_status) {
> +                     case PED_EXCEPTION_UNHANDLED:
> +                             ped_exception_catch();
> +                     case PED_EXCEPTION_NO:
> +                             return 0;
> +             }
> +

Is this really necessary to call ped_exception_catch() ?

>               if (ped_exception_throw (
>                       PED_EXCEPTION_BUG,
> -                     PED_EXCEPTION_IGNORE_CANCEL,
> -                     _("The sector size on %s is %d bytes.  Parted is known "
> -                     "not to work properly with drives with sector sizes "
> -                     "other than %d bytes."),
> +                     PED_EXCEPTION_YES_NO,
> +                     _("Use the sector size on %s of %d bytes?"),

Maybe we could add "(answering no will make Parted use a sector size of %d 
bytes)"
and keep PED_SECTOR_SIZE ?

>                       dev->path,
> -                     sector_size,
> -                     PED_SECTOR_SIZE)
> -                             == PED_EXCEPTION_IGNORE)
> +                     sector_size)
> +                             == PED_EXCEPTION_YES)
>                       return sector_size;
>               else
>                       return PED_SECTOR_SIZE;


Cheers,
Guillaume Knispel