[PATCH 3/3] fdisk: dos: always write mbr

[PATCH 3/3] fdisk: dos: always write mbr

[Davidlohr Bueso]

There's no harm when writing the MBR, changed or not. This also
allows us to get rid of the 'MBRbuffer_changed' global variable.

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 fdisks/fdisk.c         |    9 ++++++---
 fdisks/fdisk.h         |    2 --
 fdisks/fdiskdoslabel.c |   23 ++++++++++-------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/fdisks/fdisk.c b/fdisks/fdisk.c
index 7db25af..d79b55a 100644
--- a/fdisks/fdisk.c
+++ b/fdisks/fdisk.c
@@ -51,8 +51,6 @@
 
 #include "gpt.h"
 
-int MBRbuffer_changed;
-
 #define hex_val(c)	({ \
 				char _c = (c); \
 				isdigit(_c) ? _c - '0' : \
@@ -498,13 +496,18 @@ static int is_partition_table_changed(void)
 
 static void maybe_exit(int rc, int *asked)
 {
+	int i, mbr_changed = 0;
 	char line[LINE_LENGTH];
 
 	putchar('\n');
 	if (asked)
 		*asked = 0;
 
-	if (is_partition_table_changed() || MBRbuffer_changed) {
+	for (i = 0; i < 4; i++)
+		if (ptes[i].changed)
+			mbr_changed = 1;
+
+	if (is_partition_table_changed() || mbr_changed) {
 		fprintf(stderr, _("Do you really want to quit? "));
 
 		if (!fgets(line, LINE_LENGTH, stdin) || rpmatch(line) == 1)
diff --git a/fdisks/fdisk.h b/fdisks/fdisk.h
index 53ef6bc..639d7db 100644
--- a/fdisks/fdisk.h
+++ b/fdisks/fdisk.h
@@ -283,8 +283,6 @@ extern const char * str_units(int);
 
 extern sector_t get_nr_sects(struct partition *p);
 
-extern int MBRbuffer_changed;
-
 /* start_sect and nr_sects are stored little endian on all machines */
 /* moreover, they are not aligned correctly */
 static inline void
diff --git a/fdisks/fdiskdoslabel.c b/fdisks/fdiskdoslabel.c
index e813a21..f83b794 100644
--- a/fdisks/fdiskdoslabel.c
+++ b/fdisks/fdiskdoslabel.c
@@ -332,7 +332,6 @@ void dos_set_mbr_id(struct fdisk_context *cxt)
 		return;
 
 	mbr_set_id(cxt->firstsector, new_id);
-	MBRbuffer_changed = 1;
 	dos_print_mbr_id(cxt);
 }
 
@@ -798,20 +797,18 @@ static int write_sector(struct fdisk_context *cxt, sector_t secno,
 
 static int dos_write_disklabel(struct fdisk_context *cxt)
 {
-	int i, rc = 0;
+	int i, rc = 0, mbr_changed = 0;
 
-	/* MBR (primary partitions) */
-	if (!MBRbuffer_changed) {
-		for (i = 0; i < 4; i++)
-			if (ptes[i].changed)
-				MBRbuffer_changed = 1;
-	}
-	if (MBRbuffer_changed) {
+	for (i = 0; i < 4; i++)
+		if (ptes[i].changed)
+			mbr_changed = 1;
+
+	if (mbr_changed)
 		mbr_set_magic(cxt->firstsector);
-		rc = write_sector(cxt, 0, cxt->firstsector);
-		if (rc)
-			goto done;
-	}
+	rc = write_sector(cxt, 0, cxt->firstsector);
+	if (rc)
+		goto done;
+
 	/* EBR (logical partitions) */
 	for (i = 4; i < partitions; i++) {
 		struct pte *pe = &ptes[i];
-- 
1.7.9.5

Re: [PATCH 3/3] fdisk: dos: always write mbr

[Karel Zak]

On Wed, Nov 14, 2012 at 12:04:52AM -0800, Davidlohr Bueso wrote:
> There's no harm when writing the MBR, changed or not. This also

it's harm -- kernel has to parse the new PT, udev has to scan all the
stuff etc. ... many event and every event is painful if you have
complicated setup (DM, multipath, udisks, ...).

> allows us to get rid of the 'MBRbuffer_changed' global variable.

It would be nice to add to fdisk_context

    void *labeldata;

 and define in fdiskdos.h

 struct fdisk_dosdata {
    int write_status;             /* FDISKDOS_WRITE_* flags */
    ...
 }

 enum {
    FDISKDOS_WRITE_HDR     = (1 << 1)  /* header buffer modified (e.g. ID change) */
    FDISKDOS_WRITE_PT      = (1 << 2)  /* partition table modified */
    FDISKDOS_WRITE_EPT     = (1 << 3)  /* any extended partition table modified */
 };

(maybe you found better names for the macros and variables :-)


 than we can add fdisk_is_modified() and is_modified() to fdisk_label
 driver and remove all label specific junk from fdisk.c.

    Karel


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

Re: [PATCH 3/3] fdisk: dos: always write mbr

[Davidlohr Bueso]

On Wed, 2012-11-14 at 10:43 +0100, Karel Zak wrote:
> On Wed, Nov 14, 2012 at 12:04:52AM -0800, Davidlohr Bueso wrote:
> > There's no harm when writing the MBR, changed or not. This also
> 

Sorry about the long delay, life has been crazy busy - trying to squeeze
in some util-linux time :-)

> it's harm -- kernel has to parse the new PT, udev has to scan all the
> stuff etc. ... many event and every event is painful if you have
> complicated setup (DM, multipath, udisks, ...).

good point.

> 
> > allows us to get rid of the 'MBRbuffer_changed' global variable.
> 
> It would be nice to add to fdisk_context
> 
>     void *labeldata;
> 
>  and define in fdiskdos.h
> 
>  struct fdisk_dosdata {
>     int write_status;             /* FDISKDOS_WRITE_* flags */
>     ...
>  }
> 
>  enum {
>     FDISKDOS_WRITE_HDR     = (1 << 1)  /* header buffer modified (e.g. ID change) */
>     FDISKDOS_WRITE_PT      = (1 << 2)  /* partition table modified */
>     FDISKDOS_WRITE_EPT     = (1 << 3)  /* any extended partition table modified */
>  };
> 
> (maybe you found better names for the macros and variables :-)
> 
> 
>  than we can add fdisk_is_modified() and is_modified() to fdisk_label
>  driver and remove all label specific junk from fdisk.c.

Yeah, a generic pointer to label-specific data is something that we have
to do. I will take a look at this option for a v2.

Thanks,
Davidlohr