On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso@xxxxxxxxxx> wrote: > On 11/14/19 12:44 PM, Marc Hartmayer wrote: >> The commit 'close callback: move it to driver' (88f09b75eb99) moved >> the responsibility for the close callback to the driver. But if the >> driver doesn't support the connectRegisterCloseCallback API this >> function does nothing, even no unsupported error report. This behavior >> may lead to problems, for example memory leaks, as the caller cannot >> differentiate whether the close callback was 'really' registered or >> not. >> > > > Full context: > v1 subtrhead with jferlan and danpb: > https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html > https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html > > v2 subthread with john: > https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html > > My first thought is, why not just make this API start raising an error > if it isn't supported? > > But you tried that here: > https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html First, thanks for doing all the “history research”. > > I'm not really sure I buy the argument that we can't change the > semantics of the API here. This is the only callback API that seems to > not raise an explicit error. It's documented to raise an error. And > there's possible memory leak due the ambiguity. If we’re doing this so let’s fix the behavior of 'virConnectUnregisterCloseCallback' as well. > > Yeah I see that virsh needs to be updated to match. In practice virsh > shouldn't be a problem: this issue will only hit for local drivers, and > virsh and client library will be updated together for that case. > > In theory if a python app is using this API and not expecting an > exception, it could cause a regression. But it's also informing them > that, hey, that connection callback you requested wasn't working in the > first place. So it's arguably a correctness issue. > > So IMO we should be able to adjust this to return a proper error. > > > BUT, if we stick with this direction, then we need to extend the doc > text here to describe all of this: > > * Returns -1 if the driver can support close callback, but registering > one failed. User must free opaque? > * Returns 0 if the driver does not support close callback. We will free > data for you > * Returns 0 if the driver successfully registered a close callback. When > that callback is triggered, opaque will be free'd > > But that sounds pretty nutty IMO :/ I know… > >> Therefore call directly @freecb if the connectRegisterCloseCallback >> API is not supported by the driver used by the connection and update >> the documentation. >> >> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx> >> --- >> src/libvirt-host.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/src/libvirt-host.c b/src/libvirt-host.c >> index 221a1b7a4353..94383ed449a9 100644 >> --- a/src/libvirt-host.c >> +++ b/src/libvirt-host.c >> @@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn) >> * @conn: pointer to connection object >> * @cb: callback to invoke upon close >> * @opaque: user data to pass to @cb >> - * @freecb: callback to free @opaque >> + * @freecb: callback to free @opaque when not used anymore >> * >> * Registers a callback to be invoked when the connection >> * is closed. This callback is invoked when there is any >> @@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn) >> * >> * The @freecb must not invoke any other libvirt public >> * APIs, since it is not called from a re-entrant safe >> - * context. >> + * context. Only if virConnectRegisterCloseCallback is >> + * successful, @freecb will be called, otherwise the >> + * caller is responsible to free @opaque. > > This reads wrong to me. If cb() is successfully registered, then > freecb() is invoked automatically after cb() is called, right? Yep, or if the callback is unregistered. > This > comment makes it sound like freecb() is invoked immediately when > virConnectRegisterCloseCallback returns 0 I can reword it. > > Hopefully I'm not confusing things more :) No, I appreciate that you’re looking for it. > > - Cole > >> * >> * Returns 0 on success, -1 on error >> */ >> @@ -1428,9 +1430,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn, >> virCheckConnectReturn(conn, -1); >> virCheckNonNullArgGoto(cb, error); >> >> - if (conn->driver->connectRegisterCloseCallback && >> - conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0) >> - goto error; >> + if (conn->driver->connectRegisterCloseCallback) { >> + if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0) >> + goto error; >> + } else { >> + if (freecb) >> + freecb(opaque); >> + } >> >> return 0; >> >> > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
