[PATCH 03/10] fdisk: API: add fdisk_label_change

[PATCH 03/10] fdisk: API: add fdisk_label_change

[Davidlohr Bueso]

From: Davidlohr Bueso <dave@gnu.org>

A new fdisk_label_change() function is added for situations when the disk label is
changed (ie: creating a new sun label). This function only updates the label pointer
in the context to use the newly specified label type.

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 fdisks/fdisk.h         |    2 ++
 fdisks/fdiskdoslabel.c |    1 +
 fdisks/fdisksgilabel.c |    2 ++
 fdisks/fdisksunlabel.c |    1 +
 fdisks/utils.c         |   37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/fdisks/fdisk.h b/fdisks/fdisk.h
index d716824..d7e85f5 100644
--- a/fdisks/fdisk.h
+++ b/fdisks/fdisk.h
@@ -104,6 +104,7 @@ enum fdisk_error {
 	FDISK_ERROR_WRITE,
 	FDISK_ERROR_IOCTL,
 	FDISK_ERROR_PROBE,
+	FDISK_ERROR_NOLABEL,
 	FDISK_ERROR_UNKNOWN
 };
 
@@ -164,6 +165,7 @@ extern void fdisk_mbr_zeroize(struct fdisk_context *cxt);
 extern void fdisk_geom_set_cyls(struct fdisk_context *cxt);
 extern const char *fdisk_error_name(enum fdisk_error errcode);
 extern void fdisk_error_fatal(struct fdisk_context *cxt, enum fdisk_error errcode);
+extern int fdisk_label_change(struct fdisk_context *cxt, const char *name);
 
 /* prototypes for fdisk.c */
 extern char *disk_device, *line_ptr;
diff --git a/fdisks/fdiskdoslabel.c b/fdisks/fdiskdoslabel.c
index 535afdc..9b9b23a 100644
--- a/fdisks/fdiskdoslabel.c
+++ b/fdisks/fdiskdoslabel.c
@@ -230,6 +230,7 @@ void create_doslabel(struct fdisk_context *cxt)
 
 	dos_init(cxt);
 	fdisk_mbr_zeroize(cxt);
+	fdisk_label_change(cxt, "dos");
 	set_all_unchanged();
 	set_changed(0);
 
diff --git a/fdisks/fdisksgilabel.c b/fdisks/fdisksgilabel.c
index e38d98f..6001038 100644
--- a/fdisks/fdisksgilabel.c
+++ b/fdisks/fdisksgilabel.c
@@ -780,6 +780,8 @@ create_sgilabel(struct fdisk_context *cxt)
 		}
 
 	fdisk_mbr_zeroize(cxt);
+	fdisk_label_change(cxt, "sgi");
+
 	sgilabel->magic = SSWAP32(SGI_LABEL_MAGIC);
 	sgilabel->boot_part = SSWAP16(0);
 	sgilabel->swap_part = SSWAP16(1);
diff --git a/fdisks/fdisksunlabel.c b/fdisks/fdisksunlabel.c
index 4123806..b63335c 100644
--- a/fdisks/fdisksunlabel.c
+++ b/fdisks/fdisksunlabel.c
@@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
 
 	init();
 	fdisk_mbr_zeroize(cxt);
+	fdisk_label_change(cxt, "sun");
 
 	sunlabel->magic = SSWAP16(SUN_LABEL_MAGIC);
 	sunlabel->sanity = SSWAP32(SUN_LABEL_SANE);
diff --git a/fdisks/utils.c b/fdisks/utils.c
index cf9484c..48dedfb 100644
--- a/fdisks/utils.c
+++ b/fdisks/utils.c
@@ -44,6 +44,43 @@ static const struct fdisk_label *labels[] =
 	&mac_label,
 };
 
+/**
+ * fdisk_label_change:
+ * @cxt: fdisk context
+ * @name: new label name
+ *
+ * Updates the disk label type to the one specified by @name.
+ *
+ * Returns 0 on success, otherwise, a corresponding error.
+ */
+int fdisk_label_change(struct fdisk_context *cxt, const char *name)
+{
+	int i;
+
+	if (!cxt || !cxt->label || !name)
+		return FDISK_ERROR_UNKNOWN;
+
+	/* not really changing the label */
+	if (!strncmp(name, cxt->label->name, strlen(name)))
+		goto done;
+
+	for (i = 0; i < ARRAY_SIZE(labels); i++) {
+		if (strncmp(name, labels[i]->name, strlen(name)))
+			continue;
+	
+		/* found the new label */
+		memset(cxt->label, 0, sizeof(struct fdisk_label));
+		memcpy(cxt->label, labels[i], sizeof(struct fdisk_label));
+		DBG(LABEL, dbgprint("changing to a %s label\n", labels[i]->name));
+		goto done;
+	}
+
+	/* couldn't find the requested label type */
+	return FDISK_ERROR_NOLABEL;
+done:
+	return 0;
+}
+
 static int __probe_labels(struct fdisk_context *cxt)
 {
 	int i, rc = 0;
-- 
1.7.4.1

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Bernhard Voelker]

On 07/22/2012 07:05 PM, Davidlohr Bueso wrote:
> --- a/fdisks/fdisksunlabel.c
> +++ b/fdisks/fdisksunlabel.c
> @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
>  
>  	init();
>  	fdisk_mbr_zeroize(cxt);
> +	fdisk_label_change(cxt, "sun");
>  

...

> +int fdisk_label_change(struct fdisk_context *cxt, const char *name)
> +{
> +	int i;
> +
> +	if (!cxt || !cxt->label || !name)
> +		return FDISK_ERROR_UNKNOWN;
> +
> +	/* not really changing the label */
> +	if (!strncmp(name, cxt->label->name, strlen(name)))
> +		goto done;
> +
> +	for (i = 0; i < ARRAY_SIZE(labels); i++) {
> +		if (strncmp(name, labels[i]->name, strlen(name)))
> +			continue;

Shouldn't we use something else than hardcoded "sun"|"dos"|"sgi"|...
strings here? (See also "[PATCH 08/10] fdisk: API: add create disklabel
to label operations".)

Have a nice day,
Berny

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Petr Uzel]

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

On Sun, Jul 22, 2012 at 07:05:04PM +0200, Davidlohr Bueso wrote:
> From: Davidlohr Bueso <dave@gnu.org>
> 
> A new fdisk_label_change() function is added for situations when the disk label is
> changed (ie: creating a new sun label). This function only updates the label pointer
> in the context to use the newly specified label type.

Why can't we just call __probe_labels() after creating new label, e.g.
in soon-to-be introduced fdisk_label_create()?

> 
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
>  fdisks/fdisk.h         |    2 ++
>  fdisks/fdiskdoslabel.c |    1 +
>  fdisks/fdisksgilabel.c |    2 ++
>  fdisks/fdisksunlabel.c |    1 +
>  fdisks/utils.c         |   37 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/fdisks/fdisk.h b/fdisks/fdisk.h
> index d716824..d7e85f5 100644
> --- a/fdisks/fdisk.h
> +++ b/fdisks/fdisk.h
> @@ -104,6 +104,7 @@ enum fdisk_error {
>  	FDISK_ERROR_WRITE,
>  	FDISK_ERROR_IOCTL,
>  	FDISK_ERROR_PROBE,
> +	FDISK_ERROR_NOLABEL,
>  	FDISK_ERROR_UNKNOWN
>  };
>  
> @@ -164,6 +165,7 @@ extern void fdisk_mbr_zeroize(struct fdisk_context *cxt);
>  extern void fdisk_geom_set_cyls(struct fdisk_context *cxt);
>  extern const char *fdisk_error_name(enum fdisk_error errcode);
>  extern void fdisk_error_fatal(struct fdisk_context *cxt, enum fdisk_error errcode);
> +extern int fdisk_label_change(struct fdisk_context *cxt, const char *name);
>  
>  /* prototypes for fdisk.c */
>  extern char *disk_device, *line_ptr;
> diff --git a/fdisks/fdiskdoslabel.c b/fdisks/fdiskdoslabel.c
> index 535afdc..9b9b23a 100644
> --- a/fdisks/fdiskdoslabel.c
> +++ b/fdisks/fdiskdoslabel.c
> @@ -230,6 +230,7 @@ void create_doslabel(struct fdisk_context *cxt)
>  
>  	dos_init(cxt);
>  	fdisk_mbr_zeroize(cxt);
> +	fdisk_label_change(cxt, "dos");
>  	set_all_unchanged();
>  	set_changed(0);
>  
> diff --git a/fdisks/fdisksgilabel.c b/fdisks/fdisksgilabel.c
> index e38d98f..6001038 100644
> --- a/fdisks/fdisksgilabel.c
> +++ b/fdisks/fdisksgilabel.c
> @@ -780,6 +780,8 @@ create_sgilabel(struct fdisk_context *cxt)
>  		}
>  
>  	fdisk_mbr_zeroize(cxt);
> +	fdisk_label_change(cxt, "sgi");
> +
>  	sgilabel->magic = SSWAP32(SGI_LABEL_MAGIC);
>  	sgilabel->boot_part = SSWAP16(0);
>  	sgilabel->swap_part = SSWAP16(1);
> diff --git a/fdisks/fdisksunlabel.c b/fdisks/fdisksunlabel.c
> index 4123806..b63335c 100644
> --- a/fdisks/fdisksunlabel.c
> +++ b/fdisks/fdisksunlabel.c
> @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
>  
>  	init();
>  	fdisk_mbr_zeroize(cxt);
> +	fdisk_label_change(cxt, "sun");
>  
>  	sunlabel->magic = SSWAP16(SUN_LABEL_MAGIC);
>  	sunlabel->sanity = SSWAP32(SUN_LABEL_SANE);
> diff --git a/fdisks/utils.c b/fdisks/utils.c
> index cf9484c..48dedfb 100644
> --- a/fdisks/utils.c
> +++ b/fdisks/utils.c
> @@ -44,6 +44,43 @@ static const struct fdisk_label *labels[] =
>  	&mac_label,
>  };
>  
> +/**
> + * fdisk_label_change:
> + * @cxt: fdisk context
> + * @name: new label name
> + *
> + * Updates the disk label type to the one specified by @name.
> + *
> + * Returns 0 on success, otherwise, a corresponding error.
> + */
> +int fdisk_label_change(struct fdisk_context *cxt, const char *name)
> +{
> +	int i;
> +
> +	if (!cxt || !cxt->label || !name)
> +		return FDISK_ERROR_UNKNOWN;
> +
> +	/* not really changing the label */
> +	if (!strncmp(name, cxt->label->name, strlen(name)))
> +		goto done;
> +
> +	for (i = 0; i < ARRAY_SIZE(labels); i++) {
> +		if (strncmp(name, labels[i]->name, strlen(name)))
> +			continue;
> +	
> +		/* found the new label */
> +		memset(cxt->label, 0, sizeof(struct fdisk_label));
> +		memcpy(cxt->label, labels[i], sizeof(struct fdisk_label));
> +		DBG(LABEL, dbgprint("changing to a %s label\n", labels[i]->name));
> +		goto done;
> +	}
> +
> +	/* couldn't find the requested label type */
> +	return FDISK_ERROR_NOLABEL;
> +done:
> +	return 0;
> +}
> +
>  static int __probe_labels(struct fdisk_context *cxt)
>  {
>  	int i, rc = 0;
> -- 
> 1.7.4.1
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Petr

-- 
Petr Uzel
IRC: ptr_uzl @ freenode

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

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Petr Uzel]

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

On Tue, Jul 24, 2012 at 11:47:53AM +0200, Bernhard Voelker wrote:
> 
> 
> On 07/22/2012 07:05 PM, Davidlohr Bueso wrote:
> > --- a/fdisks/fdisksunlabel.c
> > +++ b/fdisks/fdisksunlabel.c
> > @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
> >  
> >  	init();
> >  	fdisk_mbr_zeroize(cxt);
> > +	fdisk_label_change(cxt, "sun");
> >  
> 
> ...
> 
> > +int fdisk_label_change(struct fdisk_context *cxt, const char *name)
> > +{
> > +	int i;
> > +
> > +	if (!cxt || !cxt->label || !name)
> > +		return FDISK_ERROR_UNKNOWN;
> > +
> > +	/* not really changing the label */
> > +	if (!strncmp(name, cxt->label->name, strlen(name)))
> > +		goto done;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(labels); i++) {
> > +		if (strncmp(name, labels[i]->name, strlen(name)))
> > +			continue;
> 
> Shouldn't we use something else than hardcoded "sun"|"dos"|"sgi"|...
> strings here? 

What's the problem with these strings? I think these are well
recognized with clear meaning. What alternative do you propose?

Thanks,

        Petr


> (See also "[PATCH 08/10] fdisk: API: add create disklabel
> to label operations".)
> 
> Have a nice day,
> Berny
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Petr

-- 
Petr Uzel
IRC: ptr_uzl @ freenode

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

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Bernhard Voelker]

On 07/24/2012 11:56 AM, Petr Uzel wrote:
> On Tue, Jul 24, 2012 at 11:47:53AM +0200, Bernhard Voelker wrote:
>>
>>
>> On 07/22/2012 07:05 PM, Davidlohr Bueso wrote:
>>> --- a/fdisks/fdisksunlabel.c
>>> +++ b/fdisks/fdisksunlabel.c
>>> @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
>>>  
>>>  	init();
>>>  	fdisk_mbr_zeroize(cxt);
>>> +	fdisk_label_change(cxt, "sun");
>>>  
>>

>>
>> Shouldn't we use something else than hardcoded "sun"|"dos"|"sgi"|...
>> strings here? 
> 
> What's the problem with these strings? I think these are well
> recognized with clear meaning. What alternative do you propose?

Sorry, I should've proposed this already in my first mail:

-	fdisk_label_change(cxt, "sun");
+	fdisk_label_change(cxt, sun_label.name);

It's already defined ;-)

Have a nice day,
Berny

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Davidlohr Bueso]

On Tue, 2012-07-24 at 12:32 +0200, Bernhard Voelker wrote:
> 
> On 07/24/2012 11:56 AM, Petr Uzel wrote:
> > On Tue, Jul 24, 2012 at 11:47:53AM +0200, Bernhard Voelker wrote:
> >>
> >>
> >> On 07/22/2012 07:05 PM, Davidlohr Bueso wrote:
> >>> --- a/fdisks/fdisksunlabel.c
> >>> +++ b/fdisks/fdisksunlabel.c
> >>> @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
> >>>  
> >>>  	init();
> >>>  	fdisk_mbr_zeroize(cxt);
> >>> +	fdisk_label_change(cxt, "sun");
> >>>  
> >>
> 
> >>
> >> Shouldn't we use something else than hardcoded "sun"|"dos"|"sgi"|...
> >> strings here? 
> > 
> > What's the problem with these strings? I think these are well
> > recognized with clear meaning. What alternative do you propose?
> 
> Sorry, I should've proposed this already in my first mail:
> 
> -	fdisk_label_change(cxt, "sun");
> +	fdisk_label_change(cxt, sun_label.name);
> 

I'm planning on adding the disklabel type (numeric) to the context
structure in a near future, so we'd then have two ways of identifying
labels. I really do not see much difference between numbers and strings
- this is not a performance critical program.

Thanks,
Davidlohr

> It's already defined ;-)
> 
> Have a nice day,
> Berny
> 
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Davidlohr Bueso]

On Tue, 2012-07-24 at 11:52 +0200, Petr Uzel wrote:
> On Sun, Jul 22, 2012 at 07:05:04PM +0200, Davidlohr Bueso wrote:
> > From: Davidlohr Bueso <dave@gnu.org>
> > 
> > A new fdisk_label_change() function is added for situations when the disk label is
> > changed (ie: creating a new sun label). This function only updates the label pointer
> > in the context to use the newly specified label type.
> 
> Why can't we just call __probe_labels() after creating new label, e.g.
> in soon-to-be introduced fdisk_label_create()?

Because this way we give users extra functionality in the API. There
should be a way of changing label types on the fly, and since we don't
export __probe_labels(), this is the other option.

Thanks,
Davidlohr

> 
> > 
> > Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> > ---
> >  fdisks/fdisk.h         |    2 ++
> >  fdisks/fdiskdoslabel.c |    1 +
> >  fdisks/fdisksgilabel.c |    2 ++
> >  fdisks/fdisksunlabel.c |    1 +
> >  fdisks/utils.c         |   37 +++++++++++++++++++++++++++++++++++++
> >  5 files changed, 43 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fdisks/fdisk.h b/fdisks/fdisk.h
> > index d716824..d7e85f5 100644
> > --- a/fdisks/fdisk.h
> > +++ b/fdisks/fdisk.h
> > @@ -104,6 +104,7 @@ enum fdisk_error {
> >  	FDISK_ERROR_WRITE,
> >  	FDISK_ERROR_IOCTL,
> >  	FDISK_ERROR_PROBE,
> > +	FDISK_ERROR_NOLABEL,
> >  	FDISK_ERROR_UNKNOWN
> >  };
> >  
> > @@ -164,6 +165,7 @@ extern void fdisk_mbr_zeroize(struct fdisk_context *cxt);
> >  extern void fdisk_geom_set_cyls(struct fdisk_context *cxt);
> >  extern const char *fdisk_error_name(enum fdisk_error errcode);
> >  extern void fdisk_error_fatal(struct fdisk_context *cxt, enum fdisk_error errcode);
> > +extern int fdisk_label_change(struct fdisk_context *cxt, const char *name);
> >  
> >  /* prototypes for fdisk.c */
> >  extern char *disk_device, *line_ptr;
> > diff --git a/fdisks/fdiskdoslabel.c b/fdisks/fdiskdoslabel.c
> > index 535afdc..9b9b23a 100644
> > --- a/fdisks/fdiskdoslabel.c
> > +++ b/fdisks/fdiskdoslabel.c
> > @@ -230,6 +230,7 @@ void create_doslabel(struct fdisk_context *cxt)
> >  
> >  	dos_init(cxt);
> >  	fdisk_mbr_zeroize(cxt);
> > +	fdisk_label_change(cxt, "dos");
> >  	set_all_unchanged();
> >  	set_changed(0);
> >  
> > diff --git a/fdisks/fdisksgilabel.c b/fdisks/fdisksgilabel.c
> > index e38d98f..6001038 100644
> > --- a/fdisks/fdisksgilabel.c
> > +++ b/fdisks/fdisksgilabel.c
> > @@ -780,6 +780,8 @@ create_sgilabel(struct fdisk_context *cxt)
> >  		}
> >  
> >  	fdisk_mbr_zeroize(cxt);
> > +	fdisk_label_change(cxt, "sgi");
> > +
> >  	sgilabel->magic = SSWAP32(SGI_LABEL_MAGIC);
> >  	sgilabel->boot_part = SSWAP16(0);
> >  	sgilabel->swap_part = SSWAP16(1);
> > diff --git a/fdisks/fdisksunlabel.c b/fdisks/fdisksunlabel.c
> > index 4123806..b63335c 100644
> > --- a/fdisks/fdisksunlabel.c
> > +++ b/fdisks/fdisksunlabel.c
> > @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
> >  
> >  	init();
> >  	fdisk_mbr_zeroize(cxt);
> > +	fdisk_label_change(cxt, "sun");
> >  
> >  	sunlabel->magic = SSWAP16(SUN_LABEL_MAGIC);
> >  	sunlabel->sanity = SSWAP32(SUN_LABEL_SANE);
> > diff --git a/fdisks/utils.c b/fdisks/utils.c
> > index cf9484c..48dedfb 100644
> > --- a/fdisks/utils.c
> > +++ b/fdisks/utils.c
> > @@ -44,6 +44,43 @@ static const struct fdisk_label *labels[] =
> >  	&mac_label,
> >  };
> >  
> > +/**
> > + * fdisk_label_change:
> > + * @cxt: fdisk context
> > + * @name: new label name
> > + *
> > + * Updates the disk label type to the one specified by @name.
> > + *
> > + * Returns 0 on success, otherwise, a corresponding error.
> > + */
> > +int fdisk_label_change(struct fdisk_context *cxt, const char *name)
> > +{
> > +	int i;
> > +
> > +	if (!cxt || !cxt->label || !name)
> > +		return FDISK_ERROR_UNKNOWN;
> > +
> > +	/* not really changing the label */
> > +	if (!strncmp(name, cxt->label->name, strlen(name)))
> > +		goto done;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(labels); i++) {
> > +		if (strncmp(name, labels[i]->name, strlen(name)))
> > +			continue;
> > +	
> > +		/* found the new label */
> > +		memset(cxt->label, 0, sizeof(struct fdisk_label));
> > +		memcpy(cxt->label, labels[i], sizeof(struct fdisk_label));
> > +		DBG(LABEL, dbgprint("changing to a %s label\n", labels[i]->name));
> > +		goto done;
> > +	}
> > +
> > +	/* couldn't find the requested label type */
> > +	return FDISK_ERROR_NOLABEL;
> > +done:
> > +	return 0;
> > +}
> > +
> >  static int __probe_labels(struct fdisk_context *cxt)
> >  {
> >  	int i, rc = 0;
> > -- 
> > 1.7.4.1
> > 
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe util-linux" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Petr
> 

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Petr Uzel]

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

On Tue, Jul 24, 2012 at 12:39:07PM +0200, Davidlohr Bueso wrote:
> > >> Shouldn't we use something else than hardcoded "sun"|"dos"|"sgi"|...
> > >> strings here? 
> > > 
> > > What's the problem with these strings? I think these are well
> > > recognized with clear meaning. What alternative do you propose?
> > 
> > Sorry, I should've proposed this already in my first mail:
> > 
> > -	fdisk_label_change(cxt, "sun");
> > +	fdisk_label_change(cxt, sun_label.name);
 
Ah, right. Deduplicating the strings makes sense, IMO.

> I'm planning on adding the disklabel type (numeric) to the context
> structure in a near future, so we'd then have two ways of identifying
> labels. I really do not see much difference between numbers and strings
> - this is not a performance critical program.

The concern here is not speed, but maintainability ;)

Best,

Petr

-- 
Petr Uzel
IRC: ptr_uzl @ freenode

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

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Bernhard Voelker]

On 07/24/2012 12:42 PM, Petr Uzel wrote:
> On Tue, Jul 24, 2012 at 12:39:07PM +0200, Davidlohr Bueso wrote:
>>>>> Shouldn't we use something else than hardcoded "sun"|"dos"|"sgi"|...
>>>>> strings here? 
>>>>
>>>> What's the problem with these strings? I think these are well
>>>> recognized with clear meaning. What alternative do you propose?
>>>
>>> Sorry, I should've proposed this already in my first mail:
>>>
>>> -	fdisk_label_change(cxt, "sun");
>>> +	fdisk_label_change(cxt, sun_label.name);
>  
> Ah, right. Deduplicating the strings makes sense, IMO.
> 
>> I'm planning on adding the disklabel type (numeric) to the context
>> structure in a near future, so we'd then have two ways of identifying
>> labels. I really do not see much difference between numbers and strings
>> - this is not a performance critical program.
> 
> The concern here is not speed, but maintainability ;)

Right ... and to save a bit size of the binary (well, not too much
in this case) ;-)

Have a nice day,
Berny

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Petr Uzel]

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

On Tue, Jul 24, 2012 at 12:41:50PM +0200, Davidlohr Bueso wrote:
> On Tue, 2012-07-24 at 11:52 +0200, Petr Uzel wrote:
> > On Sun, Jul 22, 2012 at 07:05:04PM +0200, Davidlohr Bueso wrote:
> > > From: Davidlohr Bueso <dave@gnu.org>
> > > 
> > > A new fdisk_label_change() function is added for situations when the disk label is
> > > changed (ie: creating a new sun label). This function only updates the label pointer
> > > in the context to use the newly specified label type.
> > 
> > Why can't we just call __probe_labels() after creating new label, e.g.
> > in soon-to-be introduced fdisk_label_create()?
> 
> Because this way we give users extra functionality in the API. There
> should be a way of changing label types on the fly, and since we don't
> export __probe_labels(), this is the other option.

What operation could change label type besides creating a new label?

> 
> Thanks,
> Davidlohr
> 
> > 
> > > 
> > > Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> > > ---
> > >  fdisks/fdisk.h         |    2 ++
> > >  fdisks/fdiskdoslabel.c |    1 +
> > >  fdisks/fdisksgilabel.c |    2 ++
> > >  fdisks/fdisksunlabel.c |    1 +
> > >  fdisks/utils.c         |   37 +++++++++++++++++++++++++++++++++++++
> > >  5 files changed, 43 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/fdisks/fdisk.h b/fdisks/fdisk.h
> > > index d716824..d7e85f5 100644
> > > --- a/fdisks/fdisk.h
> > > +++ b/fdisks/fdisk.h
> > > @@ -104,6 +104,7 @@ enum fdisk_error {
> > >  	FDISK_ERROR_WRITE,
> > >  	FDISK_ERROR_IOCTL,
> > >  	FDISK_ERROR_PROBE,
> > > +	FDISK_ERROR_NOLABEL,
> > >  	FDISK_ERROR_UNKNOWN
> > >  };
> > >  
> > > @@ -164,6 +165,7 @@ extern void fdisk_mbr_zeroize(struct fdisk_context *cxt);
> > >  extern void fdisk_geom_set_cyls(struct fdisk_context *cxt);
> > >  extern const char *fdisk_error_name(enum fdisk_error errcode);
> > >  extern void fdisk_error_fatal(struct fdisk_context *cxt, enum fdisk_error errcode);
> > > +extern int fdisk_label_change(struct fdisk_context *cxt, const char *name);
> > >  
> > >  /* prototypes for fdisk.c */
> > >  extern char *disk_device, *line_ptr;
> > > diff --git a/fdisks/fdiskdoslabel.c b/fdisks/fdiskdoslabel.c
> > > index 535afdc..9b9b23a 100644
> > > --- a/fdisks/fdiskdoslabel.c
> > > +++ b/fdisks/fdiskdoslabel.c
> > > @@ -230,6 +230,7 @@ void create_doslabel(struct fdisk_context *cxt)
> > >  
> > >  	dos_init(cxt);
> > >  	fdisk_mbr_zeroize(cxt);
> > > +	fdisk_label_change(cxt, "dos");
> > >  	set_all_unchanged();
> > >  	set_changed(0);
> > >  
> > > diff --git a/fdisks/fdisksgilabel.c b/fdisks/fdisksgilabel.c
> > > index e38d98f..6001038 100644
> > > --- a/fdisks/fdisksgilabel.c
> > > +++ b/fdisks/fdisksgilabel.c
> > > @@ -780,6 +780,8 @@ create_sgilabel(struct fdisk_context *cxt)
> > >  		}
> > >  
> > >  	fdisk_mbr_zeroize(cxt);
> > > +	fdisk_label_change(cxt, "sgi");
> > > +
> > >  	sgilabel->magic = SSWAP32(SGI_LABEL_MAGIC);
> > >  	sgilabel->boot_part = SSWAP16(0);
> > >  	sgilabel->swap_part = SSWAP16(1);
> > > diff --git a/fdisks/fdisksunlabel.c b/fdisks/fdisksunlabel.c
> > > index 4123806..b63335c 100644
> > > --- a/fdisks/fdisksunlabel.c
> > > +++ b/fdisks/fdisksunlabel.c
> > > @@ -161,6 +161,7 @@ void create_sunlabel(struct fdisk_context *cxt)
> > >  
> > >  	init();
> > >  	fdisk_mbr_zeroize(cxt);
> > > +	fdisk_label_change(cxt, "sun");
> > >  
> > >  	sunlabel->magic = SSWAP16(SUN_LABEL_MAGIC);
> > >  	sunlabel->sanity = SSWAP32(SUN_LABEL_SANE);
> > > diff --git a/fdisks/utils.c b/fdisks/utils.c
> > > index cf9484c..48dedfb 100644
> > > --- a/fdisks/utils.c
> > > +++ b/fdisks/utils.c
> > > @@ -44,6 +44,43 @@ static const struct fdisk_label *labels[] =
> > >  	&mac_label,
> > >  };
> > >  
> > > +/**
> > > + * fdisk_label_change:
> > > + * @cxt: fdisk context
> > > + * @name: new label name
> > > + *
> > > + * Updates the disk label type to the one specified by @name.
> > > + *
> > > + * Returns 0 on success, otherwise, a corresponding error.
> > > + */
> > > +int fdisk_label_change(struct fdisk_context *cxt, const char *name)
> > > +{
> > > +	int i;
> > > +
> > > +	if (!cxt || !cxt->label || !name)
> > > +		return FDISK_ERROR_UNKNOWN;
> > > +
> > > +	/* not really changing the label */
> > > +	if (!strncmp(name, cxt->label->name, strlen(name)))
> > > +		goto done;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(labels); i++) {
> > > +		if (strncmp(name, labels[i]->name, strlen(name)))
> > > +			continue;
> > > +	
> > > +		/* found the new label */
> > > +		memset(cxt->label, 0, sizeof(struct fdisk_label));
> > > +		memcpy(cxt->label, labels[i], sizeof(struct fdisk_label));
> > > +		DBG(LABEL, dbgprint("changing to a %s label\n", labels[i]->name));
> > > +		goto done;
> > > +	}
> > > +
> > > +	/* couldn't find the requested label type */
> > > +	return FDISK_ERROR_NOLABEL;
> > > +done:
> > > +	return 0;
> > > +}
> > > +
> > >  static int __probe_labels(struct fdisk_context *cxt)
> > >  {
> > >  	int i, rc = 0;
> > > -- 
> > > 1.7.4.1
> > > 
> > > 
> > > 
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe util-linux" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > Petr
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Petr

-- 
Petr Uzel
IRC: ptr_uzl @ freenode

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

Re: [PATCH 03/10] fdisk: API: add fdisk_label_change

[Karel Zak]

On Sun, Jul 22, 2012 at 07:05:04PM +0200, Davidlohr Bueso wrote:
>  5 files changed, 43 insertions(+), 0 deletions(-)

 Not applied, it seems unnecessary.

 I have temporary added fdisk_create_default_disklabel, but the final
 code (the current master) contains your fdisk_create_disklabel()
 where NULL name means a default disk label.

 I have renamed many functions :-) It seems better to use

   fdisk_create_disklabel()   than  fdisk_label_create()
   fdisk_add_partition()      than  fdisk_label_partition_new()

.. etc.

> +	/* not really changing the label */
> +	if (!strncmp(name, cxt->label->name, strlen(name)))
> +		goto done;

 BTW, I don't see a problem with strings -- very probably we will use
 strings in user interface, and add extra layer to translate strings
 to IDs seems like unnecessary optimization and complexity.

> +	for (i = 0; i < ARRAY_SIZE(labels); i++) {
> +		if (strncmp(name, labels[i]->name, strlen(name)))
> +			continue;
> +	
> +		/* found the new label */
> +		memset(cxt->label, 0, sizeof(struct fdisk_label));
> +		memcpy(cxt->label, labels[i], sizeof(struct fdisk_label));
> +		DBG(LABEL, dbgprint("changing to a %s label\n", labels[i]->name));
> +		goto done;
> +	}

 Oh no, let's use labels[] as constant array and cxt->label as
 const struct pointer. The list of functions does not have to
 be modifiable, and

    cxt->label = labels[i];

 is definitely better than memcpy().

 Note that it's will probably necessary to add

    void *labeldata;

 to fdisk_context for label private data. (For example to kill
 MBRbuffer_changed or another DOS specific global variables.)

    Karel

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