[PATCH] Have the smbcli_session record the OS and Native LANMAN of the remote server

[Amin Azez]

* Gerald Carter wrote, On 29/04/09 17:32:
>> There's more than one established convention widely
>> present in the code base.
>
> Agreed.  But this should not really be a question of
> personal style.  If it matches README.Coding and
> prog_guide4.txt you should be safe.  If you follow
> that and the patch is refused due to style reasons,
> it is the project's fault and not yours.

I'm not picking fault here, or trying to argue; just pointing out how it
looks from my position.

The documents you mention encourage the style I used, and do not
discourage it, apart from tab-8 spacing which no-one follows.

The two documents you mention make recommendations that are not followed
even in new code.

In short, they can't be trusted, it seems that they misled me and are
somewhat ignored by everyone else.



prog_guide4.txt

I've read prog_guide4.txt numerous times before before and re-read it today.

It's still only "solicit[ing] comments from a few" and the only
reference to the aspects of style we were discussing is a preference for
another document elsewhere: Documentation/CodingStyle in the linux kernel.



Documentation/CodingStyle

Recommends 8-space tabs, but samba uses 4 spaces.

Encourages use of braces-less if statements, e.g.

|        if (buffer == NULL)
|                return -ENOMEM;

We regularly break the recommendation about not returning from a
function via a macro. (822 times by my count)

Against the advice of this document, we have loads of macros that depend
on local variables having certain names.

To my mind both of this "infractions" are fine, and are probably what
README.Coding refers to as "coding style should never outweigh coding
itself"



README.Coding

It refers to Documentation/CodingStyle again and K&R guidelines (where
ever they are?) and humourously suggests an automatic re-formatter,
later disparaged by prog_guide4.txt

It is also out of date, It suggests 8 space tabs to indent, but 4 spaces
are used nearly everywhere.

Finally, it explicitly encourages the style I was using:

(reformatted)
|When invoking functions that return pointer values, either of the
|following are acceptable.  Use you best judgement[1] and choose the
|more readable option.
|Remember that many other people will review it.::
|
|        if ((x = malloc(sizeof(short)*10)) == NULL ) {
|                fprintf(stderr, "Unable to alloc memory!\n");
|        }
|
|or::
|
|        x = malloc(sizeof(short)*10);
|        if (!x) {
|                fprintf(stderr, "Unable to alloc memory!\n");
|        }



So I've broken the "secret" Samba team coding style, by following the
recommendations of the published coding style documents, which even says
(README.Coding):

	"However, coding style should never outweigh coding itself..."

[1] In my judgement I used the more readable option; however I
acknowledge that many other people review it, but compare the attached
51 line patch (48 lines to copy 6 strings!) and see which is more
readable - I mean readable-for-intent, not human-as-a-compiler.

Maybe a third way with this macro? We're certainly not afraid of macros
in samba:

/* Return true if src is NULL or can be talloc_strdup'd to dest
   Return false if src is not NULL and can't be copied to dest */
#define talloc_strdup_ok(mem_ctx, dest, src) \
	((dest)=((src)?(talloc_strdup((mem_ctx), (src))):NULL),\
	 ((src)?(dest!=NULL):1==1))

Then we can have:

if (! talloc_strdup_ok(session, session->os, state->setup.old.out.os))
	c->status = NT_STATUS_NO_MEMORY;



NOT-Signed-off: Sam Liddicott <sam at liddicott.com>
---
 source4/libcli/raw/libcliraw.h           |    3 ++
 source4/libcli/smb_composite/sesssetup.c |   48
++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 0 deletions(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 13d07625de2a1b52fa44f09d7e511fd4ecefbe2e.diff
Type: text/x-patch
Size: 2357 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090430/aec5af63/13d07625de2a1b52fa44f09d7e511fd4ecefbe2e.bin