Skip to contentSkip to conversation

fdisk: isolate dos label logic

PATCH In review Davidlohr Bueso 2 participants 5 messages Apr 28, 2012 – May 2, 2012 Util-Linux package

lore
Share & tools
lore

Davidlohr Bueso

author
#0 lore raw

PATCH 1/1 fdisk: isolate dos label logic

From: Davidlohr Bueso <[email protected]>

DOS specific logic is currently embedded in the heart of fdisk code. This patch separates DOS label specific code into its own file, just like the rest of the labels, leaving a more generic fdisk.c file. Most changes are just moving code from fdisk.c to fdisk.h and fdiskdoslabel.[c/h].

The only logical modification is calling dos_delete_partition() from read_extended(), instead of the generic delete_partition. This is ok since read extended is only called from a DOS context.

Signed-off-by Davidlohr Bueso <[email protected]>

---
I know. It's big and ugly, but not so evil to read. Nothing we can really do about it, it must be done eventually. This patch applies on top of the last 4 patches sent.

 fdisk/Makefile.am     |    2 +
 fdisk/fdisk.c         |  420 +------------------------------------------------
 fdisk/fdisk.h         |   64 ++++++++-
 fdisk/fdiskdoslabel.c |  323 +++++++++++++++++++++++++++++++++++++
 fdisk/fdiskdoslabel.h |   50 ++++++
 5 files changed, 446 insertions(+), 413 deletions(-)
 create mode 100644 fdisk/fdiskdoslabel.c
 create mode 100644 fdisk/fdiskdoslabel.h
fdisk/Makefile.am +2 -0
index b524f5b..7851bd1 100644
@@ -32,6 +32,8 @@ fdisk_SOURCES = \
 	fdisksgilabel.h \
 	fdisksunlabel.c \
 	fdisksunlabel.h \
+	fdiskdoslabel.c \
+	fdiskdoslabel.h \
 	partname.c \
 	$(fdisk_common) \
 	$(top_srcdir)/lib/canonicalize.c
fdisk/fdisk.c +8 -412

515 lines not on this page · raw message

fdisk/fdisk.h +63 -1

81 lines not on this page · raw message

fdisk/fdiskdoslabel.c new file +323 -0

323 lines not on this page · raw message

fdisk/fdiskdoslabel.h new file +50 -0

50 lines not on this page · raw message

Signature · 1 line
1.7.4.1
maintainer
#1 lore raw
Quoted 8 lines · Davidlohr Bueso
 fdisk/Makefile.am     |    2 +
 fdisk/fdisk.c         |  420 +------------------------------------------------
 fdisk/fdisk.h         |   64 ++++++++-
 fdisk/fdiskdoslabel.c |  323 +++++++++++++++++++++++++++++++++++++
 fdisk/fdiskdoslabel.h |   50 ++++++
 5 files changed, 446 insertions(+), 413 deletions(-)
 create mode 100644 fdisk/fdiskdoslabel.c
 create mode 100644 fdisk/fdiskdoslabel.h

Applied, but I guess it's the first step only -- for example new_partition(), add_partition(), ... still contain DOS label code. It would be nice to completely move the code to fdiskdoslabel.c before we start to work on the new internal API.

Thanks!

Karel

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
author
#2 lore raw
Quoted 10 lines · Karel Zak
Quoted 8 lines · Davidlohr Bueso
 fdisk/Makefile.am     |    2 +
 fdisk/fdisk.c         |  420 +------------------------------------------------
 fdisk/fdisk.h         |   64 ++++++++-
 fdisk/fdiskdoslabel.c |  323 +++++++++++++++++++++++++++++++++++++
 fdisk/fdiskdoslabel.h |   50 ++++++
 5 files changed, 446 insertions(+), 413 deletions(-)
 create mode 100644 fdisk/fdiskdoslabel.c
 create mode 100644 fdisk/fdiskdoslabel.h
Quoted 4 lines

Applied, but I guess it's the first step only -- for example new_partition(), add_partition(), ... still contain DOS label code. It would be nice to completely move the code to fdiskdoslabel.c before we start to work on the new internal API.

Yes, absolutely. This patch was ugly enough that I didn't want to complicate it any further :) I plan on getting rid of the rest of DOS code in fdisk.c soon.

Quoted 6 lines

Thanks!

Karel

maintainer
#3 lore raw
In reply to Davidlohr Bueso
Quoted 1 line · Davidlohr Bueso

I know. It's big and ugly, but not so evil to read.

David, test your patches! Please. This patch was completely broken.

Quoted 2 lines
fdisk/fdisk.c +0 -0

...

fdisk/fdisk.c

Quoted 7 lines
fdisk/fdisk.c +0 -7
-/*
- * Raw disk label. For DOS-type partition tables the MBR,
- * with descriptions of the primary partitions.
- */
-unsigned char *MBRbuffer;
-
-int MBRbuffer_changed;

No, in .c file has to be definition and in .h file is "extern ...".

Quoted 21 lines
fdisk/fdisk.h +3 -1
index cff6b60..2032db7 100644
@@ -87,12 +87,14 @@ extern void update_units(void);
 extern char read_chars(char *mesg);
 extern void set_changed(int);
 extern void set_all_unchanged(void);
+extern int warn_geometry(void);
+extern void warn_limits(void);
+extern void warn_alignment(void);
 
 #define PLURAL	0
 #define SINGULAR 1
 extern const char * str_units(int);
 
-extern unsigned long long get_start_sect(struct partition *p);
 extern unsigned long long get_nr_sects(struct partition *p);
 
 enum labeltype {

fdisk/fdisk.h enum labeltype { · line 117

Quoted 9 lines
fdisk/fdisk.h +6 -0
@@ -107,6 +109,66 @@ enum labeltype {
 
 extern enum labeltype disklabel;
 
+/*
+ * Raw disk label. For DOS-type partition tables the MBR,
+ * with descriptions of the primary partitions.
+ */
+int MBRbuffer_changed;
+unsigned char *MBRbuffer;

this belong to fdisk.c, to fdisk.h belong:

     extern int MBRbuffer_changed;
     extern unsigned char *MBRbuffer;

fdisk/fdisk.h enum labeltype { · line 121

Quoted 4 lines
fdisk/fdisk.h +4 -0
+/* start_sect and nr_sects are stored little endian on all machines */
+/* moreover, they are not aligned correctly */
+static void
+store4_little_endian(unsigned char *cp, unsigned int val) {

this is header file, so "static inline ..."

Karel

Quoted 1 line

From 96f817fb169ac38fe5535cb8c6d2fdf9b2cb8f10 Mon Sep 17 00:00:00 2001

From: Karel Zak <[email protected]>
Date: Wed, 2 May 2012 14:05:51 +0200
Subject: [PATCH] fdisk: fix fdiskdoslabel.c global variables

Signed-off-by Karel Zak <[email protected]>
---
 fdisk/fdisk.c         |    4 +++-
 fdisk/fdisk.h         |   21 ++++++++++-----------
 fdisk/fdiskdoslabel.c |    4 ++++
 fdisk/fdiskdoslabel.h |   14 ++++++++------
 4 files changed, 25 insertions(+), 18 deletions(-)
fdisk/fdisk.c +3 -1
index 6695266..acc84d1 100644
@@ -54,6 +54,9 @@
 
 static void delete_partition(int i);
 
+unsigned char *MBRbuffer;
+int MBRbuffer_changed;
+
 #define hex_val(c)	({ \
 				char _c = (c); \
 				isdigit(_c) ? _c - '0' : \

@@ -145,7 +148,6 @@ char	*disk_device,			/* must be specified */
 	line_buffer[LINE_LENGTH];
 
 int	fd,				/* the disk */
-	ext_index,			/* the prime extended partition */
 	nowarn = 0,			/* no warnings for fdisk -l/-s */
 	dos_compatible_flag = 0,	/* disabled by default */
 	dos_changed = 0,
fdisk/fdisk.h +10 -11
index da86e30..3a1cfd7 100644
@@ -77,7 +77,6 @@ extern unsigned int read_int(unsigned int low, unsigned int dflt,
 extern void print_menu(enum menutype);
 extern void print_partition_size(int num, unsigned long long start, unsigned long long stop, int sysid);
 
-extern unsigned char *MBRbuffer;
 extern void zeroize_mbr_buffer(void);
 
 extern unsigned int heads, cylinders, sector_size;

@@ -113,12 +112,12 @@ extern enum labeltype disklabel;
  * Raw disk label. For DOS-type partition tables the MBR,
  * with descriptions of the primary partitions.
  */
-int MBRbuffer_changed;
-unsigned char *MBRbuffer;
+extern unsigned char *MBRbuffer;
+extern int MBRbuffer_changed;
 
 /* start_sect and nr_sects are stored little endian on all machines */
 /* moreover, they are not aligned correctly */
-static void
+static inline void
 store4_little_endian(unsigned char *cp, unsigned int val) {
 	cp[0] = (val & 0xff);
 	cp[1] = ((val >> 8) & 0xff);

@@ -126,45 +125,45 @@ store4_little_endian(unsigned char *cp, unsigned int val) {
 	cp[3] = ((val >> 24) & 0xff);
 }
 
-static unsigned int read4_little_endian(const unsigned char *cp)
+static inline unsigned int read4_little_endian(const unsigned char *cp)
 {
 	return (unsigned int)(cp[0]) + ((unsigned int)(cp[1]) << 8)
 		+ ((unsigned int)(cp[2]) << 16)
 		+ ((unsigned int)(cp[3]) << 24);
 }
 
-static void set_nr_sects(struct partition *p, unsigned long long nr_sects)
+static inline void set_nr_sects(struct partition *p, unsigned long long nr_sects)
 {
 	store4_little_endian(p->size4, nr_sects);
 }
 
-static void set_start_sect(struct partition *p, unsigned int start_sect)
+static inline void set_start_sect(struct partition *p, unsigned int start_sect)
 {
 	store4_little_endian(p->start4, start_sect);
 }
 
-static void seek_sector(int fd, unsigned long long secno)
+static inline void seek_sector(int fd, unsigned long long secno)
 {
 	off_t offset = (off_t) secno * sector_size;
 	if (lseek(fd, offset, SEEK_SET) == (off_t) -1)
 		fatal(unable_to_seek);
 }
 
-static void read_sector(int fd, unsigned long long secno, unsigned char *buf)
+static inline void read_sector(int fd, unsigned long long secno, unsigned char *buf)
 {
 	seek_sector(fd, secno);
 	if (read(fd, buf, sector_size) != sector_size)
 		fatal(unable_to_read);
 }
 
-static void write_sector(int fd, unsigned long long secno, unsigned char *buf)
+static inline void write_sector(int fd, unsigned long long secno, unsigned char *buf)
 {
 	seek_sector(fd, secno);
 	if (write(fd, buf, sector_size) != sector_size)
 		fatal(unable_to_write);
 }
 
-static unsigned long long get_start_sect(struct partition *p)
+static inline unsigned long long get_start_sect(struct partition *p)
 {
 	return read4_little_endian(p->start4);
 }
fdisk/fdiskdoslabel.c +4 -0
index 04fdf84..6a9a044 100644
@@ -12,6 +12,10 @@
 #include "fdisk.h"
 #include "fdiskdoslabel.h"
 
+struct pte ptes[MAXIMUM_PARTS];
+unsigned long long extended_offset;
+int ext_index;
+
 /* Allocate a buffer and read a partition table sector */
 static void read_pte(int fd, int pno, unsigned long long offset)
 {
fdisk/fdiskdoslabel.h +8 -6
index b444243..f5568df 100644
@@ -15,27 +15,29 @@ struct pte {
 	char changed;			/* boolean */
 	unsigned long long offset;	/* disk sector number */
 	unsigned char *sectorbuffer;	/* disk sector contents */
-} ptes[MAXIMUM_PARTS];
+};
+
+extern struct pte ptes[MAXIMUM_PARTS];
 
 #define pt_offset(b, n)	((struct partition *)((b) + 0x1be + \
 					      (n) * sizeof(struct partition)))
 
-int ext_index; /* the prime extended partition */
-unsigned long long extended_offset;
+extern int ext_index; /* the prime extended partition */
+extern unsigned long long extended_offset;
 
-static void write_part_table_flag(unsigned char *b)
+static inline void write_part_table_flag(unsigned char *b)
 {
 	b[510] = 0x55;
 	b[511] = 0xaa;
 }
 
 /* A valid partition table sector ends in 0x55 0xaa */
-static unsigned int part_table_flag(unsigned char *b)
+static inline unsigned int part_table_flag(unsigned char *b)
 {
 	return ((unsigned int) b[510]) + (((unsigned int) b[511]) << 8);
 }
 
-static unsigned long long get_partition_start(struct pte *pe)
+static inline unsigned long long get_partition_start(struct pte *pe)
 {
 	return pe->offset + get_start_sect(pe->part_table);
 }
Signature · 7 lines
1.7.7.6



-- 
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
author
#4 lore raw
Quoted 4 lines · Karel Zak
Quoted 1 line · Davidlohr Bueso

I know. It's big and ugly, but not so evil to read.

David, test your patches! Please. This patch was completely broken.

I did - I ran fdisk locally and through the test scripts. What did I break?

Quoted 236 lines
Quoted 3 lines
fdisk/fdisk.c +0 -0

...

fdisk/fdisk.c

Quoted 7 lines
fdisk/fdisk.c +0 -7
-/*
- * Raw disk label. For DOS-type partition tables the MBR,
- * with descriptions of the primary partitions.
- */
-unsigned char *MBRbuffer;
-
-int MBRbuffer_changed;

No, in .c file has to be definition and in .h file is "extern ...".

Quoted 21 lines
fdisk/fdisk.h +3 -1
index cff6b60..2032db7 100644
@@ -87,12 +87,14 @@ extern void update_units(void);
 extern char read_chars(char *mesg);
 extern void set_changed(int);
 extern void set_all_unchanged(void);
+extern int warn_geometry(void);
+extern void warn_limits(void);
+extern void warn_alignment(void);
 
 #define PLURAL	0
 #define SINGULAR 1
 extern const char * str_units(int);
 
-extern unsigned long long get_start_sect(struct partition *p);
 extern unsigned long long get_nr_sects(struct partition *p);
 
 enum labeltype {

fdisk/fdisk.h enum labeltype { · line 117

Quoted 9 lines
fdisk/fdisk.h +6 -0
@@ -107,6 +109,66 @@ enum labeltype {
 
 extern enum labeltype disklabel;
 
+/*
+ * Raw disk label. For DOS-type partition tables the MBR,
+ * with descriptions of the primary partitions.
+ */
+int MBRbuffer_changed;
+unsigned char *MBRbuffer;

this belong to fdisk.c, to fdisk.h belong:

     extern int MBRbuffer_changed;
     extern unsigned char *MBRbuffer;

fdisk/fdisk.h enum labeltype { · line 121

Quoted 4 lines
fdisk/fdisk.h +4 -0
+/* start_sect and nr_sects are stored little endian on all machines */
+/* moreover, they are not aligned correctly */
+static void
+store4_little_endian(unsigned char *cp, unsigned int val) {

this is header file, so "static inline ..."

Karel

Quoted 1 line

From 96f817fb169ac38fe5535cb8c6d2fdf9b2cb8f10 Mon Sep 17 00:00:00 2001

From: Karel Zak <[email protected]>
Date: Wed, 2 May 2012 14:05:51 +0200
Subject: [PATCH] fdisk: fix fdiskdoslabel.c global variables

Signed-off-by Karel Zak <[email protected]>
---
 fdisk/fdisk.c         |    4 +++-
 fdisk/fdisk.h         |   21 ++++++++++-----------
 fdisk/fdiskdoslabel.c |    4 ++++
 fdisk/fdiskdoslabel.h |   14 ++++++++------
 4 files changed, 25 insertions(+), 18 deletions(-)
fdisk/fdisk.c +3 -1
index 6695266..acc84d1 100644
@@ -54,6 +54,9 @@
 
 static void delete_partition(int i);
 
+unsigned char *MBRbuffer;
+int MBRbuffer_changed;
+
 #define hex_val(c)	({ \
 				char _c = (c); \
 				isdigit(_c) ? _c - '0' : \

@@ -145,7 +148,6 @@ char	*disk_device,			/* must be specified */
 	line_buffer[LINE_LENGTH];
 
 int	fd,				/* the disk */
-	ext_index,			/* the prime extended partition */
 	nowarn = 0,			/* no warnings for fdisk -l/-s */
 	dos_compatible_flag = 0,	/* disabled by default */
 	dos_changed = 0,
fdisk/fdisk.h +10 -11
index da86e30..3a1cfd7 100644
@@ -77,7 +77,6 @@ extern unsigned int read_int(unsigned int low, unsigned int dflt,
 extern void print_menu(enum menutype);
 extern void print_partition_size(int num, unsigned long long start, unsigned long long stop, int sysid);
 
-extern unsigned char *MBRbuffer;
 extern void zeroize_mbr_buffer(void);
 
 extern unsigned int heads, cylinders, sector_size;

@@ -113,12 +112,12 @@ extern enum labeltype disklabel;
  * Raw disk label. For DOS-type partition tables the MBR,
  * with descriptions of the primary partitions.
  */
-int MBRbuffer_changed;
-unsigned char *MBRbuffer;
+extern unsigned char *MBRbuffer;
+extern int MBRbuffer_changed;
 
 /* start_sect and nr_sects are stored little endian on all machines */
 /* moreover, they are not aligned correctly */
-static void
+static inline void
 store4_little_endian(unsigned char *cp, unsigned int val) {
 	cp[0] = (val & 0xff);
 	cp[1] = ((val >> 8) & 0xff);

@@ -126,45 +125,45 @@ store4_little_endian(unsigned char *cp, unsigned int val) {
 	cp[3] = ((val >> 24) & 0xff);
 }
 
-static unsigned int read4_little_endian(const unsigned char *cp)
+static inline unsigned int read4_little_endian(const unsigned char *cp)
 {
 	return (unsigned int)(cp[0]) + ((unsigned int)(cp[1]) << 8)
 		+ ((unsigned int)(cp[2]) << 16)
 		+ ((unsigned int)(cp[3]) << 24);
 }
 
-static void set_nr_sects(struct partition *p, unsigned long long nr_sects)
+static inline void set_nr_sects(struct partition *p, unsigned long long nr_sects)
 {
 	store4_little_endian(p->size4, nr_sects);
 }
 
-static void set_start_sect(struct partition *p, unsigned int start_sect)
+static inline void set_start_sect(struct partition *p, unsigned int start_sect)
 {
 	store4_little_endian(p->start4, start_sect);
 }
 
-static void seek_sector(int fd, unsigned long long secno)
+static inline void seek_sector(int fd, unsigned long long secno)
 {
 	off_t offset = (off_t) secno * sector_size;
 	if (lseek(fd, offset, SEEK_SET) == (off_t) -1)
 		fatal(unable_to_seek);
 }
 
-static void read_sector(int fd, unsigned long long secno, unsigned char *buf)
+static inline void read_sector(int fd, unsigned long long secno, unsigned char *buf)
 {
 	seek_sector(fd, secno);
 	if (read(fd, buf, sector_size) != sector_size)
 		fatal(unable_to_read);
 }
 
-static void write_sector(int fd, unsigned long long secno, unsigned char *buf)
+static inline void write_sector(int fd, unsigned long long secno, unsigned char *buf)
 {
 	seek_sector(fd, secno);
 	if (write(fd, buf, sector_size) != sector_size)
 		fatal(unable_to_write);
 }
 
-static unsigned long long get_start_sect(struct partition *p)
+static inline unsigned long long get_start_sect(struct partition *p)
 {
 	return read4_little_endian(p->start4);
 }
fdisk/fdiskdoslabel.c +4 -0
index 04fdf84..6a9a044 100644
@@ -12,6 +12,10 @@
 #include "fdisk.h"
 #include "fdiskdoslabel.h"
 
+struct pte ptes[MAXIMUM_PARTS];
+unsigned long long extended_offset;
+int ext_index;
+
 /* Allocate a buffer and read a partition table sector */
 static void read_pte(int fd, int pno, unsigned long long offset)
 {
fdisk/fdiskdoslabel.h +8 -6
index b444243..f5568df 100644
@@ -15,27 +15,29 @@ struct pte {
 	char changed;			/* boolean */
 	unsigned long long offset;	/* disk sector number */
 	unsigned char *sectorbuffer;	/* disk sector contents */
-} ptes[MAXIMUM_PARTS];
+};
+
+extern struct pte ptes[MAXIMUM_PARTS];
 
 #define pt_offset(b, n)	((struct partition *)((b) + 0x1be + \
 					      (n) * sizeof(struct partition)))
 
-int ext_index; /* the prime extended partition */
-unsigned long long extended_offset;
+extern int ext_index; /* the prime extended partition */
+extern unsigned long long extended_offset;
 
-static void write_part_table_flag(unsigned char *b)
+static inline void write_part_table_flag(unsigned char *b)
 {
 	b[510] = 0x55;
 	b[511] = 0xaa;
 }
 
 /* A valid partition table sector ends in 0x55 0xaa */
-static unsigned int part_table_flag(unsigned char *b)
+static inline unsigned int part_table_flag(unsigned char *b)
 {
 	return ((unsigned int) b[510]) + (((unsigned int) b[511]) << 8);
 }
 
-static unsigned long long get_partition_start(struct pte *pe)
+static inline unsigned long long get_partition_start(struct pte *pe)
 {
 	return pe->offset + get_start_sect(pe->part_table);
 }
Signature · 1 line
1.7.7.6