Re: [libvirt] [PATCH v3 17/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/qemu/*

On 05/03/2013 08:53 AM, Michal Privoznik wrote: I think this file needs to be its own patch, because it has API considerations. More below. API change. Prior to this change, users passing a virConnectAuthCallbackPtr as part of their virConnectAuthPtr argument of virConnectOpenAuth could write that callback where they could modify the credential object passed to the callback. For example, I could do: int myCallback(virConnectCredentialPtr cred, unsigned int ncred, void *opaque) { cred->prompt = "new prompt"; return 0; } With your change, this will now fail to compile (attempt to assign a const char * into a char *). Granted, our documentation states that the callback writer should touch _only_ the result and resultlen field, but the fact that the user has to modify the incoming object at all means that the object itself has to declare which fields are input (with const) vs output. So I have no idea if someone writing such a callback handler is violating other aspects of code, and maybe our API break will never affect proper code; but at the same time, we make a strong promise of back-compat, and I'm reluctant to go back on that promise without strong reason. NACK to this change; API breaks are bad. Anywhere that we can't prep these fields using VIR_STRDUP (because VIR_STRDUP rightfully wants to assign to char*, not const char*), then we have to fix that code to do assignment in two parts (VIR_STRDUP into temporary char * var, then add const by assigning and/or casting into the callback struct we are populating). I need to look more at how these structs are used, but I have the same concern of an API break. (ABI-wise, it is still compatible). But since these two structs don't mix const and non-const char*, we might be able to get away with these changes. Sending now, to get my NACK out on the table, while I spend more time reviewing the rest of the patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org