fdisk: introduce fdisk context

PATCH Applied Davidlohr Bueso 2 participants 7 messages May 20, 2012 – May 23, 2012 Util-Linux package

lore

Share & tools

lore

Davidlohr Bueso

author

2012-05-20 16:11 UTC #0 lore raw

PATCH 5/6 fdisk: introduce fdisk context

From: Davidlohr Bueso <[email protected]>

This is the first patch that adds the initial parts of the new fdisk internal API. Two functions are created to both init and deinit the fdisk context. Only the device's descriptor and path are added as a start and these are replaced throughout fdisk.c and label specific code.

All logic that opens the file does it with fdisk_new_context_from_filename(), and this enforces always opening the device with rw, then, if unsuccessful, with read-only. This changes the current logic that opens the device with certain permissions depending on the user given options. For example, -l opens the device read-only.

This patch passed regression tests and local modifications for sgi/dos/sun disk labels.

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

---
 fdisk/Makefile.am     |    1 +
 fdisk/fdisk.c         |   82 ++++++++++++++++++++++--------------------------
 fdisk/fdisk.h         |    9 +++++
 fdisk/fdiskbsdlabel.c |   44 +++++++++++++-------------
 fdisk/fdiskdoslabel.c |    6 ++--
 fdisk/fdisksgilabel.c |   24 +++++++-------
 fdisk/fdisksunlabel.c |   18 +++++-----
 fdisk/utils.c         |   71 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 165 insertions(+), 90 deletions(-)
 create mode 100644 fdisk/utils.c

fdisk/Makefile.am +1 -0

index 7851bd1..e9be841 100644
@@ -20,6 +20,7 @@ if !ARCH_M68K
 sbin_PROGRAMS = fdisk
 dist_man_MANS = fdisk.8
 fdisk_SOURCES = \
+	utils.c \
 	fdisk.c \
 	fdisk.h \
 	fdiskaixlabel.c \

fdisk/fdisk.c +38 ~~-44~~

231 lines not on this page · raw message

fdisk/fdisk.h +9 -0

index 82888dc..c43517c 100644
@@ -63,6 +63,15 @@ struct geom {
 	unsigned int cylinders;
 };
 
+struct fdisk_context {
+	int dev_fd;     /* device descriptor */
+	char *dev_path; /* device path */
+};
+
+extern struct fdisk_context *cxt;
+extern struct fdisk_context *fdisk_new_context_from_filename(const char *fname);
+extern void fdisk_free_context(struct fdisk_context *cxt);
+
 /* prototypes for fdisk.c */
 extern char *disk_device, *line_ptr;
 extern int fd, partitions;

fdisk/fdiskbsdlabel.c +22 ~~-22~~

129 lines not on this page · raw message

fdisk/fdiskdoslabel.c +3 -3

index 34bcde6..4a93691 100644
@@ -156,7 +156,7 @@ static void read_extended(int ext)
 			return;
 		}
 
-		read_pte(fd, partitions, extended_offset + get_start_sect(p));
+		read_pte(cxt->dev_fd, partitions, extended_offset + get_start_sect(p));
 
 		if (!extended_offset)
 			extended_offset = get_start_sect(p);

@@ -670,7 +670,7 @@ void dos_write_table(void)
 	}
 	if (MBRbuffer_changed) {
 		write_part_table_flag(MBRbuffer);
-		write_sector(fd, 0, MBRbuffer);
+		write_sector(cxt->dev_fd, 0, MBRbuffer);
 	}
 	/* EBR (logical partitions) */
 	for (i = 4; i < partitions; i++) {

@@ -678,7 +678,7 @@ void dos_write_table(void)
 
 		if (pe->changed) {
 			write_part_table_flag(pe->sectorbuffer);
-			write_sector(fd, pe->offset, pe->sectorbuffer);
+			write_sector(cxt->dev_fd, pe->offset, pe->sectorbuffer);
 		}
 	}
 }

fdisk/fdisksgilabel.c +12 ~~-12~~

index 5e851bc..c2ef06d 100644
@@ -183,7 +183,7 @@ sgi_list_table(int xtra) {
 	int kpi = 0;		/* kernel partition ID */
 	char *type;
 
-	w = strlen(disk_device);
+	w = strlen(cxt->dev_path);
 
 	if (xtra) {
 		printf(_("\nDisk %s (SGI disk label): %d heads, %llu sectors\n"

@@ -191,7 +191,7 @@ sgi_list_table(int xtra) {
 			 "%d extra sects/cyl, interleave %d:1\n"
 			 "%s\n"
 			 "Units = %s of %d * %d bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       SSWAP16(sgiparam.pcylcount),
 		       (int) sgiparam.sparecyl, SSWAP16(sgiparam.ilfact),
 		       (char *)sgilabel,

@@ -201,7 +201,7 @@ sgi_list_table(int xtra) {
 		printf(_("\nDisk %s (SGI disk label): "
 			 "%d heads, %llu sectors, %d cylinders\n"
 			 "Units = %s of %d * %d bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       str_units(PLURAL), units_per_sector,
                        sector_size);
 	}

@@ -216,7 +216,7 @@ sgi_list_table(int xtra) {
 			printf(
 				"%2d: %s %4s %9ld %9ld %9ld  %2x  %s\n",
 /* fdisk part number */   i+1,
-/* device */              partname(disk_device, kpi, w+2),
+/* device */              partname(cxt->dev_path, kpi, w+2),
 /* flags */               (sgi_get_swappartition() == i) ? "swap" :
 /* flags */               (sgi_get_bootpartition() == i) ? "boot" : "    ", 
 /* start */               (long) scround(start),

@@ -357,9 +357,9 @@ sgi_write_table(void) {
 		sizeof(*sgilabel)));
 	assert(two_s_complement_32bit_sum(
 		(unsigned int*)sgilabel, sizeof(*sgilabel)) == 0);
-	if (lseek(fd, 0, SEEK_SET) < 0)
+	if (lseek(cxt->dev_fd, 0, SEEK_SET) < 0)
 		fatal(unable_to_seek);
-	if (write(fd, sgilabel, SECTOR_SIZE) != SECTOR_SIZE)
+	if (write(cxt->dev_fd, sgilabel, SECTOR_SIZE) != SECTOR_SIZE)
 		fatal(unable_to_write);
 	if (! strncmp((char *) sgilabel->directory[0].vol_file_name, "sgilabel", 8)) {
 		/*

@@ -368,10 +368,10 @@ sgi_write_table(void) {
 		 */
 		sgiinfo *info = fill_sgiinfo();
 		int infostartblock = SSWAP32(sgilabel->directory[0].vol_file_start);
-		if (lseek(fd, (off_t) infostartblock*
+		if (lseek(cxt->dev_fd, (off_t) infostartblock*
 				SECTOR_SIZE, SEEK_SET) < 0)
 			fatal(unable_to_seek);
-		if (write(fd, info, SECTOR_SIZE) != SECTOR_SIZE)
+		if (write(cxt->dev_fd, info, SECTOR_SIZE) != SECTOR_SIZE)
 			fatal(unable_to_write);
 		free(info);
 	}

@@ -702,11 +702,11 @@ create_sgilabel(void)
 
 	other_endian = (BYTE_ORDER == LITTLE_ENDIAN);
 
-	res = blkdev_get_sectors(fd, &llsectors);
+	res = blkdev_get_sectors(cxt->dev_fd, &llsectors);
 
 #ifdef HDIO_GETGEO
-	if (ioctl(fd, HDIO_GETGEO, &geometry) < 0)
-		err(EXIT_FAILURE, _("HDIO_GETGEO ioctl failed on %s"), disk_device);
+	if (ioctl(cxt->dev_fd, HDIO_GETGEO, &geometry) < 0)
+		err(EXIT_FAILURE, _("HDIO_GETGEO ioctl failed on %s"), cxt->dev_path);
 
 	heads = geometry.heads;
 	sectors = geometry.sectors;

@@ -724,7 +724,7 @@ create_sgilabel(void)
 			_("Warning:  BLKGETSIZE ioctl failed on %s.  "
 			  "Using geometry cylinder value of %d.\n"
 			  "This value may be truncated for devices"
-			  " > 33.8 GB.\n"), disk_device, cylinders);
+			  " > 33.8 GB.\n"), cxt->dev_path, cylinders);
 	}
 #endif
 	for (i = 0; i < 4; i++) {

fdisk/fdisksunlabel.c +9 -9

index 4a6db35..6944824 100644
@@ -172,11 +172,11 @@ void create_sunlabel(void)
 	sunlabel->version = SSWAP32(SUN_LABEL_VERSION);
 	sunlabel->num_partitions = SSWAP16(SUN_NUM_PARTITIONS);
 
-	res = blkdev_get_sectors(fd, &llsectors);
+	res = blkdev_get_sectors(cxt->dev_fd, &llsectors);
 	sec_fac = sector_size / 512;
 
 #ifdef HDIO_GETGEO
-	if (!ioctl(fd, HDIO_GETGEO, &geometry)) {
+	if (!ioctl(cxt->dev_fd, HDIO_GETGEO, &geometry)) {
 	        heads = geometry.heads;
 	        sectors = geometry.sectors;
 		if (res == 0) {

@@ -190,7 +190,7 @@ void create_sunlabel(void)
 				_("Warning:  BLKGETSIZE ioctl failed on %s.  "
 				  "Using geometry cylinder value of %d.\n"
 				  "This value may be truncated for devices"
-				  " > 33.8 GB.\n"), disk_device, cylinders);
+				  " > 33.8 GB.\n"), cxt->dev_path, cylinders);
 		}
 	} else
 #endif

@@ -538,7 +538,7 @@ void sun_list_table(int xtra)
 	int i, w;
 	char *type;
 
-	w = strlen(disk_device);
+	w = strlen(cxt->dev_path);
 	if (xtra)
 		printf(
 		_("\nDisk %s (Sun disk label): %u heads, %llu sectors, %d rpm\n"

@@ -547,7 +547,7 @@ void sun_list_table(int xtra)
 		"Label ID: %s\n"
 		"Volume ID: %s\n"
 		"Units = %s of %d * 512 bytes\n\n"),
-		       disk_device, heads, sectors, SSWAP16(sunlabel->rpm),
+		       cxt->dev_path, heads, sectors, SSWAP16(sunlabel->rpm),
 		       cylinders, SSWAP16(sunlabel->acyl),
 		       SSWAP16(sunlabel->pcyl),
 		       SSWAP16(sunlabel->apc),

@@ -559,7 +559,7 @@ void sun_list_table(int xtra)
 		printf(
 	_("\nDisk %s (Sun disk label): %u heads, %llu sectors, %u cylinders\n"
 	"Units = %s of %d * 512 bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       str_units(PLURAL), units_per_sector);
 
 	printf(_("%*s Flag    Start       End    Blocks   Id  System\n"),

@@ -573,7 +573,7 @@ void sun_list_table(int xtra)
 			uint32_t len = SSWAP32(part->num_sectors);
 			printf(
 			    "%s %c%c %9lu %9lu %9lu%c  %2x  %s\n",
-/* device */		  partname(disk_device, i+1, w),
+/* device */		  partname(cxt->dev_path, i+1, w),
 /* flags */		  (tag->flag & SSWAP16(SUN_FLAG_UNMNT)) ? 'u' : ' ',
 			  (tag->flag & SSWAP16(SUN_FLAG_RONLY)) ? 'r' : ' ',
 /* start */		  (unsigned long) scround(start),

@@ -634,9 +634,9 @@ void sun_write_table(void)
 	while(ush < (unsigned short *)(&sunlabel->cksum))
 		csum ^= *ush++;
 	sunlabel->cksum = csum;
-	if (lseek(fd, 0, SEEK_SET) < 0)
+	if (lseek(cxt->dev_fd, 0, SEEK_SET) < 0)
 		fatal(unable_to_seek);
-	if (write(fd, sunlabel, SECTOR_SIZE) != SECTOR_SIZE)
+	if (write(cxt->dev_fd, sunlabel, SECTOR_SIZE) != SECTOR_SIZE)
 		fatal(unable_to_write);
 }

fdisk/utils.c new file +71 -0

71 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Karel Zak

maintainer

2012-05-21 09:41 UTC #1 lore raw

fdisk/utils.c line 42

Quoted 12 lines · Davidlohr Bueso

fdisk/utils.c new file +12 -0

+struct fdisk_context *fdisk_new_context_from_filename(const char *fname)
+{
+	int fd;
+	struct fdisk_context *cxt = NULL;
+
+	/*
+	 * Attempt to open the device with r-w permissions
+	 * by default, otherwise try read-only.
+	 */
+	if ((fd = open(fname, O_RDWR)) < 0)
+		if ((fd = open(fname, O_RDONLY)) < 0)
+			goto ret;

            return NULL;

fdisk/utils.c line 51

Quoted 8 lines

fdisk/utils.c new file +8 -0

+	cxt = calloc(1, sizeof(*cxt));
+	if (!cxt)
+		goto ret;
+
+	cxt->dev_fd = fd;
+	cxt->dev_path = strdup(fname);
+	if (!cxt->dev_path)
+		return NULL;

        goto fail;

Please, think about it as about library, it meas without memory and fd leaks :-)

fdisk/utils.c line 55

Quoted 4 lines

fdisk/utils.c new file +4 -0
+ret:
+	return cxt;
+}
+

return cxt;

fail:
errsv = errno;
fdisk_free_context(cxt);
errno = errsv;
return NULL;

Karel

Signature · 2 lines

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

Davidlohr Bueso

author

2012-05-21 10:44 UTC #2 lore raw

Quoted 37 lines · Karel Zak

fdisk/utils.c line 42

Quoted 12 lines · Davidlohr Bueso

fdisk/utils.c new file +12 -0

+struct fdisk_context *fdisk_new_context_from_filename(const char *fname)
+{
+	int fd;
+	struct fdisk_context *cxt = NULL;
+
+	/*
+	 * Attempt to open the device with r-w permissions
+	 * by default, otherwise try read-only.
+	 */
+	if ((fd = open(fname, O_RDWR)) < 0)
+		if ((fd = open(fname, O_RDONLY)) < 0)
+			goto ret;

            return NULL;

fdisk/utils.c line 51

Quoted 8 lines

fdisk/utils.c new file +8 -0

+	cxt = calloc(1, sizeof(*cxt));
+	if (!cxt)
+		goto ret;
+
+	cxt->dev_fd = fd;
+	cxt->dev_path = strdup(fname);
+	if (!cxt->dev_path)
+		return NULL;

        goto fail;

Please, think about it as about library, it meas without memory and fd leaks :-)

fdisk/utils.c line 55

Quoted 4 lines

fdisk/utils.c new file +4 -0
+ret:
+	return cxt;
+}
+

   return cxt;

Quoted 2 lines

fail:

   errsv = errno;

Hmm I hadn't considered saving errno stuff...

Quoted 1 line

   fdisk_free_context(cxt);

Yes, we need to plug that descriptor leak! fdisk_free_context() also frees cxt->dev_path, so we should really just call close(fd).

Thanks,
Davidlohr

Quoted 6 lines

   errno = errsv;
   return NULL;

Karel

Davidlohr Bueso

author

2012-05-21 20:14 UTC #3 lore raw

In reply to Karel Zak

Quoted 37 lines · Karel Zak

Review

Quoted 12 lines · Davidlohr Bueso

(quoted diff) +12 -0

+struct fdisk_context *fdisk_new_context_from_filename(const char *fname)
+{
+	int fd;
+	struct fdisk_context *cxt = NULL;
+
+	/*
+	 * Attempt to open the device with r-w permissions
+	 * by default, otherwise try read-only.
+	 */
+	if ((fd = open(fname, O_RDWR)) < 0)
+		if ((fd = open(fname, O_RDONLY)) < 0)
+			goto ret;

            return NULL;

Review

Quoted 8 lines

(quoted diff) +8 -0

+	cxt = calloc(1, sizeof(*cxt));
+	if (!cxt)
+		goto ret;
+
+	cxt->dev_fd = fd;
+	cxt->dev_path = strdup(fname);
+	if (!cxt->dev_path)
+		return NULL;

        goto fail;

Please, think about it as about library, it meas without memory and fd leaks :-)

Review

Quoted 4 lines

(quoted diff) +4 -0
+ret:
+	return cxt;
+}
+

   return cxt;

Quoted 5 lines

fail:

   errsv = errno;
   fdisk_free_context(cxt);
   errno = errsv;
   return NULL;

From: Davidlohr Bueso <[email protected]>
Date: Mon, 21 May 2012 22:12:33 +0200
Subject: [PATCH] fdisk: introduce fdisk context

This patch passed regression tests and local modifications for sgi/dos/sun disk labels.

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

---
 fdisk/Makefile.am     |    1 +
 fdisk/fdisk.c         |   82 ++++++++++++++++++++++--------------------------
 fdisk/fdisk.h         |    9 +++++
 fdisk/fdiskbsdlabel.c |   44 +++++++++++++-------------
 fdisk/fdiskdoslabel.c |    6 ++--
 fdisk/fdisksgilabel.c |   24 +++++++-------
 fdisk/fdisksunlabel.c |   18 +++++-----
 fdisk/utils.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 168 insertions(+), 90 deletions(-)
 create mode 100644 fdisk/utils.c

fdisk/Makefile.am +1 -0

index 7851bd1..e9be841 100644
@@ -20,6 +20,7 @@ if !ARCH_M68K
 sbin_PROGRAMS = fdisk
 dist_man_MANS = fdisk.8
 fdisk_SOURCES = \
+	utils.c \
 	fdisk.c \
 	fdisk.h \
 	fdiskaixlabel.c \

fdisk/fdisk.c +38 ~~-44~~

231 lines not on this page · raw message

fdisk/fdisk.h +9 -0

index 82888dc..c43517c 100644
@@ -63,6 +63,15 @@ struct geom {
 	unsigned int cylinders;
 };
 
+struct fdisk_context {
+	int dev_fd;     /* device descriptor */
+	char *dev_path; /* device path */
+};
+
+extern struct fdisk_context *cxt;
+extern struct fdisk_context *fdisk_new_context_from_filename(const char *fname);
+extern void fdisk_free_context(struct fdisk_context *cxt);
+
 /* prototypes for fdisk.c */
 extern char *disk_device, *line_ptr;
 extern int fd, partitions;

fdisk/fdiskbsdlabel.c +22 ~~-22~~

129 lines not on this page · raw message

fdisk/fdiskdoslabel.c +3 -3

index 34bcde6..4a93691 100644
@@ -156,7 +156,7 @@ static void read_extended(int ext)
 			return;
 		}
 
-		read_pte(fd, partitions, extended_offset + get_start_sect(p));
+		read_pte(cxt->dev_fd, partitions, extended_offset + get_start_sect(p));
 
 		if (!extended_offset)
 			extended_offset = get_start_sect(p);

@@ -670,7 +670,7 @@ void dos_write_table(void)
 	}
 	if (MBRbuffer_changed) {
 		write_part_table_flag(MBRbuffer);
-		write_sector(fd, 0, MBRbuffer);
+		write_sector(cxt->dev_fd, 0, MBRbuffer);
 	}
 	/* EBR (logical partitions) */
 	for (i = 4; i < partitions; i++) {

@@ -678,7 +678,7 @@ void dos_write_table(void)
 
 		if (pe->changed) {
 			write_part_table_flag(pe->sectorbuffer);
-			write_sector(fd, pe->offset, pe->sectorbuffer);
+			write_sector(cxt->dev_fd, pe->offset, pe->sectorbuffer);
 		}
 	}
 }

fdisk/fdisksgilabel.c +12 ~~-12~~

index 5e851bc..c2ef06d 100644
@@ -183,7 +183,7 @@ sgi_list_table(int xtra) {
 	int kpi = 0;		/* kernel partition ID */
 	char *type;
 
-	w = strlen(disk_device);
+	w = strlen(cxt->dev_path);
 
 	if (xtra) {
 		printf(_("\nDisk %s (SGI disk label): %d heads, %llu sectors\n"

@@ -191,7 +191,7 @@ sgi_list_table(int xtra) {
 			 "%d extra sects/cyl, interleave %d:1\n"
 			 "%s\n"
 			 "Units = %s of %d * %d bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       SSWAP16(sgiparam.pcylcount),
 		       (int) sgiparam.sparecyl, SSWAP16(sgiparam.ilfact),
 		       (char *)sgilabel,

@@ -201,7 +201,7 @@ sgi_list_table(int xtra) {
 		printf(_("\nDisk %s (SGI disk label): "
 			 "%d heads, %llu sectors, %d cylinders\n"
 			 "Units = %s of %d * %d bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       str_units(PLURAL), units_per_sector,
                        sector_size);
 	}

@@ -216,7 +216,7 @@ sgi_list_table(int xtra) {
 			printf(
 				"%2d: %s %4s %9ld %9ld %9ld  %2x  %s\n",
 /* fdisk part number */   i+1,
-/* device */              partname(disk_device, kpi, w+2),
+/* device */              partname(cxt->dev_path, kpi, w+2),
 /* flags */               (sgi_get_swappartition() == i) ? "swap" :
 /* flags */               (sgi_get_bootpartition() == i) ? "boot" : "    ", 
 /* start */               (long) scround(start),

@@ -357,9 +357,9 @@ sgi_write_table(void) {
 		sizeof(*sgilabel)));
 	assert(two_s_complement_32bit_sum(
 		(unsigned int*)sgilabel, sizeof(*sgilabel)) == 0);
-	if (lseek(fd, 0, SEEK_SET) < 0)
+	if (lseek(cxt->dev_fd, 0, SEEK_SET) < 0)
 		fatal(unable_to_seek);
-	if (write(fd, sgilabel, SECTOR_SIZE) != SECTOR_SIZE)
+	if (write(cxt->dev_fd, sgilabel, SECTOR_SIZE) != SECTOR_SIZE)
 		fatal(unable_to_write);
 	if (! strncmp((char *) sgilabel->directory[0].vol_file_name, "sgilabel", 8)) {
 		/*

@@ -368,10 +368,10 @@ sgi_write_table(void) {
 		 */
 		sgiinfo *info = fill_sgiinfo();
 		int infostartblock = SSWAP32(sgilabel->directory[0].vol_file_start);
-		if (lseek(fd, (off_t) infostartblock*
+		if (lseek(cxt->dev_fd, (off_t) infostartblock*
 				SECTOR_SIZE, SEEK_SET) < 0)
 			fatal(unable_to_seek);
-		if (write(fd, info, SECTOR_SIZE) != SECTOR_SIZE)
+		if (write(cxt->dev_fd, info, SECTOR_SIZE) != SECTOR_SIZE)
 			fatal(unable_to_write);
 		free(info);
 	}

@@ -702,11 +702,11 @@ create_sgilabel(void)
 
 	other_endian = (BYTE_ORDER == LITTLE_ENDIAN);
 
-	res = blkdev_get_sectors(fd, &llsectors);
+	res = blkdev_get_sectors(cxt->dev_fd, &llsectors);
 
 #ifdef HDIO_GETGEO
-	if (ioctl(fd, HDIO_GETGEO, &geometry) < 0)
-		err(EXIT_FAILURE, _("HDIO_GETGEO ioctl failed on %s"), disk_device);
+	if (ioctl(cxt->dev_fd, HDIO_GETGEO, &geometry) < 0)
+		err(EXIT_FAILURE, _("HDIO_GETGEO ioctl failed on %s"), cxt->dev_path);
 
 	heads = geometry.heads;
 	sectors = geometry.sectors;

@@ -724,7 +724,7 @@ create_sgilabel(void)
 			_("Warning:  BLKGETSIZE ioctl failed on %s.  "
 			  "Using geometry cylinder value of %d.\n"
 			  "This value may be truncated for devices"
-			  " > 33.8 GB.\n"), disk_device, cylinders);
+			  " > 33.8 GB.\n"), cxt->dev_path, cylinders);
 	}
 #endif
 	for (i = 0; i < 4; i++) {

fdisk/fdisksunlabel.c +9 -9

index 4a6db35..6944824 100644
@@ -172,11 +172,11 @@ void create_sunlabel(void)
 	sunlabel->version = SSWAP32(SUN_LABEL_VERSION);
 	sunlabel->num_partitions = SSWAP16(SUN_NUM_PARTITIONS);
 
-	res = blkdev_get_sectors(fd, &llsectors);
+	res = blkdev_get_sectors(cxt->dev_fd, &llsectors);
 	sec_fac = sector_size / 512;
 
 #ifdef HDIO_GETGEO
-	if (!ioctl(fd, HDIO_GETGEO, &geometry)) {
+	if (!ioctl(cxt->dev_fd, HDIO_GETGEO, &geometry)) {
 	        heads = geometry.heads;
 	        sectors = geometry.sectors;
 		if (res == 0) {

@@ -190,7 +190,7 @@ void create_sunlabel(void)
 				_("Warning:  BLKGETSIZE ioctl failed on %s.  "
 				  "Using geometry cylinder value of %d.\n"
 				  "This value may be truncated for devices"
-				  " > 33.8 GB.\n"), disk_device, cylinders);
+				  " > 33.8 GB.\n"), cxt->dev_path, cylinders);
 		}
 	} else
 #endif

@@ -538,7 +538,7 @@ void sun_list_table(int xtra)
 	int i, w;
 	char *type;
 
-	w = strlen(disk_device);
+	w = strlen(cxt->dev_path);
 	if (xtra)
 		printf(
 		_("\nDisk %s (Sun disk label): %u heads, %llu sectors, %d rpm\n"

@@ -547,7 +547,7 @@ void sun_list_table(int xtra)
 		"Label ID: %s\n"
 		"Volume ID: %s\n"
 		"Units = %s of %d * 512 bytes\n\n"),
-		       disk_device, heads, sectors, SSWAP16(sunlabel->rpm),
+		       cxt->dev_path, heads, sectors, SSWAP16(sunlabel->rpm),
 		       cylinders, SSWAP16(sunlabel->acyl),
 		       SSWAP16(sunlabel->pcyl),
 		       SSWAP16(sunlabel->apc),

@@ -559,7 +559,7 @@ void sun_list_table(int xtra)
 		printf(
 	_("\nDisk %s (Sun disk label): %u heads, %llu sectors, %u cylinders\n"
 	"Units = %s of %d * 512 bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       str_units(PLURAL), units_per_sector);
 
 	printf(_("%*s Flag    Start       End    Blocks   Id  System\n"),

@@ -573,7 +573,7 @@ void sun_list_table(int xtra)
 			uint32_t len = SSWAP32(part->num_sectors);
 			printf(
 			    "%s %c%c %9lu %9lu %9lu%c  %2x  %s\n",
-/* device */		  partname(disk_device, i+1, w),
+/* device */		  partname(cxt->dev_path, i+1, w),
 /* flags */		  (tag->flag & SSWAP16(SUN_FLAG_UNMNT)) ? 'u' : ' ',
 			  (tag->flag & SSWAP16(SUN_FLAG_RONLY)) ? 'r' : ' ',
 /* start */		  (unsigned long) scround(start),

@@ -634,9 +634,9 @@ void sun_write_table(void)
 	while(ush < (unsigned short *)(&sunlabel->cksum))
 		csum ^= *ush++;
 	sunlabel->cksum = csum;
-	if (lseek(fd, 0, SEEK_SET) < 0)
+	if (lseek(cxt->dev_fd, 0, SEEK_SET) < 0)
 		fatal(unable_to_seek);
-	if (write(fd, sunlabel, SECTOR_SIZE) != SECTOR_SIZE)
+	if (write(cxt->dev_fd, sunlabel, SECTOR_SIZE) != SECTOR_SIZE)
 		fatal(unable_to_write);
 }

fdisk/utils.c new file +74 -0

74 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Davidlohr Bueso

author

2012-05-21 20:28 UTC #4 lore raw

Quoted 44 lines · Davidlohr Bueso

Quoted 37 lines · Karel Zak

Review

Quoted 12 lines · Davidlohr Bueso

(quoted diff) +12 -0

+struct fdisk_context *fdisk_new_context_from_filename(const char *fname)
+{
+	int fd;
+	struct fdisk_context *cxt = NULL;
+
+	/*
+	 * Attempt to open the device with r-w permissions
+	 * by default, otherwise try read-only.
+	 */
+	if ((fd = open(fname, O_RDWR)) < 0)
+		if ((fd = open(fname, O_RDONLY)) < 0)
+			goto ret;

            return NULL;

Review

Quoted 8 lines

(quoted diff) +8 -0

+	cxt = calloc(1, sizeof(*cxt));
+	if (!cxt)
+		goto ret;
+
+	cxt->dev_fd = fd;
+	cxt->dev_path = strdup(fname);
+	if (!cxt->dev_path)
+		return NULL;

        goto fail;

Please, think about it as about library, it meas without memory and fd leaks :-)

Review

Quoted 4 lines

(quoted diff) +4 -0
+ret:
+	return cxt;
+}
+

   return cxt;

Quoted 5 lines

fail:

   errsv = errno;
   fdisk_free_context(cxt);
   errno = errsv;
   return NULL;

Quoted 3 lines

From: Davidlohr Bueso <[email protected]>
Date: Mon, 21 May 2012 22:12:33 +0200
Subject: [PATCH] fdisk: introduce fdisk context

*sigh* Please ignore this previous patch, it doesn't return cxt from the init function (/me hits himself). This one is ok, based on your comments. Thanks and sorry about the noise.

From: Davidlohr Bueso <[email protected]>
Date: Mon, 21 May 2012 22:26:32 +0200
Subject: [PATCH] fdisk: fdisk: introduce fdisk context

All logic that opens the file does it with fdisk_new_context_from_filename(), and this enforces always opening the device with rw, then, if unsuccesfull, with read-only. This changes the current logic that opens the device with certain permissions depending on the user given options. For example, -l opens the device read-only.

This patch passed regression tests and local modifications for sgi/dos/sun disk labels.

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

---
 fdisk/Makefile.am     |    1 +
 fdisk/fdisk.c         |   82 ++++++++++++++++++++++--------------------------
 fdisk/fdisk.h         |    9 +++++
 fdisk/fdiskbsdlabel.c |   44 +++++++++++++-------------
 fdisk/fdiskdoslabel.c |    6 ++--
 fdisk/fdisksgilabel.c |   24 +++++++-------
 fdisk/fdisksunlabel.c |   18 +++++-----
 fdisk/utils.c         |   76 +++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 170 insertions(+), 90 deletions(-)
 create mode 100644 fdisk/utils.c

fdisk/Makefile.am +1 -0

index 7851bd1..e9be841 100644
@@ -20,6 +20,7 @@ if !ARCH_M68K
 sbin_PROGRAMS = fdisk
 dist_man_MANS = fdisk.8
 fdisk_SOURCES = \
+	utils.c \
 	fdisk.c \
 	fdisk.h \
 	fdiskaixlabel.c \

fdisk/fdisk.c +38 ~~-44~~

231 lines not on this page · raw message

fdisk/fdisk.h +9 -0

index 82888dc..c43517c 100644
@@ -63,6 +63,15 @@ struct geom {
 	unsigned int cylinders;
 };
 
+struct fdisk_context {
+	int dev_fd;     /* device descriptor */
+	char *dev_path; /* device path */
+};
+
+extern struct fdisk_context *cxt;
+extern struct fdisk_context *fdisk_new_context_from_filename(const char *fname);
+extern void fdisk_free_context(struct fdisk_context *cxt);
+
 /* prototypes for fdisk.c */
 extern char *disk_device, *line_ptr;
 extern int fd, partitions;

fdisk/fdiskbsdlabel.c +22 ~~-22~~

129 lines not on this page · raw message

fdisk/fdiskdoslabel.c +3 -3

index 34bcde6..4a93691 100644
@@ -156,7 +156,7 @@ static void read_extended(int ext)
 			return;
 		}
 
-		read_pte(fd, partitions, extended_offset + get_start_sect(p));
+		read_pte(cxt->dev_fd, partitions, extended_offset + get_start_sect(p));
 
 		if (!extended_offset)
 			extended_offset = get_start_sect(p);

@@ -670,7 +670,7 @@ void dos_write_table(void)
 	}
 	if (MBRbuffer_changed) {
 		write_part_table_flag(MBRbuffer);
-		write_sector(fd, 0, MBRbuffer);
+		write_sector(cxt->dev_fd, 0, MBRbuffer);
 	}
 	/* EBR (logical partitions) */
 	for (i = 4; i < partitions; i++) {

@@ -678,7 +678,7 @@ void dos_write_table(void)
 
 		if (pe->changed) {
 			write_part_table_flag(pe->sectorbuffer);
-			write_sector(fd, pe->offset, pe->sectorbuffer);
+			write_sector(cxt->dev_fd, pe->offset, pe->sectorbuffer);
 		}
 	}
 }

fdisk/fdisksgilabel.c +12 ~~-12~~

index 5e851bc..c2ef06d 100644
@@ -183,7 +183,7 @@ sgi_list_table(int xtra) {
 	int kpi = 0;		/* kernel partition ID */
 	char *type;
 
-	w = strlen(disk_device);
+	w = strlen(cxt->dev_path);
 
 	if (xtra) {
 		printf(_("\nDisk %s (SGI disk label): %d heads, %llu sectors\n"

@@ -191,7 +191,7 @@ sgi_list_table(int xtra) {
 			 "%d extra sects/cyl, interleave %d:1\n"
 			 "%s\n"
 			 "Units = %s of %d * %d bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       SSWAP16(sgiparam.pcylcount),
 		       (int) sgiparam.sparecyl, SSWAP16(sgiparam.ilfact),
 		       (char *)sgilabel,

@@ -201,7 +201,7 @@ sgi_list_table(int xtra) {
 		printf(_("\nDisk %s (SGI disk label): "
 			 "%d heads, %llu sectors, %d cylinders\n"
 			 "Units = %s of %d * %d bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       str_units(PLURAL), units_per_sector,
                        sector_size);
 	}

@@ -216,7 +216,7 @@ sgi_list_table(int xtra) {
 			printf(
 				"%2d: %s %4s %9ld %9ld %9ld  %2x  %s\n",
 /* fdisk part number */   i+1,
-/* device */              partname(disk_device, kpi, w+2),
+/* device */              partname(cxt->dev_path, kpi, w+2),
 /* flags */               (sgi_get_swappartition() == i) ? "swap" :
 /* flags */               (sgi_get_bootpartition() == i) ? "boot" : "    ", 
 /* start */               (long) scround(start),

@@ -357,9 +357,9 @@ sgi_write_table(void) {
 		sizeof(*sgilabel)));
 	assert(two_s_complement_32bit_sum(
 		(unsigned int*)sgilabel, sizeof(*sgilabel)) == 0);
-	if (lseek(fd, 0, SEEK_SET) < 0)
+	if (lseek(cxt->dev_fd, 0, SEEK_SET) < 0)
 		fatal(unable_to_seek);
-	if (write(fd, sgilabel, SECTOR_SIZE) != SECTOR_SIZE)
+	if (write(cxt->dev_fd, sgilabel, SECTOR_SIZE) != SECTOR_SIZE)
 		fatal(unable_to_write);
 	if (! strncmp((char *) sgilabel->directory[0].vol_file_name, "sgilabel", 8)) {
 		/*

@@ -368,10 +368,10 @@ sgi_write_table(void) {
 		 */
 		sgiinfo *info = fill_sgiinfo();
 		int infostartblock = SSWAP32(sgilabel->directory[0].vol_file_start);
-		if (lseek(fd, (off_t) infostartblock*
+		if (lseek(cxt->dev_fd, (off_t) infostartblock*
 				SECTOR_SIZE, SEEK_SET) < 0)
 			fatal(unable_to_seek);
-		if (write(fd, info, SECTOR_SIZE) != SECTOR_SIZE)
+		if (write(cxt->dev_fd, info, SECTOR_SIZE) != SECTOR_SIZE)
 			fatal(unable_to_write);
 		free(info);
 	}

@@ -702,11 +702,11 @@ create_sgilabel(void)
 
 	other_endian = (BYTE_ORDER == LITTLE_ENDIAN);
 
-	res = blkdev_get_sectors(fd, &llsectors);
+	res = blkdev_get_sectors(cxt->dev_fd, &llsectors);
 
 #ifdef HDIO_GETGEO
-	if (ioctl(fd, HDIO_GETGEO, &geometry) < 0)
-		err(EXIT_FAILURE, _("HDIO_GETGEO ioctl failed on %s"), disk_device);
+	if (ioctl(cxt->dev_fd, HDIO_GETGEO, &geometry) < 0)
+		err(EXIT_FAILURE, _("HDIO_GETGEO ioctl failed on %s"), cxt->dev_path);
 
 	heads = geometry.heads;
 	sectors = geometry.sectors;

@@ -724,7 +724,7 @@ create_sgilabel(void)
 			_("Warning:  BLKGETSIZE ioctl failed on %s.  "
 			  "Using geometry cylinder value of %d.\n"
 			  "This value may be truncated for devices"
-			  " > 33.8 GB.\n"), disk_device, cylinders);
+			  " > 33.8 GB.\n"), cxt->dev_path, cylinders);
 	}
 #endif
 	for (i = 0; i < 4; i++) {

fdisk/fdisksunlabel.c +9 -9

index 4a6db35..6944824 100644
@@ -172,11 +172,11 @@ void create_sunlabel(void)
 	sunlabel->version = SSWAP32(SUN_LABEL_VERSION);
 	sunlabel->num_partitions = SSWAP16(SUN_NUM_PARTITIONS);
 
-	res = blkdev_get_sectors(fd, &llsectors);
+	res = blkdev_get_sectors(cxt->dev_fd, &llsectors);
 	sec_fac = sector_size / 512;
 
 #ifdef HDIO_GETGEO
-	if (!ioctl(fd, HDIO_GETGEO, &geometry)) {
+	if (!ioctl(cxt->dev_fd, HDIO_GETGEO, &geometry)) {
 	        heads = geometry.heads;
 	        sectors = geometry.sectors;
 		if (res == 0) {

@@ -190,7 +190,7 @@ void create_sunlabel(void)
 				_("Warning:  BLKGETSIZE ioctl failed on %s.  "
 				  "Using geometry cylinder value of %d.\n"
 				  "This value may be truncated for devices"
-				  " > 33.8 GB.\n"), disk_device, cylinders);
+				  " > 33.8 GB.\n"), cxt->dev_path, cylinders);
 		}
 	} else
 #endif

@@ -538,7 +538,7 @@ void sun_list_table(int xtra)
 	int i, w;
 	char *type;
 
-	w = strlen(disk_device);
+	w = strlen(cxt->dev_path);
 	if (xtra)
 		printf(
 		_("\nDisk %s (Sun disk label): %u heads, %llu sectors, %d rpm\n"

@@ -547,7 +547,7 @@ void sun_list_table(int xtra)
 		"Label ID: %s\n"
 		"Volume ID: %s\n"
 		"Units = %s of %d * 512 bytes\n\n"),
-		       disk_device, heads, sectors, SSWAP16(sunlabel->rpm),
+		       cxt->dev_path, heads, sectors, SSWAP16(sunlabel->rpm),
 		       cylinders, SSWAP16(sunlabel->acyl),
 		       SSWAP16(sunlabel->pcyl),
 		       SSWAP16(sunlabel->apc),

@@ -559,7 +559,7 @@ void sun_list_table(int xtra)
 		printf(
 	_("\nDisk %s (Sun disk label): %u heads, %llu sectors, %u cylinders\n"
 	"Units = %s of %d * 512 bytes\n\n"),
-		       disk_device, heads, sectors, cylinders,
+		       cxt->dev_path, heads, sectors, cylinders,
 		       str_units(PLURAL), units_per_sector);
 
 	printf(_("%*s Flag    Start       End    Blocks   Id  System\n"),

@@ -573,7 +573,7 @@ void sun_list_table(int xtra)
 			uint32_t len = SSWAP32(part->num_sectors);
 			printf(
 			    "%s %c%c %9lu %9lu %9lu%c  %2x  %s\n",
-/* device */		  partname(disk_device, i+1, w),
+/* device */		  partname(cxt->dev_path, i+1, w),
 /* flags */		  (tag->flag & SSWAP16(SUN_FLAG_UNMNT)) ? 'u' : ' ',
 			  (tag->flag & SSWAP16(SUN_FLAG_RONLY)) ? 'r' : ' ',
 /* start */		  (unsigned long) scround(start),

@@ -634,9 +634,9 @@ void sun_write_table(void)
 	while(ush < (unsigned short *)(&sunlabel->cksum))
 		csum ^= *ush++;
 	sunlabel->cksum = csum;
-	if (lseek(fd, 0, SEEK_SET) < 0)
+	if (lseek(cxt->dev_fd, 0, SEEK_SET) < 0)
 		fatal(unable_to_seek);
-	if (write(fd, sunlabel, SECTOR_SIZE) != SECTOR_SIZE)
+	if (write(cxt->dev_fd, sunlabel, SECTOR_SIZE) != SECTOR_SIZE)
 		fatal(unable_to_write);
 }

fdisk/utils.c new file +76 -0

76 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Karel Zak

maintainer

2012-05-23 09:43 UTC #5 lore raw

Quoted 5 lines · Davidlohr Bueso

This is the first patch that adds the initial parts of the new fdisk internal API. Two functions are created to both init and deinit the fdisk context. Only the device's descriptor and path are added as a start and these are replaced throughout fdisk.c and label specific code.

Applied (and the rest of the patches too), thanks.

Quoted 3 lines

All logic that opens the file does it with fdisk_new_context_from_filename(), and this enforces always opening the device with rw

I have added 'readonly' option to the fdisk_new_context_from_filename().

It would be better to minimize number of situation where we somewhere open devices read-write. For example udevd, udisks, ... are pretty sensitive to write operations (to detect new PT or new filesystem etc.).

Notes:

1/ What about to rename fdisk_new_context_from_filename() to
fdisk_new_context()? I guess that the device name is mandatory option.

2/ Please, don't use global 'cxt' if possible. See for example

get_geometry() -> get_topology()

disk.c: In function ‘get_topology’:
fdisk.c:456:18: warning: unused parameter ‘fd’ [-Wunused-parameter]

there is get_geometry(cxt->fd, ...), but get_topology() ignores the parameter 'fd' and the function uses the global cxt->fd. This is nasty :-)

It would be better to rename global 'cxt' to something less generic and everywhere in functions use 'cxt' parameter, for example:

     get_geometry(struct fdisk_context *cxt, struct geom *g);


    Karel

Signature · 2 lines

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

Davidlohr Bueso

author

2012-05-23 09:51 UTC #6 lore raw

Quoted 8 lines · Karel Zak

Quoted 5 lines · Davidlohr Bueso
This is the first patch that adds the initial parts of the new fdisk internal API. Two functions are created to both init and deinit the fdisk context. Only the device's descriptor and path are added as a start and these are replaced throughout fdisk.c and label specific code.
Applied (and the rest of the patches too), thanks.

thanks.

Quoted 6 lines

Quoted 4 lines
All logic that opens the file does it with fdisk_new_context_from_filename(), and this enforces always opening the device with rw
I have added 'readonly' option to the fdisk_new_context_from_filename().

Well I wanted to leave it open for new features like when no device is passed (-l option IIRC) and read from /proc/partitions.

Quoted 6 lines

It would be better to minimize number of situation where we somewhere open devices read-write. For example udevd, udisks, ... are pretty sensitive to write operations (to detect new PT or new filesystem etc.).

Quoted 7 lines

Notes:
1/ What about to rename fdisk_new_context_from_filename() to
fdisk_new_context()? I guess that the device name is mandatory option.
2/ Please, don't use global 'cxt' if possible. See for example

Totally agree. I plan to gradually modify current functions to pass cxt as an argument instead of doing it globally - but it was kind of invasive for this pachset.

Quoted 5 lines

get_geometry() -> get_topology()
disk.c: In function ‘get_topology’:
fdisk.c:456:18: warning: unused parameter ‘fd’ [-Wunused-parameter]

Quoted 8 lines

there is get_geometry(cxt->fd, ...), but get_topology() ignores the parameter 'fd' and the function uses the global cxt->fd. This is nasty :-)
It would be better to rename global 'cxt' to something less generic and everywhere in functions use 'cxt' parameter, for example:
     get_geometry(struct fdisk_context *cxt, struct geom *g);

Yep, that's my plan.

Quoted 3 lines

Karel