libvir-list-bounces@redhat.com wrote on 05/13/2010 10:53:53 AM:


>
> Hello all,
>
> This is the first patch about storage management driver on IBM Power
> Hypervisor. I reviewd a couple of times but perhaps I might be missing
> something. Any comments are welcome.

Please send the patches inline.

>
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index cec99b1..7bd203f 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -1665,6 +1665,262 @@ virDriver phypDriver = {
>      NULL, /* domainSnapshotDelete */
>  };
>  
> +virStorageDriver phypStorageDriver = {
> +    .name = "PHYP",
> +    .open = phypStorageOpen,
> +    .close = phypStorageClose,
> +
> +    .numOfPools = phypNumOfStoragePools,
> +    .listPools = phypListStoragePools,
> +    .numOfDefinedPools = NULL,
> +    .listDefinedPools = NULL,
> +    .findPoolSources = NULL,
> +    .poolLookupByName = phypSPLookupByName,
> +    .poolLookupByUUID = NULL,
> +    .poolLookupByVolume = NULL,
> +    .poolCreateXML = NULL,
> +    .poolDefineXML = NULL,
> +    .poolBuild = NULL,
> +    .poolUndefine = NULL,
> +    .poolCreate = NULL,
> +    .poolDestroy = NULL,
> +    .poolDelete = NULL,
> +    .poolRefresh = NULL,
> +    .poolGetInfo = NULL,
> +    .poolGetXMLDesc = NULL,
> +    .poolGetAutostart = NULL,
> +    .poolSetAutostart = NULL,
> +    .poolNumOfVolumes = NULL,
> +    .poolListVolumes = NULL,
> +
> +    .volLookupByName = NULL,
> +    .volLookupByKey = NULL,
> +    .volLookupByPath = NULL,
> +    .volCreateXML = NULL,
> +    .volCreateXMLFrom = NULL,
> +    .volDelete = NULL,
> +    .volGetInfo = NULL,
> +    .volGetXMLDesc = NULL,
> +    .volGetPath = NULL,
> +    .poolIsActive = NULL,
> +    .poolIsPersistent = NULL
> +};
> +
> +int
> +phypGetStoragePoolUUID(unsigned char *sp_uuid, const char *sp_name,
> +                       virConnectPtr conn)
> +{
> +    ConnectionData *connection_data = conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = conn->privateData;
> +    LIBSSH2_SESSION *session = connection_data->session;
> +    char *managed_system = phyp_driver->managed_system;
> +    int vios_id = phyp_driver->vios_id;
> +    int exit_status = 0;
> +    char *cmd = NULL;
> +    char *ret = NULL;
> +
> +    if (virAsprintf(&cmd,
> +                    "viosvrcmd -m %s --id %d -c 'lsdev -dev %s -attr "
> +                    "vgserial_id'|sed '1d'|sed '1d'",
> +                    managed_system, vios_id, sp_name, sp_name) < 0) {
> +        virReportOOMError();
> +        goto err;
> +    }
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (memmove(sp_uuid, ret, VIR_UUID_BUFLEN) == NULL)
> +        goto err;
> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return 0;
> +
> +  err:
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return -1;
> +}
> +
> +int
> +phypGetStoragePoolID(const char *sp_name, virConnectPtr conn)
> +{
> +    ConnectionData *connection_data = conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = conn->privateData;
> +    LIBSSH2_SESSION *session = connection_data->session;
> +    char *managed_system = phyp_driver->managed_system;
> +    int vios_id = phyp_driver->vios_id;
> +    int exit_status = 0;
> +    int sp_id = 0;
> +    char *char_ptr;
> +    char *cmd = NULL;
> +    char *ret = NULL;
> +
> +    if (virAsprintf(&cmd,
> +                    "viosvrcmd -m %s --id %d -c 'lssp -field Pool'"
> +                    "|sed '1d'|grep -n %s|sed -e 's/:.*$//g'",
> +                    managed_system, vios_id, sp_name) < 0) {
> +        virReportOOMError();
> +        goto err;
> +    }
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (virStrToLong_i(ret, &char_ptr, 10, &sp_id) == -1)
> +        goto err;

char_ptr not being used afterward -> could supply NULL instead.

> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return sp_id;
> +
> +  err:
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return -1;
> +}
> +
> +virStoragePoolPtr
> +phypSPLookupByName(virConnectPtr conn, const char *sp_name)
> +{
> +    virStoragePoolPtr sp = NULL;
> +    int sp_id = 0;
> +    unsigned char sp_uuid[VIR_UUID_BUFLEN];
> +
> +    sp_id = phypGetStoragePoolID(sp_name, conn);
> +    if (sp_id == -1)
> +        return NULL;
> +
> +    if (phypGetStoragePoolUUID(sp_uuid, sp_name, conn) == -1)
> +        return NULL;
> +
> +    sp = virGetStoragePool(conn, sp_name, sp_uuid);
> +
> +    if (sp)
> +        return sp;
> +    else
> +        return NULL;

Doesn't seem necessary to do if - then here.

Just a 'return sp;' should do the trick.


> +}
> +
> +int
> +phypNumOfStoragePools(virConnectPtr conn)

Make this one static since it's accessed through the interface? Or is it exported for some debugging purpose?

> +{
> +    ConnectionData *connection_data = conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = conn->privateData;
> +    LIBSSH2_SESSION *session = connection_data->session;
> +    int exit_status = 0;
> +    int nsp = 0;
> +    char *char_ptr;
> +    char *cmd = NULL;
> +    char *ret = NULL;
> +    char *managed_system = phyp_driver->managed_system;
> +    int vios_id = phyp_driver->vios_id;
> +
> +    if (virAsprintf(&cmd,
> +                    "viosvrcmd -m %s --id %d -c 'lssp -field "
> +                    "Pool'|sed '1d'|grep -c ^.*$",
> +                    managed_system, vios_id) < 0) {
> +        virReportOOMError();
> +        goto err;
> +    }
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (virStrToLong_i(ret, &char_ptr, 10, &nsp) == -1)
> +        goto err;

NULL rather than char_ptr?

> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return nsp;
> +
> +  err:
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return -1;
> +}
> +
> +int
> +phypListStoragePools(virConnectPtr conn, char **const pools, int npools)

Static function?

> +{
> +    ConnectionData *connection_data = conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = conn->privateData;
> +    LIBSSH2_SESSION *session = connection_data->session;
> +    char *managed_system = phyp_driver->managed_system;
> +    int vios_id = phyp_driver->vios_id;
> +    int exit_status = 0;
> +    int got = 0;
> +    int i;
> +    char *cmd = NULL;
> +    char *ret = NULL;
> +    char *storage_pools = NULL;
> +    char *char_ptr2 = NULL;
> +
> +    if (virAsprintf
> +        (&cmd,
> +         "viosvrcmd -m %s --id %d -c 'lssp -field Pool'|sed '1d'",
> +         managed_system, vios_id) < 0) {
> +        virReportOOMError();
> +        goto err;
> +    }
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    /* I need to parse the textual return in order to get the storage
> pools */
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +    else {
> +        storage_pools = ret;
> +
> +        while (got < npools) {
> +            char_ptr2 = strchr(storage_pools, '\n');
> +
> +            if (char_ptr2) {
> +                *char_ptr2 = '\0';
> +                if ((pools[got++] = strdup(storage_pools)) == NULL) {
> +                    virReportOOMError();
> +                    goto err;
> +                }
> +                char_ptr2++;
> +                storage_pools = char_ptr2;
> +            } else
> +                break;
> +        }
> +    }
> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return got;
> +
> +  err:
> +    for (i = 0; i < got; i++)
> +        VIR_FREE(pools[i]);
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return -1;
> +}
> +
> +virDrvOpenStatus
> +phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                int flags ATTRIBUTE_UNUSED)

Static function?

> +{
> +    return VIR_DRV_OPEN_SUCCESS;
> +}
> +
> +int
> +phypStorageClose(virConnectPtr conn ATTRIBUTE_UNUSED)

static function?

> +{
> +    return 0;
> +}
> +
>  int
>  phypBuildLpar(virConnectPtr conn, virDomainDefPtr def)
>  {
> @@ -2209,6 +2465,10 @@ waitsocket(int socket_fd, LIBSSH2_SESSION *
> session)
>  int
>  phypRegister(void)
>  {
> -    virRegisterDriver(&phypDriver);
> +    if (virRegisterDriver(&phypDriver) < 0 )
> +        return -1;
> +    if(virRegisterStorageDriver(&phypStorageDriver) < 0)
> +        return -1;
> +
>      return 0;
>  }
> diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h
> index f680994..edd0d6c 100644
> --- a/src/phyp/phyp_driver.h
> +++ b/src/phyp/phyp_driver.h
> @@ -69,6 +69,32 @@ struct _phyp_driver {
>      char *managed_system;
>  };
>  
> +
> +/*
> + * Common storage functions
> + * */
> +int phypGetStoragePoolID(const char *sp_name, virConnectPtr conn);
> +
> +int phypGetStoragePoolUUID(unsigned char *sp_uuid, const char *sp_name,
> +                           virConnectPtr conn);
> +
> +virStoragePoolPtr phypSPLookupByName(virConnectPtr conn, const char
> *name);
> +
> +int phypNumOfStoragePools(virConnectPtr conn);
> +
> +int phypListStoragePools(virConnectPtr conn, char **const pools,
> +                         int npools);
> +
> +virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                 virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                                 int flags ATTRIBUTE_UNUSED);
> +
> +int phypStorageClose(virConnectPtr conn);

If the don't need to be public then you can remove them from this file here.


   Stefan

> +
> +/*
> + * Common driver functions
> + * */
> +
>  int phypCheckSPFreeSapce(virConnectPtr conn, int required_size, char
> *sp);
>  
>  int phypGetVIOSPartitionID(virConnectPtr conn);