[Patch] fix IDL hang V2

[Jelmer Vernooij]

On Fri, Aug 22, 2014 at 10:54:29AM +0100, Noel Power wrote:
> Hi Jelmer,
> On 22/08/14 00:12, Jelmer Vernooij wrote:
> > On Thu, Aug 21, 2014 at 08:52:11PM +0100, Noel Power wrote:
> >> On 21/08/14 17:34, Jelmer Vernooij wrote:
> [...]
> >>> I'm wary of supporting constructions that are pidl-specific. We'd like to
> >>> make our IDL files more compatible with midl, not less.
> >> That's not at all clear from the current idl that seems littered with
> >> pidl specific extensions. The fact that those specific properties aid
> >> the the generation of standard marshall/unmarshall routines is imho a
> >> *good* thing and should be encouraged, hand coding marshall/unmarshall
> >> routines should be discouraged.
> > There are a couple of IDL files that have a lot of custom pidl
> > properties, but for the others it's fairly rare.
> didn't seem that way to me
> 
> find librpc/idl/ -name "*.idl" | wc -l
> 74
> find . -name "*.idl" -type f -print0 | xargs -0 grep public  | cut -f1
> -d: | sort -u | wc -l
> 47
> and for the pidl specifics mentioned in the pidl documentation (noprint,
> value, relative, subcontext, flag, nodiscriminant, charset)
> 17, 36, 10, 18, 48, 23, 45
> 
> ok, not the most accurate numbers but I would say at least indicative
> (and a quick manual look at inline arrays usages also is informative)
public, noprint, charset and value don't have an effect on the actual wire
representation. We can easily strip them out or ignore them (for e.g.
wireshark dissectors).

subcontext should be replaced with transfer_as in a lot of cases.
We've already got deprecation warnings in place for these. I agree it
would be good to have a --strict mode that warned about all properties
that are incompatible with midl.

That leaves nodiscriminant, relative and flag.

> > Most of the pidl extensions have equivalent attributes in midl. We'd
> > like to gradually move over from the pidl-specific attributes to the
> > midl specific ones, so we can more freely exchange IDL files.
> and that probably makes perfect sense for idl files that are
> wanted/needed/desirable to be exchanged, but it would be unfortunate to
> bunch all idl files into the same category, after my admittedly short
> exposure to pidl I really believe that it would be a shame to limit it's
> usefulness and functionality to being a midl clone. It's possible to
> have the best of both worlds.
There are better encodings than NDR in that case. What use do you have
for alignment or 8 byte booleans (pointers), etc if you're not trying
to be compatible with NDR?

> > If there's a struct member that is a pointer, then
> > can_contain_deferred should never check whether that member itself can
> > contain a deferred at all. How does a loop occur here?
> I don't believe that I said it did (although in fairness I did think it
> might), but really I just was trying to understand the point you were
> making with the psuedo code you had
Your patch was changing this function. If it doesn't have a loop in
it, then why does it need to be changed?

> >
> > FWIW I can compile the second IDL file without problems here, without
> > your patch:
> sure, so as can I. But....
> 
> interface hang
> {
>     typedef [public] struct {
>         uint32 count;
>         bar a[count];
>         hyper t;
> } foo ;
> 
>     typedef [public] struct {
>         foo b;
>     } bar;
> }
> 
> will also hang (and could be considered maybe a better mininal
> representation of the structure I originally has a problem with)
How many alignment bytes would you expect to use before "a" in this
case? None? Eight?

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140822/6c814c7a/attachment.pgp>