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

Re: [libvirt] [RFC] Power Hypervisor Libvirt support



Hello Kaitlin,

>  > +#include "domain_conf.h"
>  > +/* Main driver state */
>  > +typedef struct __phyp_driver phyp_driver_t;
>  > +struct __phyp_driver {
>  > +    virMutex lock;
>  > +
>  > +    virCapsPtr *caps;
>  > +
>  > +    virDomainObjList *domains;
>  > +
>  > +    char *configDir;
>  > +    char *autostartDir;
>  > +    char *stateDir;
>  > +    char *logDir;
>  > +    int have_netns;
>  > +};
> 
> I don't see this struct / typedef being used anywhere.  Will this be 
> used for something in the future?

I am still planning how I should store all driver information, so I
think I should keep this for now.

> 
> 
>  > +static int
>  > +phypListDomains(virConnectPtr conn, int *ids, int nids)
>  > +{
>  > +    SSH_SESSION *ssh_session = conn->privateData;
>  > +    const char *managed_system = conn->uri->server;
>  > +    int ret = 0;
>  > +    char id_c[10];
>  > +    unsigned int i = 0, j = 0;
>  > +    char *cmd;
>  > +    char *textual_return;
>  > +
>  > +    if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(managed_system))) {
>  > +        return PHYP_NO_MEM;
>  > +    }
>  > +
>  > +    if (VIR_ALLOC_N(textual_return, MAXSIZE)) {
>  > +        return PHYP_NO_MEM;
>  > +    }
>  > +
>  > +    nids = 0;
>  > +
>  > +    memset(id_c, 0, 10);
>  > +    sprintf(cmd, "lssyscfg -r lpar -m %s -F lpar_id", managed_system);
>  > +    ret = __inner_exec_command(ssh_session, cmd, textual_return);
> 
> You don't check ret to see if an error occured.

Already fixed, but thanks anyway.

> 
>  > +static virDomainPtr
>  > +phypDomainLookupByID(virConnectPtr conn, int lpar_id)
>  > +{
>  > +    SSH_SESSION *ssh_session = conn->privateData;
>  > +    virDomainPtr dom = NULL;
>  > +    const char *managed_system = conn->uri->server;
>  > +    char *lpar_name;
>  > +    unsigned char *lpar_uuid;
> 
> Any reason not to use a virDomainObjPtr to hold these values?

Good point. I should follow the patterns and start using virDomainObj to
store name and uuid. But unfortunately ssh_session and the
managed_system name I still need to keep them as I did.

> 
> 
>  > +
>  > +    if (VIR_ALLOC_N(lpar_name, 100 * sizeof(char)))
>  > +        return NULL;
>  > +
>  > +    if (VIR_ALLOC_N(lpar_uuid, 100 * sizeof(char)))
>  > +        return NULL;
> 
> You then wouldn't need to use a magic number here.  You could just do an 
> alloc of the virDomainObjPtr object itself.

Yes, this problem will be solved when I change it to virDomainObj.


Thanks again for the comments.
[]'s

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885 
otubo linux vnet ibm com


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