[PATCH 1/3] fdisk: use ngettext() in str_units() for plural forms

[PATCH 1/3] fdisk: use ngettext() in str_units() for plural forms

[Francesco Cosoleto]

Signed-off-by: Francesco Cosoleto <cosoleto@gmail.com>
---
 fdisk/fdisk.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fdisk/fdisk.c b/fdisk/fdisk.c
index d517d4b..5b8e24a 100644
--- a/fdisk/fdisk.c
+++ b/fdisk/fdisk.c
@@ -1542,11 +1542,11 @@ get_nonexisting_partition(int warn, int max) {
 }
 
 const char *
-str_units(int n) {	/* n==1: use singular */
-	if (n == 1)
-		return display_in_cyl_units ? _("cylinder") : _("sector");
-	else
-		return display_in_cyl_units ? _("cylinders") : _("sectors");
+str_units(int n)
+{
+	if (display_in_cyl_units)
+		return P_("cylinder", "cylinders", n);
+	return P_("sector", "sectors", n);
 }
 
 void change_units(void)
-- 
1.7.3.4

[PATCH 2/3] fdisk: avoid segfault validating a sgi label (boot/swap not set)

[Francesco Cosoleto]

swap_part or boot_part can be set to -1 when they don't exist.

Signed-off-by: Francesco Cosoleto <cosoleto@gmail.com>
---
 fdisk/fdisksgilabel.c |    8 ++++----
 fdisk/fdisksgilabel.h |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fdisk/fdisksgilabel.c b/fdisk/fdisksgilabel.c
index 091902a..002688a 100644
--- a/fdisk/fdisksgilabel.c
+++ b/fdisk/fdisksgilabel.c
@@ -262,13 +262,13 @@ sgi_get_sysid(int i)
 int
 sgi_get_bootpartition(void)
 {
-	return SSWAP16(sgilabel->boot_part);
+	return (short) SSWAP16(sgilabel->boot_part);
 }
 
 int
 sgi_get_swappartition(void)
 {
-	return SSWAP16(sgilabel->swap_part);
+	return (short) SSWAP16(sgilabel->swap_part);
 }
 
 void
@@ -513,10 +513,10 @@ verify_sgi(int verbose)
 	 * Go for details now
 	 */
 	if (verbose) {
-		if (!sgi_get_num_sectors(sgi_get_bootpartition())) {
+		if (sgi_get_bootpartition() < 0 || !sgi_get_num_sectors(sgi_get_bootpartition())) {
 			printf(_("\nThe boot partition does not exist.\n"));
 		}
-		if (!sgi_get_num_sectors(sgi_get_swappartition())) {
+		if (sgi_get_swappartition() < 0 || !sgi_get_num_sectors(sgi_get_swappartition())) {
 			printf(_("\nThe swap partition does not exist.\n"));
 		} else {
 			if ((sgi_get_sysid(sgi_get_swappartition()) != SGI_SWAP)
diff --git a/fdisk/fdisksgilabel.h b/fdisk/fdisksgilabel.h
index 1445fa9..1c47bb4 100644
--- a/fdisk/fdisksgilabel.h
+++ b/fdisk/fdisksgilabel.h
@@ -63,8 +63,8 @@ struct device_parameter { /* 48 bytes */
 
 typedef struct {
 	unsigned int   magic;		 /* expect SGI_LABEL_MAGIC */
-	unsigned short boot_part;        /* active boot partition */
-	unsigned short swap_part;        /* active swap partition */
+	short boot_part;		/* active boot partition */
+	short swap_part;		/* active swap partition */
 	unsigned char  boot_file[16];    /* name of the bootfile */
 	struct device_parameter devparam;	/*  1 * 48 bytes */
 	struct volume_directory {		/* 15 * 16 bytes */
-- 
1.7.3.4

[PATCH 3/3] fdisk: split delete_partition() off from dos_delete_partition()

[Francesco Cosoleto]

This accidentally fixes a mistake printing the "Partition n is deleted"
message as the 'i' variable get decremented or incremented in case of
logical partitions.

Signed-off-by: Francesco Cosoleto <cosoleto@gmail.com>
---
 fdisk/fdisk.c |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/fdisk/fdisk.c b/fdisk/fdisk.c
index 5b8e24a..d299c4c 100644
--- a/fdisk/fdisk.c
+++ b/fdisk/fdisk.c
@@ -1584,25 +1584,16 @@ toggle_dos_compatibility_flag(void) {
 	update_sector_offset();
 }
 
-static void
-delete_partition(int i) {
+static void dos_delete_partition(int i)
+{
 	struct pte *pe = &ptes[i];
 	struct partition *p = pe->part_table;
 	struct partition *q = pe->ext_pointer;
 
-/* Note that for the fifth partition (i == 4) we don't actually
- * decrement partitions.
- */
-
-	if (warn_geometry())
-		return;		/* C/H/S not set */
-	pe->changed = 1;
+	/* Note that for the fifth partition (i == 4) we don't actually
+	   decrement partitions. */
 
-	if (disklabel == SUN_LABEL)
-		sun_delete_partition(i);
-	else if (disklabel == SGI_LABEL)
-		sgi_delete_partition(i);
-	else if (i < 4) {
+	if (i < 4) {
 		if (IS_EXTENDED (p->sys_ind) && i == ext_index) {
 			partitions = 4;
 			ptes[ext_index].ext_pointer = NULL;
@@ -1646,6 +1637,23 @@ delete_partition(int i) {
 			/* the only logical: clear only */
 			clear_partition(ptes[i].part_table);
 	}
+}
+
+static void
+delete_partition(int i)
+{
+	if (warn_geometry())
+		return;		/* C/H/S not set */
+
+	ptes[i].changed = 1;
+
+	if (disklabel == DOS_LABEL)
+		dos_delete_partition(i);
+	else if (disklabel == SUN_LABEL)
+		sun_delete_partition(i);
+	else if (disklabel == SGI_LABEL)
+		sgi_delete_partition(i);
+
 	printf(_("Partition %d is deleted\n"), i + 1);
 }
 
-- 
1.7.3.4

Re: [PATCH 1/3] fdisk: use ngettext() in str_units() for plural forms

[Karel Zak]

On Mon, Nov 14, 2011 at 02:47:17PM +0100, Francesco Cosoleto wrote:
>  fdisk/fdisk.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/3] fdisk: avoid segfault validating a sgi label (boot/swap not set)

[Karel Zak]

On Mon, Nov 14, 2011 at 02:47:18PM +0100, Francesco Cosoleto wrote:
> swap_part or boot_part can be set to -1 when they don't exist.

 Applied, thanks... but, how do you know that the number could be
 smaller than zero? Do you have any example/code?

>  int
>  sgi_get_swappartition(void)
>  {
> -	return SSWAP16(sgilabel->swap_part);
> +	return (short) SSWAP16(sgilabel->swap_part);
>  }

 It would be nice to cleanup whole fdisksgilabel.c and use
 be32_to_cpu() and be16_to_cpu() there. (See include/bitops.h).

 [...]

>  typedef struct {
>  	unsigned int   magic;		 /* expect SGI_LABEL_MAGIC */
> -	unsigned short boot_part;        /* active boot partition */
> -	unsigned short swap_part;        /* active swap partition */
> +	short boot_part;		/* active boot partition */
> +	short swap_part;		/* active swap partition */

 I prefer stdint.h types (e.g. int16_t) for things like on-disk
 labels/superblocks. But it's nothing urgent... whole fdisk should be
 refactored one day ;-)
 
 See for example libblkid/src/partitions/sgi.c. (Well, my long-term
 goal is to use libblkid for partition tables parsing in fdisk.)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 3/3] fdisk: split delete_partition() off from dos_delete_partition()

[Karel Zak]

On Mon, Nov 14, 2011 at 02:47:19PM +0100, Francesco Cosoleto wrote:
>  fdisk/fdisk.c |   36 ++++++++++++++++++++++--------------
>  1 files changed, 22 insertions(+), 14 deletions(-)

 Good idea. Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/3] fdisk: avoid segfault validating a sgi label (boot/swap not set)

[Francesco Cosoleto]

2011/11/14 Karel Zak <kzak@redhat.com>:
>> swap_part or boot_part can be set to -1 when they don't exist.
>
> =A0Applied, thanks... but, how do you know that the number could be
> =A0smaller than zero? Do you have any example/code?

I would like to read a documentation by SGI before of editing this
code or getting access to a real machine, but I couldn't. This patch
was due to SGI labels generated by libparted, and not to be forced to
point to a partition (set to 0 -> partition 1) looks reasonable to me.
It's possible this is wrong though.

> =A0It would be nice to cleanup whole fdisksgilabel.c and use
> =A0be32_to_cpu() and be16_to_cpu() there. (See include/bitops.h).

Yes, I spotted that. SSWAP.() are redefinied in fdisksunlabel.c too.
This looks easy, even without regression tests.

> =A0I prefer stdint.h types (e.g. int16_t) for things like on-disk
> =A0labels/superblocks. But it's nothing urgent... whole fdisk should be
> =A0refactored one day ;-)
>
> =A0See for example libblkid/src/partitions/sgi.c. (Well, my long-term
> =A0goal is to use libblkid for partition tables parsing in fdisk.)

It would be nice. My fdisk goal nowadays is to improve a bit the user
interface. Don't count on me about rewriting whole fdisk. :-)

Francesco