Re: CMake: Installation directory for include files in apr 1.7.x
Den ons 29 maj 2024 kl 23:34 skrev Ivan Zhakov : > What is the state of the 1.8.x branch? Would it make sense to rather do it >> here and focus on releasing 1.8 with this change? >> >> Unfortunately the release process is complicated in APR: trunk is 2.0 and > has breaking changes. 1.8.x is a branch from 1.7.x with many backports from > trunk, but as far as I know it misses many backports that merged to 1.7.x > branch. So in my opinion it is less stable than trunk. > Thanks for the explanation! Maybe off-topic but what are the major roadblocks for releasing APR 2.0? Kind regards, Daniel >
Re: CMake: Installation directory for include files in apr 1.7.x
Den tors 25 apr. 2024 kl 18:20 skrev Ivan Zhakov : > Hi, > > I am currently working on CMake and vcpkg [1]. > > One problem that I noticed is that APR 1.7.x (and APR-Util 1.6.x) > install include files to INSTALL_PREFIX/include. While on other > platforms include files installed to INSTALL_PREFIX/include/apr-1 (and > apr-2 for apr-trunk). > > This problem is fixed in apr-trunk: CMake based builds install include > files to INSTALL_PREFIX/include/apr-2. > > Installation to root include is not recommended by vcpkg. This can > also introduce problems when apr-1 and apr-2 are installed > side-by-side. > > I want to fix this in APR 1.7.x, but this could break users who rely > that APR is installed to INSTALL_PREFIX/include. > > I see several options: > 1. Just change to INSTALL_PREFIX/include/apr-1 > 2. Add a compile-time CMake option like APR_INSTALL_INCLUDE_TO_ROOT > and enable it by default for apr 1.7.x, but disable for vcpkg builds. > > For me it seems that (2) is a safer way, but maybe it's overkill? > I'm admittedly very new here but is this a reasonable change to make in a patch release? As a consumer of the library I would be quite surprised to see this happening in a patch release (but maybe I don't fully understand the implications). What is the state of the 1.8.x branch? Would it make sense to rather do it here and focus on releasing 1.8 with this change? Kind regards, Daniel Sahlberg (Subversion, Serf, TortoiseSVN) > [1] https://vcpkg.io/en/ > > -- > Ivan Zhakov >
Re: Get a user's all groups
Den tis 2 jan. 2024 kl 14:20 skrev Joe Orton : > On Tue, Jan 02, 2024 at 02:00:00PM +0100, Daniel Sahlberg wrote: > > My idea was to incrementally improve the code but maybe a better way > > is to switch to access() completely. access() seems to be widely > > available but I will have to read up on the setuid properties to make > > sure we don't change how things has worked in the past. > > > > Background: Subversion has the ability to say a file "needs locking", if > a > > particular user/working copy doesn't hold the "lock" the file should be > > read-only (and inversely if the user holds the lock the file should be > > writeable). We check for W access using the code above and then update > the > > permissions accordingly. > > Makes sense. Yeah, I would recommend switching to using access(,W_OK) or > access(,X_OK) on Unix, I'm not aware of any portability concerns with > that. > > Regards, Joe > > Thanks Joe! We ended up changing to access(). Kind regards, Daniel
Re: Get a user's all groups
Den tis 2 jan. 2024 kl 12:03 skrev Joe Orton :
> On Tue, Dec 26, 2023 at 07:17:51PM +0100, Daniel Sahlberg wrote:
> > Hi,
> >
> > apr_uid_current() can retur the user id and primary group id of a user.
> Is
> > there a way to find out if a user also has secondary groups (something
> > similar to getgrouplist(3)?
> >
> > The Subversion project has some bug reports where a user has R/W access
> to
> > a certain file via a secondary group, but APR doesn't pick up the
> secondary
> > groups and thus we don't think the user has R/W access. I'd like to
> improve
> > this by also considering all secondary groups.
>
> How are you testing for readability/writability here exactly? On Unix
> the right way is using access() but there isn't an APR wrapper for it.
> (Trying to manually check against user/groups is not a reliable way to
> test, not just because of groups but also because of things like setuid
> processes and RBAC systems which may exist on top of the user/groups.)
>
I also thought about RBACs (but I didn't know that was the term) and we are
checking exactly the wrong way:
[[[
SVN_ERR(svn_io_stat(_info, path, APR_FINFO_PROT | APR_FINFO_OWNER,
pool));
...
#if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
apr_err = apr_uid_current(, , pool);
if (apr_err)
return svn_error_wrap_apr(apr_err, _("Error getting UID of process"));
/* Check write bit for current user. */
if (apr_uid_compare(uid, file_info->user) == APR_SUCCESS)
*read_only = !(file_info->protection & APR_UWRITE);
else if (apr_gid_compare(gid, file_info->group) == APR_SUCCESS)
*read_only = !(file_info->protection & APR_GWRITE);
else
*read_only = !(file_info->protection & APR_WWRITE);
#endif
]]]
(And for the eagle eyed reader, the code above is copied from two different
functions, thus file_info is a struct in one case and a pointer-to-struct
in another, but I assume the principle should be clear enough).
The origin of the code is from ~2002 (checking for APR_*EXECUTE), copied to
a new function around 2011 (checking for APR_*WRITE).
My idea was to incrementally improve the code but maybe a better way is to
switch to access() completely. access() seems to be widely available but I
will have to read up on the setuid properties to make sure we don't change
how things has worked in the past.
Background: Subversion has the ability to say a file "needs locking", if a
particular user/working copy doesn't hold the "lock" the file should be
read-only (and inversely if the user holds the lock the file should be
writeable). We check for W access using the code above and then update the
permissions accordingly.
Kind regards,
Daniel
Get a user's all groups
Hi, apr_uid_current() can retur the user id and primary group id of a user. Is there a way to find out if a user also has secondary groups (something similar to getgrouplist(3)? The Subversion project has some bug reports where a user has R/W access to a certain file via a secondary group, but APR doesn't pick up the secondary groups and thus we don't think the user has R/W access. I'd like to improve this by also considering all secondary groups. Kind regards, Daniel Sahlberg
[PATCH] Remove links to mail-archives.apache.org from the website
Hi, The web page Mailing Lists [1] has links to mail-archives.apache.org. That site was decommissioned by ASF Infra a while back. The addresses still work but redirect to the new Ponymail archive (same as the first link). Please consider removing these links, suggested patch below Kind regards, Daniel Sahlberg [1] https://apr.apache.org/mailing-lists.html [[[ Index: xdocs/mailing-lists.xml === --- xdocs/mailing-lists.xml (revision 1914936) +++ xdocs/mailing-lists.xml (working copy) @@ -53,8 +53,6 @@ https://lists.apache.org/list.html?dev@apr.apache.org; >Apache Pony view -https://mail-archives.apache.org/mod_mbox/apr-dev/; - >Classic mod_mbox view https://www.mail-archive.com/dev%40apr.apache.org/; >Mail-Archive.Com @@ -97,8 +95,6 @@ https://lists.apache.org/list.html?b...@apr.apache.org; >Apache Pony view -https://mail-archives.apache.org/mod_mbox/apr-bugs/; - >Classic mod_mbox view @@ -139,8 +135,6 @@ https://lists.apache.org/list.html?comm...@apr.apache.org; >Apache Pony view -https://mail-archives.apache.org/mod_mbox/apr-commits/; - >Classic mod_mbox view https://www.mail-archive.com/commits%40apr.apache.org/; >Mail-Archive.Com ]]]
Re: getopt error message with a bare "-"
Den lör 2 dec. 2023 kl 11:08 skrev Graham Leggett : > On 28 Nov 2023, at 07:46, Daniel Sahlberg > wrote: > > Subversion is using APR's apr_getopt_long for command line processing. > > A user tried to supply "-" for the filename with the intention of using > stdin (Subversion has support for this in our code). However the user got > the following error message (with the first line coming from > apr_getopt_long and the second from Subversion's code): > > $ ./svnmucc put - file:///url/to/repository > svnmucc: invalid option: > Type 'svnmucc --help' for usage. > > We found the solution, to use "--" to disable option processing, so no > discussion around this is needed. > > In the thread, someone mentioned that it would be useful if the error > message could be improved so I looked into the APR code and have a > suggestion. > > The following code is responsible for the error message. In other places, > the error message contains the offending (long) option (third argument). > However since *p is always \0, checked just above, the error message will > just say "invalid option". I'm suggesting to change this to the fixed > string "-" instead. > [[[ > --- getopt.orig 2023-11-28 08:22:16.690508444 +0100 > +++ getopt.c2023-11-28 08:22:50.270297468 +0100 > @@ -272,7 +272,7 @@ > } > else > if (*p == '\0')/* Bare "-" is illegal > */ > -return serr(os, "invalid option", p, APR_BADCH); > +return serr(os, "invalid option", "-", APR_BADCH); > } > } > ]]] > > With this change, the error message will instead be: > $ ./svnmucc put - file:///url/to/repository > svnmucc: invalid option: - > Type 'svnmucc --help' for usage. > > with the "-" indicating which part of the command line is invalid. > > Original e-mail from our users@ list: > https://lists.apache.org/thread/mbsx7rxpx07sh6sfs6xhbnvgo2wbfl0k > > Follow-up e-mail in our dev@ list: > https://lists.apache.org/thread/l7x22yp6kb71qlv3rn8tfmcrc5hk73r4 > > > Waves hi :) > Oh, hi! :) (Graham is the user I mentioned above, didn't know you were here ... ) > > +1 on the above message fix at the very least. I’ll commit it if there are > no objections. > > Another solution would be to add a switch to getopt.c that allows for - to > be counted not as a "start of short option string" but as a separate > argument, possibly hidden behind a switch in the apr_getopt_t struct. For > our use, that would be even better, but I assume it would require more code > (also on our side to remain backwards compatible with older APR versions). > > > Looks like apr_getopt_t is the right place for a flag. It seems a lot more > intuitive to have “-“ recognised as an argument rather than an invalid > option. > That's exactly how I saw it as well. Not sure if we (Subversion) would even make use of that option for a long time, since we need to be reasonably sure an uptodate version of APR is available before we release. > > Regards, > Graham > — > Kind regards Daniel >
getopt error message with a bare "-"
Hi, Subversion is using APR's apr_getopt_long for command line processing. A user tried to supply "-" for the filename with the intention of using stdin (Subversion has support for this in our code). However the user got the following error message (with the first line coming from apr_getopt_long and the second from Subversion's code): $ ./svnmucc put - file:///url/to/repository svnmucc: invalid option: Type 'svnmucc --help' for usage. We found the solution, to use "--" to disable option processing, so no discussion around this is needed. In the thread, someone mentioned that it would be useful if the error message could be improved so I looked into the APR code and have a suggestion. The following code is responsible for the error message. In other places, the error message contains the offending (long) option (third argument). However since *p is always \0, checked just above, the error message will just say "invalid option". I'm suggesting to change this to the fixed string "-" instead. [[[ --- getopt.orig 2023-11-28 08:22:16.690508444 +0100 +++ getopt.c2023-11-28 08:22:50.270297468 +0100 @@ -272,7 +272,7 @@ } else if (*p == '\0')/* Bare "-" is illegal */ -return serr(os, "invalid option", p, APR_BADCH); +return serr(os, "invalid option", "-", APR_BADCH); } } ]]] With this change, the error message will instead be: $ ./svnmucc put - file:///url/to/repository svnmucc: invalid option: - Type 'svnmucc --help' for usage. with the "-" indicating which part of the command line is invalid. Original e-mail from our users@ list: https://lists.apache.org/thread/mbsx7rxpx07sh6sfs6xhbnvgo2wbfl0k Follow-up e-mail in our dev@ list: https://lists.apache.org/thread/l7x22yp6kb71qlv3rn8tfmcrc5hk73r4 Another solution would be to add a switch to getopt.c that allows for - to be counted not as a "start of short option string" but as a separate argument, possibly hidden behind a switch in the apr_getopt_t struct. For our use, that would be even better, but I assume it would require more code (also on our side to remain backwards compatible with older APR versions). Kind regards, Daniel Sahlberg
svn:ignore a few more paths to help TortoiseSVN's build system
Hi, TortoiseSVN is importing APR and APR-util via an svn:externals definition: [[[ https://svn.apache.org/repos/asf/apr/apr/tags/1.6.5 apr https://svn.apache.org/repos/asf/apr/apr-util/tags/1.6.1 apr-util ]]] The build system creates a few new directories within the apr and apr-util directories for temporary build files. These clutter svn status and makes it more difficult to see if there are any real changes. I would propose to add a few more folders in svn:ignore. (I have already done similar modifications in the Subversion repository, see r1891003). [[[ Index: apr === --- apr (revision 1891700) +++ apr (working copy) Property changes on: apr ___ Modified: svn:ignore ## -14,8 +14,16 ## LibNTR Debug DebugNT +debug_win32 +debug_win32_static +debug_x64 +debug_x64_static Release ReleaseNT +release_win32 +release_win32_static +release_x64 +release_x64_static *.ncb *.opt *.plg Index: apr-util === --- apr-util(revision 1891700) +++ apr-util(working copy) Property changes on: apr-util ___ Modified: svn:ignore ## -17,7 +17,15 ## apu-*-config apu-config.out Debug +debug_win32 +debug_win32_static +debug_x64 +debug_x64_static Release +release_win32 +release_win32_static +release_x64 +release_x64_static LibD LibR aprutil.opt ]]] An alternative approach would be to ignore debug_* and release_*, this might help in case there is a future architecture (eg arm). (I realise this will be made on /trunk and not affect the tags that TortoiseSVN is pulling in until there is a new release, but it will be better in the future). Kind regards, Daniel Sahlberg
