[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] cleanup virDomainCreateLinux into virDomainDefineXML



On Thu, Oct 09, 2008 at 05:16:37PM +0200, Daniel Veillard wrote:
>   As promised in the libvirt-qpid thread, virDomainCreateLinux() call
> name makes no sense (anymore), and it should be renamed, i guess since
> virDomainCreate() already exist and by similarity with
> virDomainDefineXML() the best name is virDomainCreateXML().

Agreed - we already have virNetworkCreateXML(), so virDomainCreateXML()
makes perfect sense.

>   The associated patch rename virDomainCreateLinux to virDomainCreateXML
> create a small function virDomainCreateLinux calling the former,
> document it as deprecated. To help comprehension of the source code
> it's also best to rename the internal driver method in a similar
> way, which inflates the patch a bit but is IMHO worth it.

Since this doesn't impact the on-the-wire RPC number, this seems
like a worthwhile change too.

> >   The patch also fixes a few #elif define(__sun__) and turn them
> into the correct #elif defined(__sun__) cpp instructions, and changes
> include/libvirt/virterror.h to improve the generated HTML page about the
> deprected fileds in the structure, since we need to regenerate the
> API xml and the docs, it was a good opportunity for that small change.
> 
>   I removed the docs/ subdir part from the patch as it's generated
> and mostly unreadable, that keeps it smaller too.

ACK to this all with just a few minor nit-picks.

>  
> +/**
> + * virDomainCreateLinux:
> + * @conn: pointer to the hypervisor connection
> + * @xmlDesc: string containing an XML description of the domain
> + * @flags: callers should always pass 0
> + *
> + * Deprecated after 0.4.6 use virDomainCreateXML()

Can we make this a little clear that they provide 100% functionally
identical impl. Something like

   * Deprecated after 0.4.6.
   * Renamed to virDomainCreateXML() providing identical functionality.
   * This existing name will left indefinitely for API compatability.


> Index: qemud/remote_protocol.c
> ===================================================================
> RCS file: /data/cvs/libxen/qemud/remote_protocol.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 remote_protocol.c
> --- qemud/remote_protocol.c	5 Sep 2008 12:03:45 -0000	1.18
> +++ qemud/remote_protocol.c	9 Oct 2008 14:47:54 -0000
> @@ -618,7 +618,7 @@ xdr_remote_num_of_domains_ret (XDR *xdrs
>  }
>  
>  bool_t
> -xdr_remote_domain_create_linux_args (XDR *xdrs, remote_domain_create_linux_args *objp)
> +xdr_remote_domain_create_xml_args (XDR *xdrs, remote_domain_create_xml_args *objp)
>  {
>  
>           if (!xdr_remote_nonnull_string (xdrs, &objp->xml_desc))
> @@ -629,7 +629,7 @@ xdr_remote_domain_create_linux_args (XDR
>  }
>  
>  bool_t
> -xdr_remote_domain_create_linux_ret (XDR *xdrs, remote_domain_create_linux_ret *objp)
> +xdr_remote_domain_create_xml_ret (XDR *xdrs, remote_domain_create_xml_ret *objp)
>  {
>  
>           if (!xdr_remote_nonnull_domain (xdrs, &objp->dom))
> Index: qemud/remote_protocol.h
> ===================================================================
> RCS file: /data/cvs/libxen/qemud/remote_protocol.h,v
> retrieving revision 1.18
> diff -u -p -r1.18 remote_protocol.h
> --- qemud/remote_protocol.h	5 Sep 2008 12:03:45 -0000	1.18
> +++ qemud/remote_protocol.h	9 Oct 2008 14:47:54 -0000
> @@ -318,16 +318,16 @@ struct remote_num_of_domains_ret {
>  };
>  typedef struct remote_num_of_domains_ret remote_num_of_domains_ret;
>  
> -struct remote_domain_create_linux_args {
> +struct remote_domain_create_xml_args {
>          remote_nonnull_string xml_desc;
>          int flags;
>  };
> -typedef struct remote_domain_create_linux_args remote_domain_create_linux_args;
> +typedef struct remote_domain_create_xml_args remote_domain_create_xml_args;
>  
> -struct remote_domain_create_linux_ret {
> +struct remote_domain_create_xml_ret {
>          remote_nonnull_domain dom;
>  };
> -typedef struct remote_domain_create_linux_ret remote_domain_create_linux_ret;
> +typedef struct remote_domain_create_xml_ret remote_domain_create_xml_ret;
>  
>  struct remote_domain_lookup_by_id_args {
>          int id;
> @@ -1264,8 +1264,8 @@ extern  bool_t xdr_remote_domain_memory_
>  extern  bool_t xdr_remote_list_domains_args (XDR *, remote_list_domains_args*);
>  extern  bool_t xdr_remote_list_domains_ret (XDR *, remote_list_domains_ret*);
>  extern  bool_t xdr_remote_num_of_domains_ret (XDR *, remote_num_of_domains_ret*);
> -extern  bool_t xdr_remote_domain_create_linux_args (XDR *, remote_domain_create_linux_args*);
> -extern  bool_t xdr_remote_domain_create_linux_ret (XDR *, remote_domain_create_linux_ret*);
> +extern  bool_t xdr_remote_domain_create_xml_args (XDR *, remote_domain_create_xml_args*);
> +extern  bool_t xdr_remote_domain_create_xml_ret (XDR *, remote_domain_create_xml_ret*);
>  extern  bool_t xdr_remote_domain_lookup_by_id_args (XDR *, remote_domain_lookup_by_id_args*);
>  extern  bool_t xdr_remote_domain_lookup_by_id_ret (XDR *, remote_domain_lookup_by_id_ret*);
>  extern  bool_t xdr_remote_domain_lookup_by_uuid_args (XDR *, remote_domain_lookup_by_uuid_args*);
> @@ -1445,8 +1445,8 @@ extern bool_t xdr_remote_domain_memory_p
>  extern bool_t xdr_remote_list_domains_args ();
>  extern bool_t xdr_remote_list_domains_ret ();
>  extern bool_t xdr_remote_num_of_domains_ret ();
> -extern bool_t xdr_remote_domain_create_linux_args ();
> -extern bool_t xdr_remote_domain_create_linux_ret ();
> +extern bool_t xdr_remote_domain_create_xml_args ();
> +extern bool_t xdr_remote_domain_create_xml_ret ();
>  extern bool_t xdr_remote_domain_lookup_by_id_args ();
>  extern bool_t xdr_remote_domain_lookup_by_id_ret ();
>  extern bool_t xdr_remote_domain_lookup_by_uuid_args ();

All the files in qemud/ named  remote_XXXX  are automatically generated
from remote_protocol.x, so if you change that one file the others should
update. I don't see a change to remote_protocol.x in this diff, so don;t
forget to update it when committing, or next protocol update will revert
your changes by mistake.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]