fdisk: dos: always write mbr

PATCH Applied Davidlohr Bueso 2 participants 3 messages Nov 14, 2012 – Nov 26, 2012 Util-Linux package

Share & tools

Davidlohr Bueso

author

2012-11-14 08:04 UTC #0 lore raw

PATCH 3/3 fdisk: dos: always write mbr

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 <[email protected]>

---
 fdisks/fdisk.c         |    9 ++++++---
 fdisks/fdisk.h         |    2 --
 fdisks/fdiskdoslabel.c |   23 ++++++++++-------------
 3 files changed, 16 insertions(+), 18 deletions(-)

fdisks/fdisk.c +6 -3

index 7db25af..d79b55a 100644
@@ -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)

fdisks/fdisk.h +0 -2

index 53ef6bc..639d7db 100644
@@ -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

fdisks/fdiskdoslabel.c +10 ~~-13~~

index e813a21..f83b794 100644
@@ -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];

Signature · 1 line

1.7.9.5

Karel Zak

maintainer

2012-11-14 09:44 UTC #1 lore raw

Quoted 1 line · Davidlohr Bueso

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, ...).

Quoted 1 line

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

Signature · 2 lines

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

Davidlohr Bueso

author

2012-11-26 04:23 UTC #2 lore raw

Quoted 3 lines · Karel Zak

Quoted 1 line · Davidlohr Bueso
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 :-)

Quoted 3 lines

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.

Quoted 18 lines

Quoted 2 lines
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 */

Quoted 7 lines

};
(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