[ovs-dev] const attributes for some kernel ovs functions ?

[Luigi Rizzo]

On Wed, Jan 15, 2014 at 03:10:35PM -0800, Jesse Gross wrote:
> On Sat, Jan 11, 2014 at 4:33 PM, Luigi Rizzo <rizzo at iet.unipi.it> wrote:
> > Hi,
> > in porting the ovs kernel code to the FreeBSD kernel,
> > compiling with -Wcast-qual -Werror gives some
> >
> >    warning: cast discards qualifiers from pointer target type
> >
> > due to 'const' being ignored. Another couple of warnings
> > are on arguments to format strings.
> 
> What are the warnings from the format strings? (and I guess, more
> specifically, what is the definition of u64 if it is not unsigned long
> long?)

Oh i see. We used 'typedef uint64_t u64;' which seemed more appropriate
than unsigned long long (which can be larger than 64), but of course
what matters are established standards.
We can move to unsigned long long and close this
(though even the linux kernel has instances of all kinds):

	$ grep -r ' u64;' linux-3.11.0/ | grep typedef
	linux-3.11.0/arch/powerpc/platforms/cell/spufs/spu_save.c:typedef unsigned long long u64;
	linux-3.11.0/arch/powerpc/platforms/cell/spufs/spu_restore.c:typedef unsigned long long u64;
	linux-3.11.0/tools/lguest/lguest.c:typedef unsigned long long u64;
	linux-3.11.0/tools/virtio/linux/types.h:typedef uint64_t u64;
	linux-3.11.0/tools/testing/selftests/vm/hugetlbfstest.c:typedef unsigned long long u64;
	linux-3.11.0/tools/perf/util/types.h:typedef uint64_t      u64;
	linux-3.11.0/Documentation/ia64/err_inject.txt:typedef unsigned long u64;
	linux-3.11.0/include/linux/raid/pq.h:typedef uint64_t u64;
	linux-3.11.0/include/asm-generic/int-l64.h:typedef unsigned long u64;
	linux-3.11.0/include/asm-generic/int-ll64.h:typedef unsigned long long u64;

And compilers do warn about the difference:

	% cat a.c
	#include <stdio.h>
	#include <inttypes.h>
	typedef uint64_t u64;

	int f(int x) {
		u64 x_u64 = x;
		unsigned long long x_ull = x;

		printf("%llx %llx", x_u64, x_ull);
		return 0;
	}
	% cc -c -Wall a.c
	a.c: In function 'f':
	a.c:9: warning: format '%llx' expects type 'long long unsigned int', but argument 2 has type 'u64'
	luigi at luigi-bsd9:~/c-src % clang -c -Wall a.c
	a.c:9:22: warning: format specifies type 'unsigned long long' but the argument has type 'u64'
	      (aka 'unsigned long') [-Wformat]
		printf("%llx %llx", x_u64, x_ull);
			~~~~        ^~~~~
			%lx
	1 warning generated.

but of course it is a pain to use the PRIu64 format specifiers.

> > Attached is a trivial patch (partly from Daniele Di Proietto) that
> > addresses some of the cases; however, some of them could be
> > fixed in different ways.
> >
> > In particular, vport_priv() and vport_from_priv() should be
> > changed to return a const pointer, but this has a bit of
> > an avalanche effect on the code and I am not sure if you are
> > willing to go for it.
> 
> I'm not sure that it's even possible to change those functions to
> return a const pointer - it's private space so vport implementations
> are allowed to write into it.

i see. So the const qualifier in the argument to those functions
is ignored/overridden intentionally ?

> 
> > We are happy to submit a proper patch according to your
> > preferences.
> 
> There's a couple of different issues here so it might be helpful to
> break it down into a few separate patches with accompanying commit
> messages (and ideally in the context of Linux or generically since it
> is Linux-specific code after all). Also, do you have a patch against
> master? I'm not sure what branch this is against but it looks somewhat
> old.

absolutely. we will resubmit. At this point i was mostly trying to
figure out whether there was actually an issue or we did something wrong.

thanks
luigi