Discussion:
[patch] fix SANE_Device
(too old to reply)
Frank Zago
2004-11-13 19:28:16 UTC
Permalink
This patch removes the const from the name strings in SANE_Device.
I didn't commit it because it might breaks stuff. However, I believe the new
definition is actually correct. On my tree, the number of warnings drop from 137
to 94. It also introduces a few more warnings, but these can be fixed too.
If no one objects, I will commit it in a few days.

Frank.
Henning Meier-Geinitz
2004-11-13 20:09:37 UTC
Permalink
Hi,
Post by Frank Zago
This patch removes the const from the name strings in SANE_Device.
I didn't commit it because it might breaks stuff.
Be careful. SANE-Device is part of the SANE standard, so that would
need to be changed also.
Post by Frank Zago
However, I believe the new definition is actually correct.
I guess the idea was that the strings shouldn't be changed after the
SANE-Device pointer is returned to the frontend.

As far as I can see the trouble results from doing things like
sane.name = strdup (...) and freeing those in sane_exit().

Bye,
Henning
Frank Zago
2004-11-13 20:26:17 UTC
Permalink
Post by Henning Meier-Geinitz
Hi,
Post by Frank Zago
This patch removes the const from the name strings in SANE_Device.
I didn't commit it because it might breaks stuff.
Be careful. SANE-Device is part of the SANE standard, so that would
need to be changed also.
Post by Frank Zago
However, I believe the new definition is actually correct.
I guess the idea was that the strings shouldn't be changed after the
SANE-Device pointer is returned to the frontend.
Thsi behaviour should be made clear in the doc. To me, const means something else.
Since this patch makes the variable from const to non-const, I don't think it
will break anything.
Post by Henning Meier-Geinitz
As far as I can see the trouble results from doing things like
sane.name = strdup (...) and freeing those in sane_exit().
That is correct. I think it's clear from the use of that field that the const
creates more trouble than it solves.
Oliver Rauch
2004-11-13 21:34:01 UTC
Permalink
Post by Frank Zago
Post by Henning Meier-Geinitz
I guess the idea was that the strings shouldn't be changed after the
SANE-Device pointer is returned to the frontend.
Thsi behaviour should be made clear in the doc. To me, const means something else.
Since this patch makes the variable from const to non-const, I don't think it
will break anything.
Post by Henning Meier-Geinitz
As far as I can see the trouble results from doing things like
sane.name = strdup (...) and freeing those in sane_exit().
That is correct. I think it's clear from the use of that field that the const
creates more trouble than it solves.
Hello Henning, hello Frank,

I think the const should not be changed because of two reasons:

1) It makes sense to define it as const in the standard because
the frontend should not change anything in the struct that
is returned by the backend
2) It is part of the sane standard and it makes a lot of problems
to change the standard definitions

May be we need a second definition without the const in some cases,
but the struct that is returned by the backend should contain const
strings.

So when it is necessary then the routine that needs any non const
strings has to create its own struct definition.

May be this second (non const) definition can be put in a helper file
(sanei_???.h).

Oliver
Frank Zago
2004-11-13 22:36:48 UTC
Permalink
Post by Oliver Rauch
Post by Frank Zago
Post by Henning Meier-Geinitz
I guess the idea was that the strings shouldn't be changed after the
SANE-Device pointer is returned to the frontend.
Thsi behaviour should be made clear in the doc. To me, const means something else.
Since this patch makes the variable from const to non-const, I don't think it
will break anything.
Post by Henning Meier-Geinitz
As far as I can see the trouble results from doing things like
sane.name = strdup (...) and freeing those in sane_exit().
That is correct. I think it's clear from the use of that field that the const
creates more trouble than it solves.
Hello Henning, hello Frank,
1) It makes sense to define it as const in the standard because
the frontend should not change anything in the struct that
is returned by the backend
2) It is part of the sane standard and it makes a lot of problems
to change the standard definitions
May be we need a second definition without the const in some cases,
but the struct that is returned by the backend should contain const
strings.
So when it is necessary then the routine that needs any non const
strings has to create its own struct definition.
May be this second (non const) definition can be put in a helper file
(sanei_???.h).
Oliver
So what about having an external definition for frontends, and one internal for
backends?
Oliver Rauch
2004-11-13 23:14:10 UTC
Permalink
Post by Frank Zago
Post by Oliver Rauch
Post by Frank Zago
Post by Henning Meier-Geinitz
I guess the idea was that the strings shouldn't be changed after the
SANE-Device pointer is returned to the frontend.
Thsi behaviour should be made clear in the doc. To me, const means something else.
Since this patch makes the variable from const to non-const, I don't think it
will break anything.
Post by Henning Meier-Geinitz
As far as I can see the trouble results from doing things like
sane.name = strdup (...) and freeing those in sane_exit().
That is correct. I think it's clear from the use of that field that the const
creates more trouble than it solves.
Hello Henning, hello Frank,
1) It makes sense to define it as const in the standard because
the frontend should not change anything in the struct that
is returned by the backend
2) It is part of the sane standard and it makes a lot of problems
to change the standard definitions
May be we need a second definition without the const in some cases,
but the struct that is returned by the backend should contain const
strings.
So when it is necessary then the routine that needs any non const
strings has to create its own struct definition.
May be this second (non const) definition can be put in a helper file
(sanei_???.h).
Oliver
So what about having an external definition for frontends, and one internal for
backends?
Please can you explain what error/warning the const produces on your
system?

I think it should be possible to change the pointers, call free(...)
etc. The const definition should make sure that it is not possible to
change the contents of the strings (pointer to const char, not const
pointer to char).

So I think there is no need to add anything in the backend. It would be
enough when the backend creates a non const string and makes the pointer
in the struct point to the non const string - may be with a typecast,
but I think the typecast is not necessary.

Oliver
Henning Meier-Geinitz
2004-11-14 12:17:04 UTC
Permalink
Hi,
Post by Oliver Rauch
Please can you explain what error/warning the const produces on your
system?
e.g.:
abaton.c: In function sane_abaton_exit':
abaton.c:902: warning: cast discards qualifiers from pointer target type
abaton.c:903: warning: cast discards qualifiers from pointer target type

That happens in other backends, too.

The code is:
free ((void *) dev->sane.name);
free ((void *) dev->sane.model);
Post by Oliver Rauch
So I think there is no need to add anything in the backend. It would be
enough when the backend creates a non const string and makes the pointer
in the struct point to the non const string - may be with a typecast,
but I think the typecast is not necessary.
That's the work-around used in my backends to avoid the warnings. It
works, but I don't think it's really clean.
A cast doesn't seem to be necessary.

Bye,
Henning
Oliver Rauch
2004-11-14 15:29:05 UTC
Permalink
Post by Henning Meier-Geinitz
Hi,
Post by Oliver Rauch
Please can you explain what error/warning the const produces on your
system?
abaton.c:902: warning: cast discards qualifiers from pointer target type
abaton.c:903: warning: cast discards qualifiers from pointer target type
That happens in other backends, too.
free ((void *) dev->sane.name);
free ((void *) dev->sane.model);
Post by Oliver Rauch
So I think there is no need to add anything in the backend. It would be
enough when the backend creates a non const string and makes the pointer
in the struct point to the non const string - may be with a typecast,
but I think the typecast is not necessary.
That's the work-around used in my backends to avoid the warnings. It
works, but I don't think it's really clean.
A cast doesn't seem to be necessary.
Bye,
Henning
The SANE standard defines the comunication between frontend and backend.
It does not define any structs that shall be internally used by the
frontend or backend.

I think the clean appoach is that the backend handles the texts internal
as non const strings and only creats the SANE_Device struct to return
the data to the frontend. The SANE_Device struct is defined for the
purpose to transfer the data from the backend to the frontend as const
chars. When the backend has to use non const definitions then it is not
allowed to use the SANE_Device struct so it has to use it´s own data
types and not the one of the SANE standard.

Oliver
Frank Zago
2004-11-14 19:03:03 UTC
Permalink
Post by Oliver Rauch
The SANE standard defines the comunication between frontend and backend.
It does not define any structs that shall be internally used by the
frontend or backend.
I think the clean appoach is that the backend handles the texts internal
as non const strings and only creats the SANE_Device struct to return
the data to the frontend. The SANE_Device struct is defined for the
purpose to transfer the data from the backend to the frontend as const
chars. When the backend has to use non const definitions then it is not
allowed to use the SANE_Device struct so it has to use itÂŽs own data
types and not the one of the SANE standard.
Oliver
So, in that case what about the patch in attachment.

I've also changed the internal definition of SANE_Option_Descriptor for the same
reason. It breaks about 5 backends, but they can all be easily fixed by hand
by removing a few const. I'd say that the current definition of SANE_Device and
SANE_Option_Descriptor and created an ugly cascade of unnecessary casts and
consts in all the backends.

Frank.
Frank Zago
2004-11-17 05:04:16 UTC
Permalink
Post by Frank Zago
Post by Oliver Rauch
The SANE standard defines the comunication between frontend and backend.
It does not define any structs that shall be internally used by the
frontend or backend.
I think the clean appoach is that the backend handles the texts internal
as non const strings and only creats the SANE_Device struct to return
the data to the frontend. The SANE_Device struct is defined for the
purpose to transfer the data from the backend to the frontend as const
chars. When the backend has to use non const definitions then it is not
allowed to use the SANE_Device struct so it has to use it´s own data
types and not the one of the SANE standard.
Oliver
So, in that case what about the patch in attachment.
I've also changed the internal definition of SANE_Option_Descriptor for
the same reason. It breaks about 5 backends, but they can all be easily
fixed by hand by removing a few const. I'd say that the current
definition of SANE_Device and SANE_Option_Descriptor and created an ugly
cascade of unnecessary casts and consts in all the backends.
Frank.
------------------------------------------------------------------------
Index: include/sane/sane.h
===================================================================
RCS file: /cvsroot/sane/sane-backends/include/sane/sane.h,v
retrieving revision 1.6
diff -u -3 -p -B -b -u -r1.6 sane.h
--- include/sane/sane.h 13 Nov 2004 20:29:14 -0000 1.6
+++ include/sane/sane.h 14 Nov 2004 18:59:03 -0000
@@ -92,6 +92,16 @@ typedef enum
}
SANE_Unit;
+#ifdef BACKEND_NAME
+typedef struct
+ {
+ SANE_String name; /* unique device name */
+ SANE_String vendor; /* device vendor string */
+ SANE_String model; /* device model name */
+ SANE_String type; /* device type (e.g., "flatbed scanner") */
+ }
+SANE_Device;
+#else
typedef struct
{
SANE_String_Const name; /* unique device name */
@@ -100,6 +110,7 @@ typedef struct
SANE_String_Const type; /* device type (e.g., "flatbed scanner") */
}
SANE_Device;
+#endif
#define SANE_CAP_SOFT_SELECT (1 << 0)
#define SANE_CAP_HARD_SELECT (1 << 1)
@@ -134,6 +145,28 @@ typedef struct
}
SANE_Range;
+#ifdef BACKEND_NAME
+typedef struct
+ {
+ SANE_String name; /* name of this option (command-line name) */
+ SANE_String title; /* title of this option (single-line) */
+ SANE_String desc; /* description of this option (multi-line) */
+ SANE_Value_Type type; /* how are values interpreted? */
+ SANE_Unit unit; /* what is the (physical) unit? */
+ SANE_Int size;
+ SANE_Int cap; /* capabilities */
+
+ SANE_Constraint_Type constraint_type;
+ union
+ {
+ SANE_String *string_list; /* NULL-terminated list */
+ SANE_Word *word_list; /* first element is list-length */
+ SANE_Range *range;
+ }
+ constraint;
+ }
+SANE_Option_Descriptor;
+#else
typedef struct
{
SANE_String_Const name; /* name of this option (command-line name) */
@@ -154,6 +187,7 @@ typedef struct
constraint;
}
SANE_Option_Descriptor;
+#endif
typedef enum
{
SO? Is anyone for or against this patch?
Mattias Ellert
2004-11-17 07:47:58 UTC
Permalink
Post by Frank Zago
SO? Is anyone for or against this patch?
This patch introduces SANE backend internal definitions in the public
sane.h header. Backend internal definitions should be in the sanei_*.h
headers.

If you really want different definitions of the structs internally in
the backends these should be in a sanei_*.h header not in the public header.

Mattias
Frank Zago
2004-11-17 15:15:15 UTC
Permalink
Post by Mattias Ellert
Post by Frank Zago
SO? Is anyone for or against this patch?
This patch introduces SANE backend internal definitions in the public
sane.h header. Backend internal definitions should be in the sanei_*.h
headers.
If you really want different definitions of the structs internally in
the backends these should be in a sanei_*.h header not in the public header.
Alas, the backends are including the public header, so that's not possible.
Oliver Rauch
2004-11-17 18:53:48 UTC
Permalink
Post by Frank Zago
Post by Frank Zago
Post by Oliver Rauch
The SANE standard defines the comunication between frontend and backend.
It does not define any structs that shall be internally used by the
frontend or backend.
I think the clean appoach is that the backend handles the texts internal
as non const strings and only creats the SANE_Device struct to return
the data to the frontend. The SANE_Device struct is defined for the
purpose to transfer the data from the backend to the frontend as const
chars. When the backend has to use non const definitions then it is not
allowed to use the SANE_Device struct so it has to use it´s own data
types and not the one of the SANE standard.
Oliver
So, in that case what about the patch in attachment.
I've also changed the internal definition of SANE_Option_Descriptor for
the same reason. It breaks about 5 backends, but they can all be easily
fixed by hand by removing a few const. I'd say that the current
definition of SANE_Device and SANE_Option_Descriptor and created an ugly
cascade of unnecessary casts and consts in all the backends.
Frank.
------------------------------------------------------------------------
Index: include/sane/sane.h
===================================================================
RCS file: /cvsroot/sane/sane-backends/include/sane/sane.h,v
retrieving revision 1.6
diff -u -3 -p -B -b -u -r1.6 sane.h
--- include/sane/sane.h 13 Nov 2004 20:29:14 -0000 1.6
+++ include/sane/sane.h 14 Nov 2004 18:59:03 -0000
@@ -92,6 +92,16 @@ typedef enum
}
SANE_Unit;
+#ifdef BACKEND_NAME
+typedef struct
+ {
+ SANE_String name; /* unique device name */
+ SANE_String vendor; /* device vendor string */
+ SANE_String model; /* device model name */
+ SANE_String type; /* device type (e.g., "flatbed scanner") */
+ }
+SANE_Device;
+#else
typedef struct
{
SANE_String_Const name; /* unique device name */
@@ -100,6 +110,7 @@ typedef struct
SANE_String_Const type; /* device type (e.g., "flatbed scanner") */
}
SANE_Device;
+#endif
#define SANE_CAP_SOFT_SELECT (1 << 0)
#define SANE_CAP_HARD_SELECT (1 << 1)
@@ -134,6 +145,28 @@ typedef struct
}
SANE_Range;
+#ifdef BACKEND_NAME
+typedef struct
+ {
+ SANE_String name; /* name of this option (command-line name) */
+ SANE_String title; /* title of this option (single-line) */
+ SANE_String desc; /* description of this option (multi-line) */
+ SANE_Value_Type type; /* how are values interpreted? */
+ SANE_Unit unit; /* what is the (physical) unit? */
+ SANE_Int size;
+ SANE_Int cap; /* capabilities */
+
+ SANE_Constraint_Type constraint_type;
+ union
+ {
+ SANE_String *string_list; /* NULL-terminated list */
+ SANE_Word *word_list; /* first element is list-length */
+ SANE_Range *range;
+ }
+ constraint;
+ }
+SANE_Option_Descriptor;
+#else
typedef struct
{
SANE_String_Const name; /* name of this option (command-line name) */
@@ -154,6 +187,7 @@ typedef struct
constraint;
}
SANE_Option_Descriptor;
+#endif
typedef enum
{
SO? Is anyone for or against this patch?
Hello Frank.

I am not a fan of this patch.
I think it will work but I think it is a hack.

It seems that only a few backends have problems with this.
Why not fix it in a proper way:

create the strings as non const and then set the
sane_device structur to point to these strings?!

But when I am the only one who is against this patch
then I will be quiet and it will be ok for me. It is nothing
that will steal my sleep :)


Oliver
f***@austin.rr.com
2004-11-17 19:58:54 UTC
Permalink
Post by Oliver Rauch
Hello Frank.
I am not a fan of this patch.
I think it will work but I think it is a hack.
It seems that only a few backends have problems with this.
A few backend are creating the name of the manufacturer from the scsi inquiry.
A lot of backends are creating their mode list from the device.
This patch will allow us to get rid of about 150 (IMO legitimate) warnings. The current code is not clean.
The non hack solution is to have two headers: one for the symbols exported and one for internal use. Same as what is done with the linux kernel structure s.
Post by Oliver Rauch
create the strings as non const and then set the
sane_device structur to point to these strings?!
This will not work. That why there is so much warnings.
Post by Oliver Rauch
But when I am the only one who is against this patch
then I will be quiet and it will be ok for me. It is nothing
that will steal my sleep :)
Your input is appreciated. Thanks.

Frank.
Oliver Rauch
2004-11-17 21:23:36 UTC
Permalink
Post by f***@austin.rr.com
Post by Oliver Rauch
Hello Frank.
I am not a fan of this patch.
I think it will work but I think it is a hack.
It seems that only a few backends have problems with this.
A few backend are creating the name of the manufacturer from the scsi inquiry.
A lot of backends are creating their mode list from the device.
This patch will allow us to get rid of about 150 (IMO legitimate) warnings. The current code is not clean.
The non hack solution is to have two headers: one for the symbols exported and one for internal use. Same as what is done with the linux kernel structure s.
Post by Oliver Rauch
create the strings as non const and then set the
sane_device structur to point to these strings?!
This will not work. That why there is so much warnings.
IMO thats not ture.
It is allowed to do this:

char *hello;
const char *hello_const;

hello="ABC";

hello_const = hello;



and this produces a warning:

*hello_const = 'A';

what is correct becaus we use a pointer to a const char to change the
char so it is not const any more.

It is not a const pointer to a char, it is a pointer to a const char.
And it is allowed to make a pointer of type pointer to const char point
to a (non const) char.

I attach a little test c-code for this. Compile with

gcc consttest.c -o consttest -Wall

(gcc version 3.2.2 20030222)

Oliver
Post by f***@austin.rr.com
Post by Oliver Rauch
But when I am the only one who is against this patch
then I will be quiet and it will be ok for me. It is nothing
that will steal my sleep :)
Your input is appreciated. Thanks.
Frank.
Frank Zago
2004-11-18 05:09:42 UTC
Permalink
Post by Oliver Rauch
Post by f***@austin.rr.com
Post by Oliver Rauch
Hello Frank.
I am not a fan of this patch.
I think it will work but I think it is a hack.
It seems that only a few backends have problems with this.
A few backend are creating the name of the manufacturer from the scsi inquiry.
A lot of backends are creating their mode list from the device.
This patch will allow us to get rid of about 150 (IMO legitimate) warnings. The current code is not clean.
The non hack solution is to have two headers: one for the symbols exported and one for internal use. Same as what is done with the linux kernel structure s.
Post by Oliver Rauch
create the strings as non const and then set the
sane_device structur to point to these strings?!
This will not work. That why there is so much warnings.
IMO thats not ture.
char *hello;
const char *hello_const;
hello="ABC";
hello_const = hello;
*hello_const = 'A';
what is correct becaus we use a pointer to a const char to change the
char so it is not const any more.
It is not a const pointer to a char, it is a pointer to a const char.
And it is allowed to make a pointer of type pointer to const char point
to a (non const) char.
I attach a little test c-code for this. Compile with
gcc consttest.c -o consttest -Wall
(gcc version 3.2.2 20030222)
Oliver
Post by f***@austin.rr.com
Post by Oliver Rauch
But when I am the only one who is against this patch
then I will be quiet and it will be ok for me. It is nothing
that will steal my sleep :)
Your input is appreciated. Thanks.
Frank.
------------------------------------------------------------------------
#include "stdio.h"
#include "stdlib.h"
int main()
{
const char *hello_const;
char *hello;
printf("defining hello=\"Hello\"\n");
printf("defining hello_const = hello\n");
hello = "Hello";
hello_const = hello;
printf("hello = %s\n", hello);
printf("hello_const = %s\n", hello_const);
printf("\n");
printf("defining hello = (char *) malloc(10)\n");
printf("defining *hello = \'A\'\n");
printf("defining *(hello+1) = \'B\'\n");
printf("defining hello_const = hello\n");
hello= (char *) malloc(10);
hello_const = hello;
*hello='A';
*(hello+1)='B';
hello[2]=0;
printf("hello = %s\n", hello);
printf("hello_const = %s\n", hello_const);
printf("\n");
printf("unallowed defining *hello_const = \'X\'\n");
*hello_const='X';
printf("hello = %s\n", hello);
printf("hello_const = %s\n", hello_const);
return 0;
}
Your example is missing the one thing I'd like to see fixed. How do you free
your memory allocated?
free(hello_const) will generate a warning.
free((void *)hello_const) will also generate a warning.

So do we fix:

dev->sane.name = strdup (devname);
...
free(dev->sane.name);


with:

unconst_name = strdup (devname);
dev->sane.name = unconst_name;
..
dev->sane.name = NULL;
free(unconst_name);


Or we fix dev->sane.name's definition

Or we do nothing and live with 150 warnings (for both SANE_Device and
SANE_Descriptor)?

Frank.
Oliver Rauch
2004-11-18 13:18:14 UTC
Permalink
Post by Frank Zago
Post by Oliver Rauch
Post by f***@austin.rr.com
Post by Oliver Rauch
Hello Frank.
I am not a fan of this patch.
I think it will work but I think it is a hack.
It seems that only a few backends have problems with this.
A few backend are creating the name of the manufacturer from the scsi inquiry.
A lot of backends are creating their mode list from the device.
This patch will allow us to get rid of about 150 (IMO legitimate) warnings. The current code is not clean.
The non hack solution is to have two headers: one for the symbols exported and one for internal use. Same as what is done with the linux kernel structure s.
Post by Oliver Rauch
create the strings as non const and then set the
sane_device structur to point to these strings?!
This will not work. That why there is so much warnings.
IMO thats not ture.
char *hello;
const char *hello_const;
hello="ABC";
hello_const = hello;
*hello_const = 'A';
what is correct becaus we use a pointer to a const char to change the
char so it is not const any more.
It is not a const pointer to a char, it is a pointer to a const char.
And it is allowed to make a pointer of type pointer to const char point
to a (non const) char.
I attach a little test c-code for this. Compile with
gcc consttest.c -o consttest -Wall
(gcc version 3.2.2 20030222)
Oliver
Post by f***@austin.rr.com
Post by Oliver Rauch
But when I am the only one who is against this patch
then I will be quiet and it will be ok for me. It is nothing
that will steal my sleep :)
Your input is appreciated. Thanks.
Frank.
------------------------------------------------------------------------
#include "stdio.h"
#include "stdlib.h"
int main()
{
const char *hello_const;
char *hello;
printf("defining hello=\"Hello\"\n");
printf("defining hello_const = hello\n");
hello = "Hello";
hello_const = hello;
printf("hello = %s\n", hello);
printf("hello_const = %s\n", hello_const);
printf("\n");
printf("defining hello = (char *) malloc(10)\n");
printf("defining *hello = \'A\'\n");
printf("defining *(hello+1) = \'B\'\n");
printf("defining hello_const = hello\n");
hello= (char *) malloc(10);
hello_const = hello;
*hello='A';
*(hello+1)='B';
hello[2]=0;
printf("hello = %s\n", hello);
printf("hello_const = %s\n", hello_const);
printf("\n");
printf("unallowed defining *hello_const = \'X\'\n");
*hello_const='X';
printf("hello = %s\n", hello);
printf("hello_const = %s\n", hello_const);
return 0;
}
Your example is missing the one thing I'd like to see fixed. How do you free
your memory allocated?
free(hello_const) will generate a warning.
free((void *)hello_const) will also generate a warning.
dev->sane.name = strdup (devname);
...
free(dev->sane.name);
unconst_name = strdup (devname);
dev->sane.name = unconst_name;
..
dev->sane.name = NULL;
free(unconst_name);
Thats what I prefer.

We only have to keep the original pointer
and everything is ok.

Oliver

Continue reading on narkive:
Loading...