On 05/03/2013 08:53 AM, Michal Privoznik wrote: > --- > include/libvirt/libvirt.h.in | 10 +- I think this file needs to be its own patch, because it has API considerations. More below. > src/qemu/qemu_capabilities.c | 79 ++++---- > src/qemu/qemu_cgroup.c | 4 +- > src/qemu/qemu_command.c | 419 +++++++++++++++++-------------------------- > src/qemu/qemu_conf.c | 58 +++--- > src/qemu/qemu_domain.c | 26 ++- > src/qemu/qemu_driver.c | 113 ++++-------- > src/qemu/qemu_hotplug.c | 15 +- > src/qemu/qemu_migration.c | 20 +-- > src/qemu/qemu_monitor_json.c | 63 ++----- > src/qemu/qemu_monitor_text.c | 15 +- > src/qemu/qemu_process.c | 64 +++---- > src/remote/remote_driver.c | 2 +- > 13 files changed, 333 insertions(+), 555 deletions(-) > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index 693b834..9a8090d 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -1294,7 +1294,7 @@ typedef enum { > > struct _virConnectCredential { > int type; /* One of virConnectCredentialType constants */ > - const char *prompt; /* Prompt to show to user */ > + char *prompt; /* Prompt to show to user */ 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). > @@ -4504,8 +4504,8 @@ typedef enum { > */ > struct _virDomainEventGraphicsAddress { > int family; /* Address family, virDomainEventGraphicsAddressType */ > - const char *node; /* Address of node (eg IP address, or UNIX path) */ > - const char *service; /* Service name/number (eg TCP port, or NULL) */ > + char *node; /* Address of node (eg IP address, or UNIX path) */ > + char *service; /* Service name/number (eg TCP port, or NULL) */ > }; > typedef struct _virDomainEventGraphicsAddress virDomainEventGraphicsAddress; > typedef virDomainEventGraphicsAddress *virDomainEventGraphicsAddressPtr; > @@ -4520,8 +4520,8 @@ typedef virDomainEventGraphicsAddress *virDomainEventGraphicsAddressPtr; > * some examples are 'x509dname' and 'saslUsername'. > */ > struct _virDomainEventGraphicsSubjectIdentity { > - const char *type; /* Type of identity */ > - const char *name; /* Identity value */ > + char *type; /* Type of identity */ > + char *name; /* Identity value */ 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list