Re: [PATCH] fdisk: isolate dos label logic

Davidlohr Bueso
2012-05-02T12:29:18Z

On Wed, 2012-05-02 at 14:17 +0200, Karel Zak wrote:
> On Sun, Apr 29, 2012 at 12:02:45AM +0200, Davidlohr Bueso wrote:
> > 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?

>
> > --- a/fdisk/fdisk.c
> > +++ b/fdisk/fdisk.c
> ...
> > -/*
> > - * 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 ...".
>
> > diff --git a/fdisk/fdisk.h b/fdisk/fdisk.h
> > index cff6b60..2032db7 100644
> > --- a/fdisk/fdisk.h
> > +++ b/fdisk/fdisk.h
> > @@ -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 {
> > @@ -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;
>
> > +/* 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
>
>
> >From 96f817fb169ac38fe5535cb8c6d2fdf9b2cb8f10 Mon Sep 17 00:00:00 2001
> From: Karel Zak <kzak@redhat.com>
> Date: Wed, 2 May 2012 14:05:51 +0200
> Subject: [PATCH] fdisk: fix fdiskdoslabel.c global variables
>
> Signed-off-by: Karel Zak <kzak@redhat.com>
> ---
> fdisk/fdisk.c | 4 +++-
> fdisk/fdisk.h | 21 ++++++++++-----------
> fdisk/fdiskdoslabel.c | 4 ++++
> fdisk/fdiskdoslabel.h | 14 ++++++++------
> 4 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/fdisk/fdisk.c b/fdisk/fdisk.c
> index 6695266..acc84d1 100644
> --- a/fdisk/fdisk.c
> +++ b/fdisk/fdisk.c
> @@ -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,
> diff --git a/fdisk/fdisk.h b/fdisk/fdisk.h
> index da86e30..3a1cfd7 100644
> --- a/fdisk/fdisk.h
> +++ b/fdisk/fdisk.h
> @@ -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);
> }
> diff --git a/fdisk/fdiskdoslabel.c b/fdisk/fdiskdoslabel.c
> index 04fdf84..6a9a044 100644
> --- a/fdisk/fdiskdoslabel.c
> +++ b/fdisk/fdiskdoslabel.c
> @@ -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)
> {
> diff --git a/fdisk/fdiskdoslabel.h b/fdisk/fdiskdoslabel.h
> index b444243..f5568df 100644
> --- a/fdisk/fdiskdoslabel.h
> +++ b/fdisk/fdiskdoslabel.h
> @@ -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);
> }
> --
> 1.7.7.6
>
>
>