[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: confusing message patch
From: |
K.G. |
Subject: |
Re: confusing message patch |
Date: |
Mon, 24 Oct 2005 20:08:39 +0200 |
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, §or_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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: confusing message patch,
K.G. <=