Eric Blake
2019-Jul-16 16:38 UTC
Re: [Libguestfs] [PATCH libnbd] generator: Swap parameters of nbd_add_close_callback.
On 7/16/19 10:39 AM, Eric Blake wrote:> On 7/16/19 10:04 AM, Richard W.M. Jones wrote: >> The API changes from: >> >> int nbd_add_close_callback (struct nbd_handle *h, >> nbd_close_callback cb, >> void *user_data); >> >> to: >> >> int nbd_add_close_callback (struct nbd_handle *h, >> void *user_data, >> nbd_close_callback cb); >> >> The second way is consistent with how other callbacks work throughout >> the API (ie. having the user_data passed first). >> --- >> generator/generator | 10 +++++----- >> lib/handle.c | 2 +- >> 2 files changed, 6 insertions(+), 6 deletions(-) > > ACK. >A bit of bike-shedding: In libc, we have qsort_r() which takes the function pointer before the opaque data. I'm trying to find other common frameworks that have common Closure conventions, to see if we should instead swap our nbd_aio_FOO functions to take user_data after the function pointers, instead of this switch to the nbd_add_close_callback parameter order. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Jul-16 16:56 UTC
Re: [Libguestfs] [PATCH libnbd] generator: Swap parameters of nbd_add_close_callback.
On Tue, Jul 16, 2019 at 11:38:36AM -0500, Eric Blake wrote:> On 7/16/19 10:39 AM, Eric Blake wrote: > > On 7/16/19 10:04 AM, Richard W.M. Jones wrote: > >> The API changes from: > >> > >> int nbd_add_close_callback (struct nbd_handle *h, > >> nbd_close_callback cb, > >> void *user_data); > >> > >> to: > >> > >> int nbd_add_close_callback (struct nbd_handle *h, > >> void *user_data, > >> nbd_close_callback cb); > >> > >> The second way is consistent with how other callbacks work throughout > >> the API (ie. having the user_data passed first). > >> --- > >> generator/generator | 10 +++++----- > >> lib/handle.c | 2 +- > >> 2 files changed, 6 insertions(+), 6 deletions(-) > > > > ACK. > > > > A bit of bike-shedding: > > In libc, we have qsort_r() which takes the function pointer before the > opaque data. > > I'm trying to find other common frameworks that have common Closure > conventions, to see if we should instead swap our nbd_aio_FOO functions > to take user_data after the function pointers, instead of this switch to > the nbd_add_close_callback parameter order.I don't really mind except to say we should do it consistently one way or the other, and we should decide which way to do it fairly soon :-) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Eric Blake
2019-Jul-16 17:42 UTC
Re: [Libguestfs] [PATCH libnbd] generator: Swap parameters of nbd_add_close_callback.
On 7/16/19 11:56 AM, Richard W.M. Jones wrote:>>>> The second way is consistent with how other callbacks work throughout >>>> the API (ie. having the user_data passed first). >>>> --- >>>> generator/generator | 10 +++++----- >>>> lib/handle.c | 2 +- >>>> 2 files changed, 6 insertions(+), 6 deletions(-) >>> >>> ACK. >>> >> >> A bit of bike-shedding: >> >> In libc, we have qsort_r() which takes the function pointer before the >> opaque data. >> >> I'm trying to find other common frameworks that have common Closure >> conventions, to see if we should instead swap our nbd_aio_FOO functions >> to take user_data after the function pointers, instead of this switch to >> the nbd_add_close_callback parameter order.pthread_create()/pthread_cleanup_push() in libc, and glib's g_thread_new(), also use opaque after function pointer. More history on qsort_r (much of which I did): http://austingroupbugs.net/view.php?id=900 Originally, BSD and glibc picked opposite orderings, but based on my arguments about the glibc behavior being slightly easier to use was enough to convince the FreeBSD authors to propose a patch to switch their qsort_r to match glibc. As an odd example, libvirt has: int virConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, /* Optional, to filter */ int eventID, virConnectDomainEventGenericCallback cb, void *opaque, virFreeCallback freecb); where there are two callback functions with a shared opaque, but the opaque comes between the two arguments. But if you overlook the point of the freecb, then it falls in line with the idea of opaque after the function pointer, rather than before.> > I don't really mind except to say we should do it consistently one way > or the other, and we should decide which way to do it fairly soon :-)Indeed, and since we've already broken API, now is the time to make the switch (whichever one we do switch) so that we can cut another pre-stable release. I'm now leaning 60-40 towards dropping this patch and instead teaching the generator to gather the opaque argument last rather than first. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Possibly Parallel Threads
- Re: [PATCH libnbd] generator: Swap parameters of nbd_add_close_callback.
- [PATCH libnbd] generator: Swap parameters of nbd_add_close_callback.
- Re: [PATCH libnbd] generator: Swap parameters of nbd_add_close_callback.
- [PATCH libnbd v3 2/2] lib: Remove nbd_add_close_callback.
- [libnbd PATCH] generator: Prefer closure opaque after function pointer in C