[PATCHES] s3-sysacls - modify SMB_ACL_PERMSET_T to be a mode_t (possible HPUX fix)

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES] s3-sysacls - modify SMB_ACL_PERMSET_T to be a mode_t (possible HPUX fix)

Samba - samba-technical mailing list
Hi,

This patch set fixes clang picky-developer errors on FreeBSD around
POSIX acls. It possibly also fixes an hpux issue reported in
https://bugzilla.samba.org/show_bug.cgi?id=11490#c4

I've solicited response on that bug in bugzilla - we need a bug report
and official verification in order to add a BUG: and backport, but even
without bug report, it fixes picky-developer on FreeBSD.

The first patch is the real fix (both to warning and possible bug), and
the rest are cleanup patches (I suppose this can be further cleaned up
by removing return code and error checking from APIs that cannot fail,
and possibly by removing SMB_ACL_PERMSET_T altogether and using mode_t.
There seems to be no end to cleanup...)

Review appreciated,
Uri.

sysacls-permset.patch.txt (35K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] s3-sysacls - modify SMB_ACL_PERMSET_T to be a mode_t (possible HPUX fix)

Samba - samba-technical mailing list
On 11/28/2017 08:08 AM, Arjit Gupta wrote:

> HI Uri,
>
> Thanks for providing the patch.
>
> We did below change and it is working fine.
>
> diff ./pidl/lib/Parse/Pidl/Typelist.pm.bak ./pidl/lib/Parse/Pidl/Typelist.pm
> 87c87,89
> <         "mode_t"        => "uint32",
> ---
>> # on HP-UX mode_t is of uint16_t, so changed uint32 to uint16
>> #       "mode_t"        => "uint32",
>>         "mode_t"        => "uint16",
>
> Is this change fine or should we port the patch provided by you.
>

This change certainly fixes things for your system, but it cannot be
upstreamed (at least not as-is) because that would break Linux. The fix
I'm proposing hopefully fixes things without IDL changes.

If you can open a bugzilla bug, that would be great, and if you can test
my fix (I can make a fix that builds on your version of Samba) that
would be even better. But as I said - you're good to go with your fix too.

Thanks,
Uri.

> Arjit Kumar
> 9650104435
>
> On Tue, Nov 28, 2017 at 11:09 AM, Uri Simchoni <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi,
>
>     This patch set fixes clang picky-developer errors on FreeBSD around
>     POSIX acls. It possibly also fixes an hpux issue reported in
>     https://bugzilla.samba.org/show_bug.cgi?id=11490#c4
>     <https://bugzilla.samba.org/show_bug.cgi?id=11490#c4>
>
>     I've solicited response on that bug in bugzilla - we need a bug report
>     and official verification in order to add a BUG: and backport, but even
>     without bug report, it fixes picky-developer on FreeBSD.
>
>     The first patch is the real fix (both to warning and possible bug), and
>     the rest are cleanup patches (I suppose this can be further cleaned up
>     by removing return code and error checking from APIs that cannot fail,
>     and possibly by removing SMB_ACL_PERMSET_T altogether and using mode_t.
>     There seems to be no end to cleanup...)
>
>     Review appreciated,
>     Uri.
>
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] s3-sysacls - modify SMB_ACL_PERMSET_T to be a mode_t (possible HPUX fix)

Samba - samba-technical mailing list
Hi Uri,

I will test your fix on hp-ux and let you know the same.
Please provide the patch for samba 4.5.

Arjit Kumar
9650104435

On Tue, Nov 28, 2017 at 11:50 AM, Uri Simchoni <[hidden email]> wrote:

> On 11/28/2017 08:08 AM, Arjit Gupta wrote:
> > HI Uri,
> >
> > Thanks for providing the patch.
> >
> > We did below change and it is working fine.
> >
> > diff ./pidl/lib/Parse/Pidl/Typelist.pm.bak ./pidl/lib/Parse/Pidl/
> Typelist.pm
> > 87c87,89
> > <         "mode_t"        => "uint32",
> > ---
> >> # on HP-UX mode_t is of uint16_t, so changed uint32 to uint16
> >> #       "mode_t"        => "uint32",
> >>         "mode_t"        => "uint16",
> >
> > Is this change fine or should we port the patch provided by you.
> >
>
> This change certainly fixes things for your system, but it cannot be
> upstreamed (at least not as-is) because that would break Linux. The fix
> I'm proposing hopefully fixes things without IDL changes.
>
> If you can open a bugzilla bug, that would be great, and if you can test
> my fix (I can make a fix that builds on your version of Samba) that
> would be even better. But as I said - you're good to go with your fix too.
>
> Thanks,
> Uri.
>
> > Arjit Kumar
> > 9650104435
> >
> > On Tue, Nov 28, 2017 at 11:09 AM, Uri Simchoni <[hidden email]
> > <mailto:[hidden email]>> wrote:
> >
> >     Hi,
> >
> >     This patch set fixes clang picky-developer errors on FreeBSD around
> >     POSIX acls. It possibly also fixes an hpux issue reported in
> >     https://bugzilla.samba.org/show_bug.cgi?id=11490#c4
> >     <https://bugzilla.samba.org/show_bug.cgi?id=11490#c4>
> >
> >     I've solicited response on that bug in bugzilla - we need a bug
> report
> >     and official verification in order to add a BUG: and backport, but
> even
> >     without bug report, it fixes picky-developer on FreeBSD.
> >
> >     The first patch is the real fix (both to warning and possible bug),
> and
> >     the rest are cleanup patches (I suppose this can be further cleaned
> up
> >     by removing return code and error checking from APIs that cannot
> fail,
> >     and possibly by removing SMB_ACL_PERMSET_T altogether and using
> mode_t.
> >     There seems to be no end to cleanup...)
> >
> >     Review appreciated,
> >     Uri.
> >
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] s3-sysacls - modify SMB_ACL_PERMSET_T to be a mode_t (possible HPUX fix)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Nov 28, 2017 at 07:39:43AM +0200, Uri Simchoni via samba-technical wrote:
> Hi,
>
> This patch set fixes clang picky-developer errors on FreeBSD around
> POSIX acls. It possibly also fixes an hpux issue reported in
> https://bugzilla.samba.org/show_bug.cgi?id=11490#c4

Would

--- a/source3/include/smb_acls.h
+++ b/source3/include/smb_acls.h
@@ -27,8 +27,8 @@ struct files_struct;
 struct smb_filename;

 typedef int                    SMB_ACL_TYPE_T;
-typedef mode_t                 *SMB_ACL_PERMSET_T;
-typedef mode_t                 SMB_ACL_PERM_T;
+typedef uint32_t                       *SMB_ACL_PERMSET_T;
+typedef uint32_t                       SMB_ACL_PERM_T;

 typedef enum smb_acl_tag_t SMB_ACL_TAG_T;
 typedef struct smb_acl_t *SMB_ACL_T;

possibly also solve the problem? It's not that I particularly like the
posix acl API, but it's there...

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] s3-sysacls - modify SMB_ACL_PERMSET_T to be a mode_t (possible HPUX fix)

Samba - samba-technical mailing list
On 11/28/2017 12:35 PM, Volker Lendecke via samba-technical wrote:

> On Tue, Nov 28, 2017 at 07:39:43AM +0200, Uri Simchoni via samba-technical wrote:
>> Hi,
>>
>> This patch set fixes clang picky-developer errors on FreeBSD around
>> POSIX acls. It possibly also fixes an hpux issue reported in
>> https://bugzilla.samba.org/show_bug.cgi?id=11490#c4
>
> Would
>
> --- a/source3/include/smb_acls.h
> +++ b/source3/include/smb_acls.h
> @@ -27,8 +27,8 @@ struct files_struct;
>  struct smb_filename;
>
>  typedef int                    SMB_ACL_TYPE_T;
> -typedef mode_t                 *SMB_ACL_PERMSET_T;
> -typedef mode_t                 SMB_ACL_PERM_T;
> +typedef uint32_t                       *SMB_ACL_PERMSET_T;
> +typedef uint32_t                       SMB_ACL_PERM_T;
>
>  typedef enum smb_acl_tag_t SMB_ACL_TAG_T;
>  typedef struct smb_acl_t *SMB_ACL_T;
>
> possibly also solve the problem? It's not that I particularly like the
> posix acl API, but it's there...
>
> Volker
>

:)

(tl;dr, bike-shedding alert)

It almost solves the problem (missing is a fix to make_simple_acl() in
source3/smbd/pysmbd.c, which uses the API in an incorrect way that
happens to work).

IMHO the changes I proposed make the interface simpler, harder to use
wrongly, and more idiomatic to C.

The existing interface offers two ways to look at SMB_ACL_PERMSET_T -
either as an opaque handle (a-la POSIX ACL API), or as a pointer to an
actual bitmask. Parts of the code already use the bitmask
interpretation. Parts of the code that use the "handle" interpretation
needlessly pass a pointer to the "handle".

The proposed change make it a bitmask-only, and it's clearer when one
should use SMB_ACL_PERMSET_T and when one should use a pointer to it.

The fact that the current interface resembles that of POSIX ACL API is
of little consequence IMO because actual interface with OS APIs is
hidden behind the walls of the VFS anyway.

If want to keep the interface the way it is, we can do that too.

Thanks,
Uri.