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

Re: [libvirt] [PATCH] Wrong comments



On Mon, Sep 15, 2008 at 06:36:31PM +0400, Anton Protopopov wrote:
> And why are you writing
>     unsigned char state;
> instead of
>     virDomainState state;
> ?

In C enums are problematic because they can change size when new enum
members are added -- eg. if there are <= 256 members it might be a
char, but >= 256 members it might be an int.  This breaks ABI
guarantees so we tend to avoid using enums directly in structure
members, public function parameters and so on.

  diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
  index b91d729..24b5680 100644
  --- a/include/libvirt/libvirt.h.in
  +++ b/include/libvirt/libvirt.h.in
  @@ -85,7 +85,7 @@ typedef enum {
   typedef struct _virDomainInfo virDomainInfo;
   
   struct _virDomainInfo {
  -    unsigned char state;        /* the running state, one of virDomainFlags */
  +    unsigned char state;        /* the running state, one of virDomainState */
       unsigned long maxMem;       /* the maximum memory in KBytes allowed */
       unsigned long memory;       /* the memory in KBytes used by the domain */
       unsigned short nrVirtCpu;   /* the number of virtual CPUs for the domain */

+1 to this part of the patch -- an obvious mistake.

  diff --git a/src/libvirt.c b/src/libvirt.c
  index 54ed8cf..9f024be 100644
  --- a/src/libvirt.c
  +++ b/src/libvirt.c
  @@ -1231,7 +1231,7 @@ virDomainGetConnect (virDomainPtr dom)
    * virDomainCreateLinux:
    * @conn: pointer to the hypervisor connection
    * @xmlDesc: string containing an XML description of the domain
  - * @flags: an optional set of virDomainFlags
  + * @flags: an optional set of virConnectFlags
    *
    * Launch a new Linux guest domain, based on an XML description similar
    * to the one returned by virDomainGetXMLDesc()

-1.  This parameter is never used and so the documentation should just
say that it must be passed as 0.  (Don't bother sending an updated
patch -- if others agree with the first part, I'll just fix this when
I apply the patch to CVS).

Thanks,

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/


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