[RFC] fdisk API

[RFC] fdisk API

[Davidlohr Bueso]

Hi,

The following structures are meant to represent and describe a single
device and the interactions that disk partitioners require. I would like
to know your opinion as we intend on gradually start adapting current
fdisk code around them.

It's important to mention that since flexibility is crucial, these are
opaque types and therefore all fields will be accessed through
interfaces -- same as libblkid. 

The functions themselves to use, populate/free these structures are
currently being worked on, and without wanting to overdesign things, it
will probably come in through patches. Again, I think we all want to
keep things simple.

Thanks,
Dave

-----

typedef unsigned long long sector_t;

enum fdisk_partition_type {
	PARTITION_NORMAL            = 0x00,
	PARTITION_LOGICAL           = 0x01,
	PARTITION_EXTENDED          = 0x02,
};

struct fdisk_sys_type {
 ... <--- copy/simplify struct systypes i386_sys_types[]
};

struct fdisk_partition {
	/* LBA geometry */
	sector_t p_start;
	sector_t p_end;
	sector_t p_len;

	int p_num;                        /* partition number */
	enum fdisk_partition_type p_type; /* partition type */
	struct fdisk_sys_type p_systype;  /* bsd, minix, swap, etc */
	unsigned char p_is_boot;          /* active/bootable */

	struct list_head p_list;          /* maintain dlist */
};

/*
 * All funtions return 0 on success and negative otherwise
 */
struct fdisk_operations {
	int (*add_partition)(struct fdisk_partition *new);
	int (*del_partition)(int partnum);
	int (*new_label)(void);
	int (*probe_label)(void);
};

struct fdisk_topology {
	 nsectors;
	sector_t t_lsector_size; /* logical */
	sector_t t_psector_size; /* physical */
	sector_t t_length;       /* size in bytes */
	sector_t t_min_io_size;
	sector_t t_io_size;      /* optimal */
};

/* 
 * Legacy CHS (cylinder-head-sector) based geometry
 * can this layout be used for LBA addressing?
 */
struct fdisk_geometry {
	unsigned int heads;
	unsigned int sectors;
	unsigned int cylinders;
};

struct fdisk_disk {
	const char *d_name; /* pt type, ie: bsd, dos */
	struct fdisk_operations *d_ops;
	struct fdisk_partition *d_partitions;

};

/* 
 * Represent a single device to be handled (ie: /dev/sda)
 */
struct fdisk_device {
	int dev_fd;                    /* device descriptor */  
	char *dev_path;                /* device name/path */
	struct stat dev_sb;         
	struct fdisk_disk *d_disk;
	struct fdisk_topology *d_topo;
	struct fdisk_geometry *d_kgeo; /* kernel/bios geometry */
};

Re: [RFC] fdisk API

[Petr Uzel]

[-- Attachment #1: Type: text/plain, Size: 3792 bytes --]

On Thu, Apr 26, 2012 at 02:48:05PM +0200, Davidlohr Bueso wrote:
> Hi,

Hi Dave,

> The following structures are meant to represent and describe a single
> device and the interactions that disk partitioners require. I would like
> to know your opinion as we intend on gradually start adapting current
> fdisk code around them.
> 
> It's important to mention that since flexibility is crucial, these are
> opaque types and therefore all fields will be accessed through
> interfaces -- same as libblkid. 
> 
> The functions themselves to use, populate/free these structures are
> currently being worked on, and without wanting to overdesign things, it
> will probably come in through patches. Again, I think we all want to
> keep things simple.
> 
> Thanks,
> Dave
> 
> -----
> 
> typedef unsigned long long sector_t;
> 
> enum fdisk_partition_type {
> 	PARTITION_NORMAL            = 0x00,
> 	PARTITION_LOGICAL           = 0x01,
> 	PARTITION_EXTENDED          = 0x02,
> };

This applies only for msdos label. With GPT (and other pt labels we
might eventually want to support), there is nothing like
extended or logical. E.g. parted maintains list of valid partition
types per label.

> struct fdisk_sys_type {
>  ... <--- copy/simplify struct systypes i386_sys_types[]
> };

This is not applicable for GPT. What about adding something like
'void *label_data' to fdisk_partition to support these per-PT label data?

I'm also missing a structure to describe a label as a whole, not only
the individual partition entries.
> 
> struct fdisk_partition {
> 	/* LBA geometry */
> 	sector_t p_start;
> 	sector_t p_end;
> 	sector_t p_len;
> 
> 	int p_num;                        /* partition number */
> 	enum fdisk_partition_type p_type; /* partition type */
> 	struct fdisk_sys_type p_systype;  /* bsd, minix, swap, etc */
> 	unsigned char p_is_boot;          /* active/bootable */

Boot flag is also label specific.

> 
> 	struct list_head p_list;          /* maintain dlist */
> };
> 
> /*
>  * All funtions return 0 on success and negative otherwise
>  */
> struct fdisk_operations {
> 	int (*add_partition)(struct fdisk_partition *new);
> 	int (*del_partition)(int partnum);
> 	int (*new_label)(void);
> 	int (*probe_label)(void);
> };

Apart from add_partition, these prototypes do not allow any control over
the operation. Also, shouldn't the functions take a pointer to struct
fdisk_device ?

[Not a big deal for now since these functions are not supposed to be
part of any future API, right?]
> 
> struct fdisk_topology {
> 	 nsectors;
> 	sector_t t_lsector_size; /* logical */
> 	sector_t t_psector_size; /* physical */
> 	sector_t t_length;       /* size in bytes */
> 	sector_t t_min_io_size;
> 	sector_t t_io_size;      /* optimal */
> };

Having both nsectors and t_length is superfluous (with
t_lsector_size). Does it have any advantage to have both?

> /* 
>  * Legacy CHS (cylinder-head-sector) based geometry
>  * can this layout be used for LBA addressing?
>  */
> struct fdisk_geometry {
> 	unsigned int heads;
> 	unsigned int sectors;
> 	unsigned int cylinders;
> };
> 
> struct fdisk_disk {
> 	const char *d_name; /* pt type, ie: bsd, dos */
> 	struct fdisk_operations *d_ops;
> 	struct fdisk_partition *d_partitions;
> 
> };
> 
> /* 
>  * Represent a single device to be handled (ie: /dev/sda)
>  */
> struct fdisk_device {
> 	int dev_fd;                    /* device descriptor */  
> 	char *dev_path;                /* device name/path */
> 	struct stat dev_sb;         
> 	struct fdisk_disk *d_disk;
> 	struct fdisk_topology *d_topo;
> 	struct fdisk_geometry *d_kgeo; /* kernel/bios geometry */
> };
> 

Petr

-- 
Petr Uzel
IRC: ptr_uzl @ freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] fdisk API

[Davidlohr Bueso]

On Thu, 2012-04-26 at 15:49 +0200, Petr Uzel wrote:
> On Thu, Apr 26, 2012 at 02:48:05PM +0200, Davidlohr Bueso wrote:
> > Hi,
> 
> Hi Dave,
> 
> > The following structures are meant to represent and describe a single
> > device and the interactions that disk partitioners require. I would like
> > to know your opinion as we intend on gradually start adapting current
> > fdisk code around them.
> > 
> > It's important to mention that since flexibility is crucial, these are
> > opaque types and therefore all fields will be accessed through
> > interfaces -- same as libblkid. 
> > 
> > The functions themselves to use, populate/free these structures are
> > currently being worked on, and without wanting to overdesign things, it
> > will probably come in through patches. Again, I think we all want to
> > keep things simple.
> > 
> > Thanks,
> > Dave
> > 
> > -----
> > 
> > typedef unsigned long long sector_t;
> > 
> > enum fdisk_partition_type {
> > 	PARTITION_NORMAL            = 0x00,
> > 	PARTITION_LOGICAL           = 0x01,
> > 	PARTITION_EXTENDED          = 0x02,
> > };
> 
> This applies only for msdos label. With GPT (and other pt labels we
> might eventually want to support), there is nothing like
> extended or logical. E.g. parted maintains list of valid partition
> types per label.
> 
> > struct fdisk_sys_type {
> >  ... <--- copy/simplify struct systypes i386_sys_types[]
> > };
> 
> This is not applicable for GPT. What about adding something like
> 'void *label_data' to fdisk_partition to support these per-PT label data?

Makes sense. Consider as well that if it's not applicable to GPT it
might be an exception, since all other labels *do* require it.

> 
> I'm also missing a structure to describe a label as a whole, not only
> the individual partition entries.

Well, these structures are internal to each label and wasn't planning on
touching them. I only planned on having existing code fill
fdisk_operations and call that instead of always checking label type. If
(disklabel == DOS) add_dos_partition().

> > 
> > struct fdisk_partition {
> > 	/* LBA geometry */
> > 	sector_t p_start;
> > 	sector_t p_end;
> > 	sector_t p_len;
> > 
> > 	int p_num;                        /* partition number */
> > 	enum fdisk_partition_type p_type; /* partition type */
> > 	struct fdisk_sys_type p_systype;  /* bsd, minix, swap, etc */
> > 	unsigned char p_is_boot;          /* active/bootable */
> 
> Boot flag is also label specific.
> 
> > 
> > 	struct list_head p_list;          /* maintain dlist */
> > };
> > 
> > /*
> >  * All funtions return 0 on success and negative otherwise
> >  */
> > struct fdisk_operations {
> > 	int (*add_partition)(struct fdisk_partition *new);
> > 	int (*del_partition)(int partnum);
> > 	int (*new_label)(void);
> > 	int (*probe_label)(void);
> > };
> 
> Apart from add_partition, these prototypes do not allow any control over
> the operation. Also, shouldn't the functions take a pointer to struct
> fdisk_device ?

Well, this also goes for leaving current label code alone. If you see
fdisklabel.c, for example, it already controls its own label-specific
operations.

> 
> [Not a big deal for now since these functions are not supposed to be
> part of any future API, right?]

Although true, I'd like to get it right now and not have to rethink it
alter when implementing the API perse.

> > 
> > struct fdisk_topology {
> > 	 nsectors;
> > 	sector_t t_lsector_size; /* logical */
> > 	sector_t t_psector_size; /* physical */
> > 	sector_t t_length;       /* size in bytes */
> > 	sector_t t_min_io_size;
> > 	sector_t t_io_size;      /* optimal */
> > };
> 
> Having both nsectors and t_length is superfluous (with
> t_lsector_size). Does it have any advantage to have both?

IIRC when looking at the code, nsectors can obtained through ioctl. In
any case I'm planning on using blkid for most of the topology stuff.

> 
> > /* 
> >  * Legacy CHS (cylinder-head-sector) based geometry
> >  * can this layout be used for LBA addressing?
> >  */
> > struct fdisk_geometry {
> > 	unsigned int heads;
> > 	unsigned int sectors;
> > 	unsigned int cylinders;
> > };
> > 
> > struct fdisk_disk {
> > 	const char *d_name; /* pt type, ie: bsd, dos */
> > 	struct fdisk_operations *d_ops;
> > 	struct fdisk_partition *d_partitions;
> > 
> > };
> > 
> > /* 
> >  * Represent a single device to be handled (ie: /dev/sda)
> >  */
> > struct fdisk_device {
> > 	int dev_fd;                    /* device descriptor */  
> > 	char *dev_path;                /* device name/path */
> > 	struct stat dev_sb;         
> > 	struct fdisk_disk *d_disk;
> > 	struct fdisk_topology *d_topo;
> > 	struct fdisk_geometry *d_kgeo; /* kernel/bios geometry */
> > };
> > 
> 
> Petr
> 

Thanks for reviewing,
Davidlohr

Re: [RFC] fdisk API

[Karel Zak]

On Thu, Apr 26, 2012 at 02:48:05PM +0200, Davidlohr Bueso wrote:
> typedef unsigned long long sector_t;
> 
> enum fdisk_partition_type {
> 	PARTITION_NORMAL            = 0x00,
> 	PARTITION_LOGICAL           = 0x01,
> 	PARTITION_EXTENDED          = 0x02,
> };
> 
> struct fdisk_sys_type {
>  ... <--- copy/simplify struct systypes i386_sys_types[]
> };

I agree with Petr, this is label specific -- all this specific
stuff should be in label specific file (e.g. mbr.c).

I understand (and agree) that you want to remove all the stupid
"if(disktype == FOO)" from the code, but then it seems that very
lightweight API is necessary only. From my point of view all
we need is:

  - operations
  - very basic and generic partitions description
  - device topology (i/o limits)
  - any top-level struct (context) where all stuff will be connected
    together.

That's all. Don't forget that tasks like "list all partitions" will be
also label specific (for DOS you need extended/primary/logical, for
GPT type UUIDs, ...).

> struct fdisk_partition {
> 	/* LBA geometry */
> 	sector_t p_start;
> 	sector_t p_end;
> 	sector_t p_len;

Why we need "p_end"? It seems that end = start + len.

> 	int p_num;                        /* partition number */

> 	enum fdisk_partition_type p_type; /* partition type */
> 	struct fdisk_sys_type p_systype;  /* bsd, minix, swap, etc */
> 	unsigned char p_is_boot;          /* active/bootable */

label specific, unnecessary here

> 
> 	struct list_head p_list;          /* maintain dlist */
> };

Can you describe interaction between libblkid (partitions probing) and
fdisk? Would be possible to use libblkid to get the current on-disk
layout? (Then we can support (at least) "fdisk -l"  for almost
arbitrary partition table type.)

Maybe you can also add "blkid_partition *ondisk" with the original
partition layout.

And as said Petr, "void *data" for private label specific data.

> /*
>  * All funtions return 0 on success and negative otherwise
>  */
> struct fdisk_operations {
> 	int (*add_partition)(struct fdisk_partition *new);
> 	int (*del_partition)(int partnum);
> 	int (*new_label)(void);
> 	int (*probe_label)(void);
> };

My experience is that the best is to create any central "context"
struct and use the struct in all high-level functions. This solution
allows to extend API in arbitrary way without API breaks.

   int (*add_partition)(struct fdisk *fdisk);

and if you want to add some new functionality then you only need to
add a new function to modify "struct fdisk" before you call add_partition().

Anyway, what about another operations like move-begin-of-partition,
set-partition-type, ... it seems that many many operations in fdisk
menu are label specific.


> struct fdisk_topology {
> 	 nsectors;
> 	sector_t t_lsector_size; /* logical */
> 	sector_t t_psector_size; /* physical */
> 	sector_t t_length;       /* size in bytes */
> 	sector_t t_min_io_size;
> 	sector_t t_io_size;      /* optimal */
> };

This is incomplete (you also need alignment_offset). Anyway, we
already have this struct in libblkid/src/topology/topology.c and all
is exported by blkid_topology_get_* functions.

What about to reuse the stuff from libblkid (especially if you want to
read the info from lbblkid).

> /* 
>  * Legacy CHS (cylinder-head-sector) based geometry
>  * can this layout be used for LBA addressing?
>  */
> struct fdisk_geometry {
> 	unsigned int heads;
> 	unsigned int sectors;
> 	unsigned int cylinders;
> };

Oh, CHS has to die! ;-)

> struct fdisk_disk {
> 	const char *d_name; /* pt type, ie: bsd, dos */

d_ptname or d_labelname ?

> 	struct fdisk_operations *d_ops;
> 	struct fdisk_partition *d_partitions;
> 
> };

but is it really necessary to have fdisk_disk?

> /* 
>  * Represent a single device to be handled (ie: /dev/sda)
>  */
> struct fdisk_device {
> 	int dev_fd;                    /* device descriptor */  
> 	char *dev_path;                /* device name/path */
> 	struct stat dev_sb;         
> 	struct fdisk_disk *d_disk;
> 	struct fdisk_topology *d_topo;
> 	struct fdisk_geometry *d_kgeo; /* kernel/bios geometry */
> };

What about to rename this to "struct fdisk" or "struct fdisk_context" and
use it as central struct for all fdisk stuff?

struct fdisk {
    const char *pttype;
    int dev_fd;
    char *dev_path;
    struct stat dev_sb;

    blkid_topology  *topology;
    struct fdisk_operations *ops;
    struct fdisk_partition *partitions;

    void *pt_data;  /* private layout data */
};


And:

 * debug support, the best thing on libmount and libblkid is
   LIB<NAME>_DEBUG=<mask> :-)

  - please see libmount/src/mountP.h and #ifdef CONFIG_LIBMOUNT_DEBUG
  - and libmount/src/init.c
  - and DBG() macros in code

 * use assert() (see also libmount)

    Karel

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