Re: [libvirt] [PATCH] phyp: first part of storage management driver

14 May 2010

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);

    

Re: [libvirt] [PATCH] phyp: first part of storage management driver

Stefan Berger