Re: svn commit: r1895508 - in /apr/apr/trunk: dso/win32/ file_io/win32/ locks/win32/ misc/unix/ misc/win32/ network_io/unix/ network_io/win32/ passwd/ shmem/win32/ threadproc/win32/ time/win32/ user/w
On Thu, Jan 20, 2022 at 12:49 PM Ivan Zhakov wrote: > > On Thu, 20 Jan 2022 at 00:43, William A Rowe Jr wrote: >> >> On Wed, Jan 19, 2022 at 11:31 AM Yann Ylavic wrote: >> > >> > I'm a bit surprised that _WIN32_WCE used CreateThread() while more >> > recent ones use _beginthreadex(), shouldn't it be the opposite? >> > CreateThread() aligns more with the usual/modern naming convention on >> > Windows.. >> >> Ignore naming conventions. We are a library that sits on top of >> MSVCRT/LIBCMT, and >> so the stdc lib we use needs to know a thread has started. >> CreateThread doesn't let >> the clib particpate and this causes specific problems for .dll loaded >> modules like our lib. > > That's true. > > But Microsoft finally fixed this in Universal CRT ( Visual Studio 2015). In > UCRT beginthreadex() is almost a wrapper around CreateThread(). So maybe it > makes sense to require VS 2015 for APR trunk and just use CreateThread? This is my docker build env... you won't find any argument from me; https://github.com/envoyproxy/envoy-build-tools/blob/main/build_container/build_container_windows.ps1
Re: svn commit: r1895508 - in /apr/apr/trunk: dso/win32/ file_io/win32/ locks/win32/ misc/unix/ misc/win32/ network_io/unix/ network_io/win32/ passwd/ shmem/win32/ threadproc/win32/ time/win32/ user/w
On Thu, 20 Jan 2022 at 00:43, William A Rowe Jr wrote: > On Wed, Jan 19, 2022 at 11:31 AM Yann Ylavic wrote: > > > > I'm a bit surprised that _WIN32_WCE used CreateThread() while more > > recent ones use _beginthreadex(), shouldn't it be the opposite? > > CreateThread() aligns more with the usual/modern naming convention on > Windows.. > > Ignore naming conventions. We are a library that sits on top of > MSVCRT/LIBCMT, and > so the stdc lib we use needs to know a thread has started. > CreateThread doesn't let > the clib particpate and this causes specific problems for .dll loaded > modules like our lib. > That's true. But Microsoft finally fixed this in Universal CRT ( Visual Studio 2015). In UCRT beginthreadex() is almost a wrapper around CreateThread(). So maybe it makes sense to require VS 2015 for APR trunk and just use CreateThread? -- Ivan Zhakov
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 12:32 PM William A Rowe Jr wrote: > > On Thu, Jan 20, 2022 at 9:44 AM Yann Ylavic wrote: > > > > My bad, re-reading the thread the proposal was a compat apu_ -> apr_ > > layer in APR *util* 1.7.x, yet since apr-util 1.7.0 is released > > already it would be the same kind of compat issue. > > No worries. The larger question is about whether to do this during > 1.7.0 -> 1.7.1? > Or 1.7.0 -> 1.8.0, in anticipation of 2.0.0. And the actual implications can totally live on 1.8.x branch, that's why I forked it. Thanks for your efforts!
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 9:44 AM Yann Ylavic wrote: > > > My bad, re-reading the thread the proposal was a compat apu_ -> apr_ > layer in APR *util* 1.7.x, yet since apr-util 1.7.0 is released > already it would be the same kind of compat issue. No worries. The larger question is about whether to do this during 1.7.0 -> 1.7.1? Or 1.7.0 -> 1.8.0, in anticipation of 2.0.0.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
On Thu, 20 Jan 2022 at 17:39, Ivan Zhakov wrote: > On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov wrote: > >> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem wrote: >> >>> >>> >>> On 1/13/22 7:04 PM, Ivan Zhakov wrote: >>> > [[ sorry for delayed response ]] >>> > >>> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic wrote: >>> >> >>> >> Hi Ivan, >>> >> >>> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov >>> wrote: >>> >>> >>> >>> This change does not compile on Windows in APR 1.7.x: >>> >>> [[[ >>> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared >>> identifier >>> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' >>> must >>> >>> point to struct/union >>> >> >>> >> I was missing backport of r1895178, does r1896808 compile now? >>> >> (Sorry, no Windows at hand..). >>> > Yes, it builds now. Thanks! >>> > >>> >> >>> >>> >>> >>> I also have a high-level objection against backporting this change to >>> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only >>> >>> regression fixes should be backported to the stable branch. r1896510 >>> >>> is a significant change and as far as I understand it's not a >>> >>> regression fix. So I think it would be better to revert r1896510 and >>> >>> release it as part of APR 2.0 (or 1.8.x). >>> >> >>> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are >>> >> fixes for bugs that were there before 1.7 already, not regressions >>> >> introduced by 1.7.0. >>> > >>> > Agreed on the bugfix/regressions part. I have misunderstood that >>> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that >>> > it adds new functionality. But even with that in mind, I still think >>> > that the size of the change might be just too large for it to be an >>> > appropriate fit for a patch release. >>> > >>> > Speaking of the change itself, I think that there might be an >>> > alternative to making the apr_file_t also handle sockets on Windows. >>> > It might be better to specifically change the pollset implementation >>> > so that on Windows it would add a socket and use it for wakeup, >>> > instead of using the socket disguised as a file. >>> > >>> > If this alternative approach sounds fine, I could try to implement it. >>> >>> But this could wait for a 1.7.2, correct? I am asking because there is >>> some desire to get 1.7.1 out of the door soon. >>> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and >>> is released soon after 1.7.2. >>> >>> 1. Revert this change from 1.7.x >> 2. Release 1.7.1 >> 3. Rework this code on trunk without changing the apr_file_t's behavior >> > This part is now in the following branch: > > https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation > > Sorry, wrong branch URL. The correct URL is: https://svn.apache.org/repos/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation -- Ivan Zhakov
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 4:37 PM Yann Ylavic wrote: > > On Thu, Jan 20, 2022 at 3:56 PM William A Rowe Jr wrote: > > > > On Thu, Jan 20, 2022 at 8:50 AM Eric Covener wrote: > > > > > > > Code that will compile on 1.7.1 release > > > > still won't compile on 1.7.0, unless I misunderstand something. > > > > > > Is it a requirement in this direction? > > > https://apr.apache.org/versioning.html says "later versions" > > > > That's why I raise the question, I don't have the authoritative answer. > > > > Our SVN participants who adopted and recommended strongly that > > APR follow the semantic versioning [1] approach so they could -also- > > adopt it probably feel more strongly about this. > > It was proposed in another thread to add the APU macros to 1.7.x to > ease later migration to 2.x, I don't think that adding these apr_pool_ > macros can have more compat issues.. My bad, re-reading the thread the proposal was a compat apu_ -> apr_ layer in APR *util* 1.7.x, yet since apr-util 1.7.0 is released already it would be the same kind of compat issue. > > > Cheers; > Yann.
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 3:56 PM William A Rowe Jr wrote: > > On Thu, Jan 20, 2022 at 8:50 AM Eric Covener wrote: > > > > > Code that will compile on 1.7.1 release > > > still won't compile on 1.7.0, unless I misunderstand something. > > > > Is it a requirement in this direction? > > https://apr.apache.org/versioning.html says "later versions" > > That's why I raise the question, I don't have the authoritative answer. > > Our SVN participants who adopted and recommended strongly that > APR follow the semantic versioning [1] approach so they could -also- > adopt it probably feel more strongly about this. It was proposed in another thread to add the APU macros to 1.7.x to ease later migration to 2.x, I don't think that adding these apr_pool_ macros can have more compat issues.. Cheers; Yann.
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
On Thu, Jan 20, 2022 at 8:50 AM Eric Covener wrote: > > > Code that will compile on 1.7.1 release > > still won't compile on 1.7.0, unless I misunderstand something. > > Is it a requirement in this direction? > https://apr.apache.org/versioning.html says "later versions" That's why I raise the question, I don't have the authoritative answer. Our SVN participants who adopted and recommended strongly that APR follow the semantic versioning [1] approach so they could -also- adopt it probably feel more strongly about this. Bill. [1] https://semver.org/
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
> Code that will compile on 1.7.1 release > still won't compile on 1.7.0, unless I misunderstand something. Is it a requirement in this direction? https://apr.apache.org/versioning.html says "later versions"
Re: svn commit: r1897209 - in /apr/apr/branches/1.7.x: ./ include/apr_pools.h memory/unix/apr_pools.c
I agree this ticks the ABI compatibility box.
I doesn't check the API compatibility box, IMO. Code that will compile
on 1.7.1 release
still won't compile on 1.7.0, unless I misunderstand something.
I'd really like us to start on the 1.7 releases today, but I'd have to
vote against this
candidate.
What are others' opinions?
On Wed, Jan 19, 2022 at 10:53 AM wrote:
>
> Author: ylavic
> Date: Wed Jan 19 16:53:54 2022
> New Revision: 1897209
>
> URL: http://svn.apache.org/viewvc?rev=1897209=rev
> Log:
> apr_pools: Follow up to r1863234: APR_POOL_DEBUG macros for compat.
>
> To preserve ABI, define apr_pool_{join,find,num_bytes,lock}() as no-op
> macros in 1.7.x.
>
>
> Modified:
> apr/apr/branches/1.7.x/ (props changed)
> apr/apr/branches/1.7.x/include/apr_pools.h
> apr/apr/branches/1.7.x/memory/unix/apr_pools.c
>
> Propchange: apr/apr/branches/1.7.x/
> --
> Reverse-merged /apr/apr/trunk:r1863217
>
> Modified: apr/apr/branches/1.7.x/include/apr_pools.h
> URL:
> http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/include/apr_pools.h?rev=1897209=1897208=1897209=diff
> ==
> --- apr/apr/branches/1.7.x/include/apr_pools.h (original)
> +++ apr/apr/branches/1.7.x/include/apr_pools.h Wed Jan 19 16:53:54 2022
> @@ -754,12 +754,14 @@ APR_DECLARE(void) apr_pool_cleanup_for_e
> * In this case the caller must call apr_pool_join() to indicate this
> * guarantee to the APR_POOL_DEBUG code.
> *
> - * These functions have an empty implementation if APR is compiled
> - * with #APR_POOL_DEBUG not set.
> + * These functions are implemented when #APR_POOL_DEBUG is set and
> + * defined as no-op macros otherwise.
> *
> * @{
> */
>
> +#if APR_POOL_DEBUG || defined(DOXYGEN)
> +
> /**
> * Guarantee that a subpool has the same lifetime as the parent.
> * @param p The parent pool
> @@ -791,7 +793,21 @@ APR_DECLARE(apr_size_t) apr_pool_num_byt
> */
> APR_DECLARE(void) apr_pool_lock(apr_pool_t *pool, int flag);
>
> -/** @} */
> +#else /* APR_POOL_DEBUG or DOXYGEN */
> +
> +#undef apr_pool_join
> +#define apr_pool_join(a,b)
> +
> +#undef apr_pool_find
> +#define apr_pool_find(mem) (NULL)
> +
> +#undef apr_pool_num_bytes
> +#define apr_pool_num_bytes(p, rec) ((apr_size_t)0)
> +
> +#undef apr_pool_lock
> +#define apr_pool_lock(pool, lock)
> +
> +#endif /* APR_POOL_DEBUG or DOXYGEN */
>
> /** @} */
>
>
> Modified: apr/apr/branches/1.7.x/memory/unix/apr_pools.c
> URL:
> http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/memory/unix/apr_pools.c?rev=1897209=1897208=1897209=diff
> ==
> --- apr/apr/branches/1.7.x/memory/unix/apr_pools.c (original)
> +++ apr/apr/branches/1.7.x/memory/unix/apr_pools.c Wed Jan 19 16:53:54 2022
> @@ -2929,29 +2929,6 @@ APR_DECLARE(apr_status_t) apr_pool_creat
> return apr_pool_create_unmanaged_ex(newpool, abort_fn, allocator);
> }
>
> -/*
> - * Other stubs, for people who are running
> - * mixed release/debug enviroments.
> - */
> -
> -APR_DECLARE(void) apr_pool_join(apr_pool_t *p, apr_pool_t *sub)
> -{
> -}
> -
> -APR_DECLARE(apr_pool_t *) apr_pool_find(const void *mem)
> -{
> -return NULL;
> -}
> -
> -APR_DECLARE(apr_size_t) apr_pool_num_bytes(apr_pool_t *pool, int recurse)
> -{
> -return 0;
> -}
> -
> -APR_DECLARE(void) apr_pool_lock(apr_pool_t *pool, int flag)
> -{
> -}
> -
> #else /* APR_POOL_DEBUG */
>
> #undef apr_palloc
>
>
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov wrote: > On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem wrote: > >> >> >> On 1/13/22 7:04 PM, Ivan Zhakov wrote: >> > [[ sorry for delayed response ]] >> > >> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic wrote: >> >> >> >> Hi Ivan, >> >> >> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov wrote: >> >>> >> >>> This change does not compile on Windows in APR 1.7.x: >> >>> [[[ >> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared >> identifier >> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must >> >>> point to struct/union >> >> >> >> I was missing backport of r1895178, does r1896808 compile now? >> >> (Sorry, no Windows at hand..). >> > Yes, it builds now. Thanks! >> > >> >> >> >>> >> >>> I also have a high-level objection against backporting this change to >> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only >> >>> regression fixes should be backported to the stable branch. r1896510 >> >>> is a significant change and as far as I understand it's not a >> >>> regression fix. So I think it would be better to revert r1896510 and >> >>> release it as part of APR 2.0 (or 1.8.x). >> >> >> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are >> >> fixes for bugs that were there before 1.7 already, not regressions >> >> introduced by 1.7.0. >> > >> > Agreed on the bugfix/regressions part. I have misunderstood that >> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that >> > it adds new functionality. But even with that in mind, I still think >> > that the size of the change might be just too large for it to be an >> > appropriate fit for a patch release. >> > >> > Speaking of the change itself, I think that there might be an >> > alternative to making the apr_file_t also handle sockets on Windows. >> > It might be better to specifically change the pollset implementation >> > so that on Windows it would add a socket and use it for wakeup, >> > instead of using the socket disguised as a file. >> > >> > If this alternative approach sounds fine, I could try to implement it. >> >> But this could wait for a 1.7.2, correct? I am asking because there is >> some desire to get 1.7.1 out of the door soon. >> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is >> released soon after 1.7.2. >> >> 1. Revert this change from 1.7.x > 2. Release 1.7.1 > 3. Rework this code on trunk without changing the apr_file_t's behavior > This part is now in the following branch: https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation What do you think? It would be great if someone could take a look on the implementation from the *nix perspective. After that, I propose to merge the branch into trunk. (I will be travelling/on vacation for a couple of weeks, so might not be able to answer promptly.) -- Ivan Zhakov
Re: svn commit: r1897208 - in /apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation: file_io/win32/ include/arch/unix/ include/arch/win32/ poll/unix/
On Thu, 20 Jan 2022 at 01:00, Yann Ylavic wrote:
> On Wed, Jan 19, 2022 at 5:42 PM wrote:
> >
> > Author: ivan
> > Date: Wed Jan 19 16:41:59 2022
> > New Revision: 1897208
> >
> > URL: http://svn.apache.org/viewvc?rev=1897208=rev
> > Log:
> > On 'win32-pollset-wakeup-no-file-socket-emulation' branch:
> >
> > Windows: For the pollset wakeup, use apr_socket_t directly instead of
> using a
> > socket disguised as an apr_file_t.
> []
> >
> > -apr_status_t apr_file_socket_pipe_create(apr_file_t **in,
> > - apr_file_t **out,
> > +apr_status_t apr_file_socket_pipe_create(apr_socket_t **in,
> > + apr_socket_t **out,
> > apr_pool_t *p)
> > {
> > apr_status_t rv;
> > SOCKET rd;
> > SOCKET wr;
> >
> > +*in = NULL;
> > +*out = NULL;
> > +
> > if ((rv = create_socket_pipe(, )) != APR_SUCCESS) {
> > return rv;
> > }
> > -(*in) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
> > -(*in)->pool = p;
> > -(*in)->fname = NULL;
> > -(*in)->ftype = APR_FILETYPE_SOCKET;
> > -(*in)->timeout = 0; /* read end of the pipe is non-blocking */
> > -(*in)->ungetchar = -1;
> > -(*in)->eof_hit = 0;
> > -(*in)->filePtr = 0;
> > -(*in)->bufpos = 0;
> > -(*in)->dataRead = 0;
> > -(*in)->direction = 0;
> > -(*in)->pOverlapped = NULL;
> > -(*in)->filehand = (HANDLE)rd;
> > -
> > -(*out) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
> > -(*out)->pool = p;
> > -(*out)->fname = NULL;
> > -(*out)->ftype = APR_FILETYPE_SOCKET;
> > -(*out)->timeout = -1;
> > -(*out)->ungetchar = -1;
> > -(*out)->eof_hit = 0;
> > -(*out)->filePtr = 0;
> > -(*out)->bufpos = 0;
> > -(*out)->dataRead = 0;
> > -(*out)->direction = 0;
> > -(*out)->pOverlapped = NULL;
> > -(*out)->filehand = (HANDLE)wr;
> > +apr_os_sock_put(in, , p);
> > +apr_os_sock_put(out, , p);
>
> I think that the *in apr_socket and the underlying socket descriptor
> are not aligned w.r.t. non-blocking.
> We probably need to call apr_socket_timeout_set(*in, 0) here to make
> that happen, thus remove the last ioctlsocket() in
> create_socket_pipe() that sets the descriptor non-blocking before
> leaving (which would be useless now).
>
> Something like the below on top of your branch?
>
> Index: file_io/win32/pipe.c
> ===
> --- file_io/win32/pipe.c(revision 1897212)
> +++ file_io/win32/pipe.c(working copy)
> @@ -381,14 +381,7 @@ static apr_status_t create_socket_pipe(SOCKET *rd,
> goto cleanup;
> }
> if (nrd == (int)sizeof(uid) && memcmp(iid, uid, sizeof(uid)) ==
> 0) {
> -/* Got the right identifier, put the poll()able read side of
> - * the pipe in nonblocking mode and return.
> - */
> -bm = 1;
> -if (ioctlsocket(*rd, FIONBIO, ) == SOCKET_ERROR) {
> -rv = apr_get_netos_error();
> -goto cleanup;
> -}
> +/* Got the right identifier, return. */
> break;
> }
> closesocket(*rd);
> @@ -438,6 +431,9 @@ apr_status_t apr_file_socket_pipe_create(apr_socke
> apr_os_sock_put(in, , p);
> apr_os_sock_put(out, , p);
>
> +/* read end of the pipe is non-blocking */
> +apr_socket_timeout_set(*in, 0);
> +
> apr_pool_cleanup_register(p, (void *)(*in), socket_pipe_cleanup,
>apr_pool_cleanup_null);
> apr_pool_cleanup_register(p, (void *)(*out), socket_pipe_cleanup,
> --
>
>
Makes sense to me. Committed in r1897245 .
Thanks!
--
Ivan Zhakov
