On Mon, 2012-03-12 at 12:52 -0700, Boaz Harrosh wrote:
> On 03/11/2012 10:37 AM, Myklebust, Trond wrote:
> > Hi Boaz,
> >
> > When I run 'sparse' on the object layout driver, I get the following
> > errors:
> >
> > fs/nfs/objlayout/objio_osd.c:210:37: error: bad constant expression
> > fs/nfs/objlayout/objio_osd.c:211:39: error: bad constant expression
> > fs/nfs/objlayout/objio_osd.c:315:59: error: bad constant expression
> >
>
> Then sparse is broken
>
> > which boils down to the following section of code:
> >
> > int __alloc_objio_seg(unsigned numdevs, gfp_t gfp_flags,
> > struct objio_segment **pseg)
> > {
> > struct __alloc_objio_segment {
> > struct objio_segment olseg;
> > struct ore_dev *ods[numdevs];
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > struct ore_comp comps[numdevs];
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > } *aolseg;
> >
> > aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
> >
> >
> > Dynamically allocated arrays such as the above are a GNU C compiler
> > feature that we cannot use in kernel code. If the above array sizes need
> > to be dynamically determined, then please use the kcalloc() interface to
> > allocate them.
>
> I am using kzalloc to do a single allocation. The *ods[numdevs] is only
> used for the sizeof() expression it does not allocate anything actually.
>
> We use gcc extensions all over the place. From the simple __func__ to
> the famous typeof().
__func__ is actually valid C99.
> I don't understand why you decide to pick on this one in particular.
It has been shown to be troublesome previously. See for instance
http://lkml.org/lkml/2011/10/23/25
Then there is the issue that alternative compilers such as clang barf
all over it.
> > I.e. something like
> >
> > struct __alloc_objio_segment {
> > struct objio_segment olseg;
> > struct ore_dev **ods;
> > struct ore_comp *comps;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > } *aolseg;
> >
> > aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
> > if (aolseg) {
> > aolseg->ods = kcalloc(numdevs, sizeof(struct ore_dev *), gfp_flags);
> > aolseg->ore_comp = kcalloc(numdevs, sizeof(struct ore_comp), gfp_flags);
>
> This is not the same code. It is three allocations instead of one. And it is an
> extra pointer reference instead of just pointer offsetting.
>
> If it would change it would change to:
>
> aolseg = kzalloc(sizeof(*aolseg) +
> numdevs * sizeof(struct ore_dev *),
> numdevs * sizeof(struct ore_comp),
> gfp_flags);
That is broken, since it doesn't take into account alignment issues
within the structure. I also don't see how you would express the above
as a single structure without using variable length arrays.
> And all the pointer arithmetic that follow. But why the very ugly code
> when there is an elegant, type-safe, and self documenting way. It's
> beautifully clear this way what is the in memory structure of the
> allocation.
While it may look beautiful, it has been proven to be immature and to
generate ugly code.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
