10:04 a.m.
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(-)
diff --git a/generator/generator b/generator/generator
index 951349d..ed05ea9 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3305,8 +3305,8 @@ let generate_include_libnbd_h () =
pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
pr "\n";
pr "extern int nbd_add_close_callback (struct nbd_handle *h,\n";
- pr " nbd_close_callback cb,\n";
- pr " void *user_data);\n";
+ pr " void *user_data,\n";
+ pr " nbd_close_callback cb);\n";
pr "#define LIBNBD_HAVE_NBD_ADD_CLOSE_CALLBACK 1\n";
pr "\n";
List.iter (
@@ -3593,7 +3593,7 @@ from other programming languages.
typedef void (*nbd_close_callback) (void *user_data);
int nbd_add_close_callback (struct nbd_handle *nbd,
- nbd_close_callback cb, void *user_data);
+ void *user_data, nbd_close_callback cb);
";
@@ -4195,7 +4195,7 @@ let print_python_binding name { args; ret } =
| Closure (false, _) -> ()
| Closure (true, _) ->
pr " /* This ensures the callback data is freed eventually. */\n";
- pr " nbd_add_close_callback (h, free_%s_user_data, user_data);\n" name
+ pr " nbd_add_close_callback (h, user_data, free_%s_user_data);\n" name
| Flags _ -> ()
| Int _ -> ()
| Int64 _ -> ()
@@ -4862,7 +4862,7 @@ let print_ocaml_binding (name, { args; ret }) =
(match has_closures with
| None | Some false -> ()
| Some true ->
- pr " nbd_add_close_callback (h, free_%s_user_data, user_data);\n" name
+ pr " nbd_add_close_callback (h, user_data, free_%s_user_data);\n" name
);
let errcode =
diff --git a/lib/handle.c b/lib/handle.c
index 5003227..00d0ac2 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -203,7 +203,7 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const char
*name)
*/
int
nbd_add_close_callback (struct nbd_handle *h,
- nbd_close_callback cb, void *user_data)
+ void *user_data, nbd_close_callback cb)
{
int ret;
struct close_callback *cc;
--
2.22.0
10:39 a.m.
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:38 a.m.
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
11:56 a.m.
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
12:42 p.m.
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
1958
days inactive
1958
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones