On 05/10/2013 12:11 PM, Eric Blake wrote:
> 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.
>
>> @@ -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.
Here, I _think_ it would be okay to change:
typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn,
virDomainPtr dom,
int phase,
virDomainEventGraphicsAddressPtr local,
virDomainEventGraphicsAddressPtr remote,
const char
*authScheme,
virDomainEventGraphicsSubjectPtr subject,
void *opaque);
to:
typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn,
virDomainPtr dom,
int phase,
const
virDomainEventGraphicsAddressPtr local,
const
virDomainEventGraphicsAddressPtr remote,
const char
*authScheme,
const
virDomainEventGraphicsSubjectPtr subject,
void *opaque);
Although this is an API change, I think that real callers won't be
impacted. Why?
1. these callback members are read-only (unlike the ConnectCredential
callback struct which was read-write), so it is less likely that someone
is trying to assign into the struct members.
2. The only way to register a virConnectDomainEventGraphicsCallback is
to cast it through a call to virConnectDomainEventRegisterAny. That is,
even if the user's callback function leaves out the const, we never use
the typedef as the direct type of any API parameter. Since they are
already casting their function pointer into a munged type before
registering it, their code will continue to compile.
IF you agree with my reasoning about adding 'const' to this particular
callback's signature, THEN we can also remove 'const' from the members
within those types (that is, your patch did the THEN part; I'd be okay
acking if you also add the IF part in the same commit, isolated to an
independent patch since it touches user-visible declarations).
We can't do that for ConnectCredential, because that type is used
directly with no mandated user-casting, and because that type is
read-write instead of read-only like these two types.
--
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