On Tue, 2012-07-24 at 11:41 +0200, Petr Uzel wrote:
> On Sun, Jul 22, 2012 at 07:05:01PM +0200, Davidlohr Bueso wrote:
> > From: Davidlohr Bueso <dave@xxxxxxx>
> >
> > The context structure is the fdisk API's main data type as it keeps all data together. Add the
> > label structure to it, so that the pt-specific operations can be called from the context.
> >
> > The internal probing function is updated so that if a label is not probed, for example when a
> > disk is not formated, it will default to use either sun or dos label operations. This avoids
> > scenarios that could crash the program if no label is detected - ie: right after creating a device.
>
> Wouldn't it be nicer to have dummy/fallback entry for unknown/nonexistent label
> type, e.g. 'no_label'. It's operations could return something like
> NO_LABEL_PLS_CREATE_IT_FIRST. fdisk_context would be initialized to
> point to this dummy label type and later overridden by successful label
> probe.
It's an option, however it would break current functionality and the
user would be forced into creating a new disklabel - there's really no
point in using fdisk if no label is being created.
Thanks,
Davidlohr
>
>
> > Signed-off-by: Davidlohr Bueso <dave@xxxxxxx>
> > ---
> > fdisks/fdisk.c | 4 ++--
> > fdisks/fdisk.h | 23 +++++++++++++++--------
> > fdisks/utils.c | 35 ++++++++++++++++++++++++++++-------
> > 3 files changed, 45 insertions(+), 17 deletions(-)
> >
> > diff --git a/fdisks/fdisk.c b/fdisks/fdisk.c
> > index 3b9bd7b..46322bc 100644
> > --- a/fdisks/fdisk.c
> > +++ b/fdisks/fdisk.c
> > @@ -68,7 +68,7 @@ int MBRbuffer_changed;
> > struct menulist_descr {
> > char command; /* command key */
> > char *description; /* command description */
> > - enum labeltype label[2]; /* disklabel types associated with main and expert menu */
> > + enum fdisk_labeltype label[2]; /* disklabel types associated with main and expert menu */
> > };
> >
> > static const struct menulist_descr menulist[] = {
> > @@ -138,7 +138,7 @@ unsigned int user_cylinders, user_heads, user_sectors;
> > sector_t sector_offset = 1;
> > unsigned int units_per_sector = 1, display_in_cyl_units = 0;
> > unsigned long grain = DEFAULT_SECTOR_SIZE;
> > -enum labeltype disklabel; /* Current disklabel */
> > +enum fdisk_labeltype disklabel; /* Current disklabel */
> >
> > static void __attribute__ ((__noreturn__)) usage(FILE *out)
> > {
> > diff --git a/fdisks/fdisk.h b/fdisks/fdisk.h
> > index 8107aa8..d716824 100644
> > --- a/fdisks/fdisk.h
> > +++ b/fdisks/fdisk.h
> > @@ -132,6 +132,8 @@ struct fdisk_context {
> > /* geometry */
> > sector_t total_sectors; /* in logical sectors */
> > struct fdisk_geometry geom;
> > +
> > + struct fdisk_label *label;
> > };
> >
> > /*
> > @@ -139,6 +141,8 @@ struct fdisk_context {
> > */
> > struct fdisk_label {
> > const char *name;
> > +
> > + /* probe disk label */
> > int (*probe)(struct fdisk_context *cxt);
> > };
> >
> > @@ -206,17 +210,20 @@ extern const char * str_units(int);
> >
> > extern sector_t get_nr_sects(struct partition *p);
> >
> > -enum labeltype {
> > +/*
> > + * Supported partition table types (labels)
> > + */
> > +enum fdisk_labeltype {
> > + ANY_LABEL = -1,
> > DOS_LABEL = 1,
> > - SUN_LABEL = 2,
> > - SGI_LABEL = 4,
> > - AIX_LABEL = 8,
> > - OSF_LABEL = 16,
> > - MAC_LABEL = 32,
> > - ANY_LABEL = -1
> > + SUN_LABEL,
> > + SGI_LABEL,
> > + AIX_LABEL,
> > + OSF_LABEL,
> > + MAC_LABEL,
> > };
> >
> > -extern enum labeltype disklabel;
> > +extern enum fdisk_labeltype disklabel;
> > extern int MBRbuffer_changed;
> > extern unsigned long grain;
> >
> > diff --git a/fdisks/utils.c b/fdisks/utils.c
> > index 363f7aa..cf9484c 100644
> > --- a/fdisks/utils.c
> > +++ b/fdisks/utils.c
> > @@ -36,11 +36,11 @@ int fdisk_debug_mask;
> > */
> > static const struct fdisk_label *labels[] =
> > {
> > - &bsd_label,
> > &dos_label,
> > - &sgi_label,
> > &sun_label,
> > + &sgi_label,
> > &aix_label,
> > + &bsd_label,
> > &mac_label,
> > };
>
> Is the ordering of labels important here? If so, it should be documented.
>
>
> >
> > @@ -51,14 +51,34 @@ static int __probe_labels(struct fdisk_context *cxt)
> > disklabel = ANY_LABEL;
> > update_units(cxt);
> >
> > + cxt->label = calloc(1, sizeof(struct fdisk_label));
> > + if (!cxt->label) {
> > + rc = FDISK_ERROR_MEM;
> > + goto done;
> > + }
> > +
> > for (i = 0; i < ARRAY_SIZE(labels); i++) {
> > rc = labels[i]->probe(cxt);
> > - if (!rc) {
> > - DBG(LABEL, dbgprint("detected a %s label\n",
> > - labels[i]->name));
> > - goto done;
> > - }
> > + if (rc)
> > + continue;
> > +
> > + /* found the label */
> > + DBG(LABEL, dbgprint("detected a %s label\n", labels[i]->name));
> > + goto copy;
> > }
> > +
> > + /*
> > + * Did not find any label - perhaps un formated;
> > + * initialize with default partition operations.
> > + */
> > +#ifdef __sparc__
> > + i = OSF_LABEL - 1;
> > +#else
> > + i = DOS_LABEL - 1;
> > +#endif
> > + DBG(LABEL, dbgprint("defaulting to a %s label\n", labels[i]->name));
> > +copy:
> > + memcpy(cxt->label, labels[i], sizeof(struct fdisk_label));
>
> Uh, may be I'm missing something, but why not just make cxt->label
> point to corresponding entry from *labels array?
>
>
> > done:
> > return rc;
> > }
> > @@ -380,6 +400,7 @@ void fdisk_free_context(struct fdisk_context *cxt)
> > DBG(CONTEXT, dbgprint("freeing context for %s", cxt->dev_path));
> > close(cxt->dev_fd);
> > free(cxt->dev_path);
> > + free(cxt->label);
> > free(cxt->mbr);
> > free(cxt);
> > }
> > --
> > 1.7.4.1
> >
> >
> >
> >
>
> Petr
>
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
