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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130510/d664daca/attachment-0001.sig>