[pulseaudio-discuss] [patches] constification round #4 (pa_mainloop_api)

[j***@gmail.com]

constification round #4 (pa_mainloop_api)

26 patches in total. I attached a zipped copy also.

The first 19 avoid (client) API change; the final 7 do not and will
have to wait until ready and happy to do an API bump.

I'm not certain if there's a public API for 3rd-party modules, but if
so then I can anticipate issue being taken with respect to
pa_iochannel_get_mainloop_api() in #14. Unpicking that though would not
be so easy :/

It's possible for patches #21, #23 and #24 to be bumped down before the
API change of #20, but as things are they are blocked by it and it
would require a temporary hack... #23 and #24 (utils) depend upon
pa_signal_init(), done in #21 which involves changing the underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static api ref
to the destroy callback which requires non-const). Getting around this
would require injecting a temporary hack into pa_signal_free() giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

[Tanu Kaskinen]

Post by j***@gmail.com
constification round #4 (pa_mainloop_api)
26 patches in total. I attached a zipped copy also.
The first 19 avoid (client) API change; the final 7 do not and will
have to wait until ready and happy to do an API bump.
I'm not certain if there's a public API for 3rd-party modules, but if
so then I can anticipate issue being taken with respect to
pa_iochannel_get_mainloop_api() in #14. Unpicking that though would not
be so easy :/

There's no public API for modules.

Post by j***@gmail.com
It's possible for patches #21, #23 and #24 to be bumped down before the
API change of #20, but as things are they are blocked by it and it
would require a temporary hack... #23 and #24 (utils) depend upon
pa_signal_init(), done in #21 which involves changing the underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static api ref
to the destroy callback which requires non-const). Getting around this
would require injecting a temporary hack into pa_signal_free() giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

We can't change the signatures of publicly declared callback types.
That will cause incompatibilities (compiler warnings at least, which is
bad enough) in applications. Therefore the only patch that I could
apply is the first one.

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com
constification round #4 (pa_mainloop_api)
26 patches in total. I attached a zipped copy also.
The first 19 avoid (client) API change; the final 7 do not and will
have to wait until ready and happy to do an API bump.
I'm not certain if there's a public API for 3rd-party modules, but if
so then I can anticipate issue being taken with respect to
pa_iochannel_get_mainloop_api() in #14. Unpicking that though would not
be so easy :/

There's no public API for modules.

Ok

Post by Tanu Kaskinen

Post by j***@gmail.com
It's possible for patches #21, #23 and #24 to be bumped down before the
API change of #20, but as things are they are blocked by it and it
would require a temporary hack... #23 and #24 (utils) depend upon
pa_signal_init(), done in #21 which involves changing the
underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static api ref
to the destroy callback which requires non-const). Getting around this
would require injecting a temporary hack into pa_signal_free() giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

We can't change the signatures of publicly declared callback types.
That will cause incompatibilities (compiler warnings at least, which is
bad enough) in applications.

Sure

Post by Tanu Kaskinen
Therefore the only patch that I could
apply is the first one.

But the types aren't changed until #19/26 ...

[Tanu Kaskinen]

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com
It's possible for patches #21, #23 and #24 to be bumped down before the
API change of #20, but as things are they are blocked by it and it
would require a temporary hack... #23 and #24 (utils) depend upon
pa_signal_init(), done in #21 which involves changing the
underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static api ref
to the destroy callback which requires non-const). Getting around this
would require injecting a temporary hack into pa_signal_free() giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

We can't change the signatures of publicly declared callback types.
That will cause incompatibilities (compiler warnings at least, which is
bad enough) in applications.

Sure

Post by Tanu Kaskinen
Therefore the only patch that I could
apply is the first one.

But the types aren't changed until #19/26 ...

Patch 4 changes the callback signatures.

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com
It's possible for patches #21, #23 and #24 to be bumped down
before
the
API change of #20, but as things are they are blocked by it and it
would require a temporary hack... #23 and #24 (utils) depend upon
pa_signal_init(), done in #21 which involves changing the underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static
api
ref
to the destroy callback which requires non-const). Getting
around
this
would require injecting a temporary hack into pa_signal_free() giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

We can't change the signatures of publicly declared callback types.
That will cause incompatibilities (compiler warnings at least,
which
is
bad enough) in applications.

Sure

Post by Tanu Kaskinen
Therefore the only patch that I could
apply is the first one.

But the types aren't changed until #19/26 ...

Patch 4 changes the callback signatures.

Right, but only in the mainloop api vtable, which user's only read
values from, so I wasn't expecting that to be an issue.

I've tested with gcc with `-Wall`, hacking the change into my locally
installed headers and recompiling a test program which utilises
`time_new`. No warnings resulted.

[Tanu Kaskinen]

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com
It's possible for patches #21, #23 and #24 to be bumped down
before
the
API change of #20, but as things are they are blocked by it and it
would require a temporary hack... #23 and #24 (utils) depend upon
pa_signal_init(), done in #21 which involves changing the underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static
api
ref
to the destroy callback which requires non-const). Getting
around
this
would require injecting a temporary hack into pa_signal_free() giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

We can't change the signatures of publicly declared callback types.
That will cause incompatibilities (compiler warnings at least,
which
is
bad enough) in applications.

Sure

Post by Tanu Kaskinen
Therefore the only patch that I could
apply is the first one.

But the types aren't changed until #19/26 ...

Patch 4 changes the callback signatures.

Right, but only in the mainloop api vtable, which user's only read
values from, so I wasn't expecting that to be an issue.

Applications can also write to it, if they implement their own
mainloops.

When I wrote my previous mail, I thought that patch 4 also changes the
event callback types (pa_io_event_cb_t etc.), and that was what I was
primarily concerned about (but applications implementing their own
mainloops is still a real problem). I can't now find the patch that
changes the callback types, but there are many patches that change the
callback implementation signatures, and if there's no corresponding
change to the callback types, that causes warnings (for example, try
applying patch 2 alone, and you'll get warnings when building
pulseaudio).

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com
It's possible for patches #21, #23 and #24 to be bumped down
before
the
API change of #20, but as things are they are blocked by it
and
it
would require a temporary hack... #23 and #24 (utils)
depend
upon
pa_signal_init(), done in #21 which involves changing the underlying
static; this depends upon changing pa_signal_destroy_cb_t (#20),
because of the way pa_signal_free() works (passing that static
api
ref
to the destroy callback which requires non-const). Getting
around
this
would require injecting a temporary hack into
pa_signal_free()
giving
alternate means of obtaining a non-const api pointer - i.e. api
function pointer comparison would be necessary to tell us which
mainloop is in use, to then get what we need using the correct
*_get_api() with api->userdata.

We can't change the signatures of publicly declared callback types.
That will cause incompatibilities (compiler warnings at least,
which
is
bad enough) in applications.

Sure

Post by Tanu Kaskinen
Therefore the only patch that I could
apply is the first one.

But the types aren't changed until #19/26 ...

Patch 4 changes the callback signatures.

Right, but only in the mainloop api vtable, which user's only read
values from, so I wasn't expecting that to be an issue.

Applications can also write to it, if they implement their own
mainloops.

Ah, that use case was not mentioned in our previous discussion. I'd
asked if the api struct was immutable, or whether there was any
intention to allow users to modify it, to say hijack one or more
function implementations (i.e. a partial customised mainloop), and
you'd said that there was no such intention. That discussion left me
with the understanding that custom/partial-custom mainloops weren't a
thing.

Do you happen to know of any examples of such programs implementing
custom mainloops that create and pass around a mainloop-api vtable
populated with their own pointers?

Post by Tanu Kaskinen
When I wrote my previous mail, I thought that patch 4 also changes
the event callback types (pa_io_event_cb_t etc.), and that was what I
was primarily concerned about

Yeah I deliberately left change to those callback signatures until a
later commit (#25 of 26 I believe), for precisely those same concerns.

Post by Tanu Kaskinen
(but applications implementing their own mainloops is still a real
problem).

Right... :/
Well, I guess no matter how rare custom mainloops might be, if custom
mainloops are a legit supported feature, then this change breaks it and
thus must be considered API breakage, and that holds back effectively
the entire patch set until you're willing to issue a new API-break
release :/

Post by Tanu Kaskinen
I can't now find the patch that changes the callback types, but there
are many patches that change the callback implementation signatures,
and if there's no corresponding change to the callback types, that
causes warnings (for example, try applying patch 2 alone, and you'll
get warnings when building pulseaudio).

[Tanu Kaskinen]

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen
Patch 4 changes the callback signatures.

Right, but only in the mainloop api vtable, which user's only read
values from, so I wasn't expecting that to be an issue.

Applications can also write to it, if they implement their own
mainloops.

Ah, that use case was not mentioned in our previous discussion. I'd
asked if the api struct was immutable, or whether there was any
intention to allow users to modify it, to say hijack one or more
function implementations (i.e. a partial customised mainloop), and
you'd said that there was no such intention. That discussion left me
with the understanding that custom/partial-custom mainloops weren't a
thing.
Do you happen to know of any examples of such programs implementing
custom mainloops that create and pass around a mainloop-api vtable
populated with their own pointers?

Debian Code Search gives a few examples:

https://codesearch.debian.net/search?q=defer_new+%3D+

Post by j***@gmail.com

Post by Tanu Kaskinen
When I wrote my previous mail, I thought that patch 4 also changes
the event callback types (pa_io_event_cb_t etc.), and that was what I
was primarily concerned about

Yeah I deliberately left change to those callback signatures until a
later commit (#25 of 26 I believe), for precisely those same concerns.

I don't know if you realized, but already patch 2 depends on that
change.

Post by j***@gmail.com

Post by Tanu Kaskinen
(but applications implementing their own mainloops is still a real
problem).

Right... :/
Well, I guess no matter how rare custom mainloops might be, if custom
mainloops are a legit supported feature, then this change breaks it and
thus must be considered API breakage, and that holds back effectively
the entire patch set until you're willing to issue a new API-break
release :/

If we're ever going to make "libpulse 2", we need to have a plan about
everything that we want to change. I don't want to create a new library
just to constify the mainloop API... Feel free to start a wiki page
that lists compatibility breaking changes that we might want to make.

[Pali Rohár]

Post by Tanu Kaskinen
If we're ever going to make "libpulse 2", we need to have a plan about
everything that we want to change. I don't want to create a new library
just to constify the mainloop API... Feel free to start a wiki page
that lists compatibility breaking changes that we might want to make.

If you are going to do such think like a new libpulse client library, I
would suggest a larger discussion and ideally also tell about it to
projects like https://github.com/i-rinat/apulse. And developers of
existing libpulse applications should be part of discussion too.

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen
Patch 4 changes the callback signatures.

Right, but only in the mainloop api vtable, which user's only read
values from, so I wasn't expecting that to be an issue.

Applications can also write to it, if they implement their own
mainloops.

Ah, that use case was not mentioned in our previous discussion. I'd
asked if the api struct was immutable, or whether there was any
intention to allow users to modify it, to say hijack one or more
function implementations (i.e. a partial customised mainloop), and
you'd said that there was no such intention. That discussion left me
with the understanding that custom/partial-custom mainloops weren't a
thing.
Do you happen to know of any examples of such programs implementing
custom mainloops that create and pass around a mainloop-api vtable
populated with their own pointers?

https://codesearch.debian.net/search?q=defer_new+%3D+

Ok, thanks I'll take a look at those

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen
When I wrote my previous mail, I thought that patch 4 also
changes
the event callback types (pa_io_event_cb_t etc.), and that was what I
was primarily concerned about

Yeah I deliberately left change to those callback signatures until a
later commit (#25 of 26 I believe), for precisely those same
concerns.

I don't know if you realized, but already patch 2 depends on that
change.

Yes, but patches 2 & 3 only depend on it to avoid a warning, which I
hoped was acceptable until the later few API change patches would make
it into a new major version

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen
(but applications implementing their own mainloops is still a real
problem).

Right... :/
Well, I guess no matter how rare custom mainloops might be, if custom
mainloops are a legit supported feature, then this change breaks it and
thus must be considered API breakage, and that holds back
effectively
the entire patch set until you're willing to issue a new API-break
release :/

If we're ever going to make "libpulse 2", we need to have a plan about
everything that we want to change. I don't want to create a new library
just to constify the mainloop API... Feel free to start a wiki page
that lists compatibility breaking changes that we might want to make.

Oh. I'm not used to library maintenance. I wasn't expecting a new lib
to be necessary for some ABI-compatible API changes, just a new major
version with an API bump to the API declaration in the version
header...

On the topic of such a list though, all that I would have in mind
besides the mainloop-api thing would be the purging of autoload (plus
removal of a related attribute in the middle of a struct that my
previous 'purge-autoload' patch deliberately left untouched), and a
switch to use of boolean parameters.