[libvirt] [PATCH] phyp: Adding storage management driver
Hello Folks, This is the result of a couple of months of hard work. I added the storage management driver to the Power Hypervisor driver. This is a big but simple patch, it's just a new set of functions, nothing more. I could split it into multiple commits, but the feature freeze starts in some hours and I really reed this feature to be included in the next release. This patch includes: * Storage driver: The set of pool-* and vol-* functions. * attach-disk function. * Support for IVM on the new functions. I've been looking at this code for a long time, so I apologize now for the silly mistakes that might be present. Looking forward to see the comments. Thanks! --- src/phyp/phyp_driver.c | 1638 +++++++++++++++++++++++++++++++++++++++++++++++- src/phyp/phyp_driver.h | 52 ++ 2 files changed, 1688 insertions(+), 2 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index cefb8be..77a74ef 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -56,6 +56,7 @@ #include "virterror_internal.h" #include "uuid.h" #include "domain_conf.h" +#include "storage_conf.h" #include "nodeinfo.h" #include "phyp_driver.h" @@ -1680,6 +1681,466 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus) } +static char * +phypGetLparProfile(virConnectPtr conn, int lpar_id) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + " -r prof --filter lpar_ids=%d -F name|head -n 1", + lpar_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static int +phypGetVIOSNextSlotNumber(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + virBufferAddLit(&buf, "echo $((`lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof --filter " + "profile_names=%s -F virtual_eth_adapters," + "virtual_opti_pool_id,virtual_scsi_adapters," + "virtual_serial_adapters|sed -e 's/\"//g' -e " + "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" + "|sort|tail -n 1` +1 ))", profile); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return slot; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypCreateServerSCSIAdapter(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + char *vios_name = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (! + (vios_name = + phypGetLparNAME(session, managed_system, vios_id, conn))) { + VIR_ERROR("%s", "Unable to get VIOS name"); + goto err; + } + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + if ((slot = phypGetVIOSNextSlotNumber(conn)) == -1) { + VIR_ERROR("%s", "Unable to get free slot number"); + goto err; + } + + /* Listing all the virtual_scsi_adapter interfaces, the new adapter must + * be appended to this list + * */ + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof --filter lpar_ids=%d,profile_names=%s" + " -F virtual_scsi_adapters|sed -e s/\"//g", + vios_id, profile); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + /* Here I change the VIOS configuration to append the new adapter + * with the free slot I got with phypGetVIOSNextSlotNumber. + * */ + virBufferAddLit(&buf, "chsyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof -i 'name=%s,lpar_id=%d," + "\"virtual_scsi_adapters=%s,%d/server/any/any/1\"'", + vios_name, vios_id, ret, slot); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + /* Finally I add the new scsi adapter to VIOS using the same slot + * I used in the VIOS configuration. + * */ + virBufferAddLit(&buf, "chhwres -r virtualio --rsubtype scsi "); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + "-p %s -o a -s %d -d 0 -a \"adapter_type=server\"", + vios_name, slot); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + VIR_FREE(profile); + VIR_FREE(vios_name); + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(profile); + VIR_FREE(vios_name); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static char * +phypGetVIOSFreeSCSIAdapter(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsmap -all -field svsa backing -fmt ,'"); + } else { + virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt ,"); + } + virBufferVSprintf(&buf, "|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + + +int +phypAttachDevice(virDomainPtr domain, const char *xml) +{ + + virConnectPtr conn = domain->conn; + ConnectionData *connection_data = domain->conn->networkPrivateData; + phyp_driverPtr phyp_driver = domain->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr = NULL; + char *cmd = NULL; + char *ret = NULL; + char *scsi_adapter = NULL; + int slot = 0; + char *vios_name = NULL; + char *profile = NULL; + virDomainDeviceDefPtr dev = NULL; + virDomainDefPtr def = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + def->os.type = strdup("aix"); + + if (def->os.type == NULL) { + virReportOOMError(); + goto err; + } + + dev = virDomainDeviceDefParse(phyp_driver->caps, def, xml, + VIR_DOMAIN_XML_INACTIVE); + if (!dev) { + virReportOOMError(); + goto err; + } + + if (! + (vios_name = + phypGetLparNAME(session, managed_system, vios_id, conn))) { + VIR_ERROR("%s", "Unable to get VIOS name"); + goto err; + } + + /* First, let's look for a free SCSI Adapter + * */ + if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { + /* If not found, let's create one. + * */ + if (phypCreateServerSCSIAdapter(conn) == -1) { + VIR_ERROR("%s", "Unable to create new virtual adapter"); + goto err; + } else { + if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { + VIR_ERROR("%s", "Unable to create new virtual adapter"); + goto err; + } + } + } + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'mkvdev -vdev %s -vadapter %s'", + dev->data.disk->src, scsi_adapter); + } else { + virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s", + dev->data.disk->src, scsi_adapter); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (!(profile = phypGetLparProfile(conn, domain->id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + /* Let's get the slot number for the adapter we just created + * */ + virBufferAddLit(&buf, "lshwres -r virtualio --rsubtype scsi "); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s", managed_system); + virBufferVSprintf(&buf, + "slot_num,backing_device|grep %s|cut -d, -f1", + dev->data.disk->src); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* Listing all the virtual_scsi_adapter interfaces, the new adapter must + * be appended to this list + * */ + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + "-r prof --filter lpar_ids=%d,profile_names=%s" + " -F virtual_scsi_adapters|sed -e s/\"//g", + vios_id, profile); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + /* Here I change the LPAR configuration to append the new adapter + * with the new slot we just created + * */ + virBufferAddLit(&buf, "chsyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + "-r prof -i 'name=%s,lpar_id=%d," + "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", + domain->name, domain->id, ret, slot, + vios_id, vios_name); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* Finally I add the new scsi adapter to VIOS using the same slot + * I used in the VIOS configuration. + * */ + virBufferAddLit(&buf, "chhwres -r virtualio --rsubtype scsi "); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", + domain->name, slot); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) { + VIR_ERROR0(_ + ("Possibly you don't have IBM Tools installed in your LPAR." + "Contact your support to enable this feature.")); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + VIR_FREE(def); + VIR_FREE(dev); + VIR_FREE(vios_name); + VIR_FREE(scsi_adapter); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + VIR_FREE(def); + VIR_FREE(dev); + VIR_FREE(vios_name); + VIR_FREE(scsi_adapter); + return -1; +} + virDriver phypDriver = { VIR_DRV_PHYP, "PHYP", phypOpen, /* open */ phypClose, /* close */ @@ -1725,7 +2186,7 @@ virDriver phypDriver = { NULL, /* domainCreateWithFlags */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ + phypAttachDevice, /* domainAttachDevice */ NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ NULL, /* domainDetachDeviceFlags */ @@ -1779,6 +2240,1175 @@ virDriver phypDriver = { NULL, /* domainSnapshotDelete */ }; +virStorageDriver phypStorageDriver = { + .name = "PHYP", + .open = phypStorageOpen, + .close = phypStorageClose, + + .numOfPools = phypNumOfStoragePools, + .listPools = phypListStoragePools, + .numOfDefinedPools = NULL, + .listDefinedPools = NULL, + .findPoolSources = NULL, + .poolLookupByName = phypStoragePoolLookupByName, + .poolLookupByUUID = phypGetStoragePoolLookUpByUUID, + .poolLookupByVolume = NULL, + .poolCreateXML = phypStoragePoolCreateXML, + .poolDefineXML = NULL, + .poolBuild = NULL, + .poolUndefine = NULL, + .poolCreate = NULL, + .poolDestroy = phypDestroyStoragePool, + .poolDelete = NULL, + .poolRefresh = NULL, + .poolGetInfo = NULL, + .poolGetXMLDesc = phypGetStoragePoolXMLDesc, + .poolGetAutostart = NULL, + .poolSetAutostart = NULL, + .poolNumOfVolumes = phypStoragePoolNumOfVolumes, + .poolListVolumes = phypStoragePoolListVolumes, + + .volLookupByName = phypVolumeLookupByName, + .volLookupByKey = NULL, + .volLookupByPath = phypVolumeLookupByPath, + .volCreateXML = NULL, + .volCreateXMLFrom = NULL, + .volDelete = NULL, + .volGetInfo = NULL, + .volGetXMLDesc = phypVolumeGetXMLDesc, + .volGetPath = phypVolumeGetPath, + .poolIsActive = NULL, + .poolIsPersistent = NULL +}; + +static int +phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) +{ + + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field lvid'", name); + } else { + virBufferVSprintf(&buf, "lslv %s -field lvid", name); + } + virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memmove(key, ret, PATH_MAX) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static char * +phypGetStoragePoolDevice(virConnectPtr conn, char *name) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lssp -detail -sp %s -field name'", name); + } else { + virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static unsigned long int +phypGetStoragePoolSize(virConnectPtr conn, char *name) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + int vios_id = phyp_driver->vios_id; + char *cmd = NULL; + char *ret = NULL; + int sp_size = 0; + char *char_ptr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lssp -detail -sp %s -field size'", name); + } else { + virBufferVSprintf(&buf, "lssp -detail -sp %s -field size", name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return sp_size; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, + unsigned int capacity, char *key) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int vios_id = phyp_driver->vios_id; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + char *cmd = NULL; + char *ret = NULL; + int exit_status = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'mklv -lv %s %s %d'", lvname, spname, + capacity); + } else { + virBufferVSprintf(&buf, "mklv -lv %s %s %d", lvname, spname, + capacity); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret); + goto err; + } + + if (phypVolumeGetKey(conn, key, lvname) == -1) + goto err;; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + + virStorageVolDefPtr voldef = NULL; + virStoragePoolDefPtr spdef = NULL; + virStorageVolPtr vol = NULL; + char *key = NULL; + + if (VIR_ALLOC(spdef) < 0) { + virReportOOMError(); + return NULL; + } + + if (VIR_ALLOC_N(key, PATH_MAX) < 0) { + virReportOOMError(); + return NULL; + } + + /* Filling spdef manually + * */ + if (pool->name != NULL) { + spdef->name = pool->name; + } else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR("%s", "Unable to determine storage pool's uuid."); + goto err; + } + + if ((spdef->capacity = + phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { + VIR_ERROR("%s", "Unable to determine storage pools's size."); + goto err; + } + + /* Information not avaliable */ + spdef->allocation = 0; + spdef->available = 0; + + spdef->source.ndevice = 1; + + /*XXX source adapter not working properly, should show hdiskX */ + if ((spdef->source.adapter = + phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { + VIR_ERROR("%s", + "Unable to determine storage pools's source adapter."); + goto err; + } + + if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { + VIR_ERROR("%s", "Error parsing volume XML."); + goto err; + } + + /* checking if this name already exists on this system */ + if (phypVolumeLookupByName(pool, voldef->name) != NULL) { + VIR_ERROR("%s", "StoragePool name already exists."); + goto err; + } + + /* The key must be NULL, the Power Hypervisor creates a key + * in the moment you create the volume. + * */ + if (voldef->key) { + VIR_ERROR("%s", + "Key must be empty, Power Hypervisor will create one for you."); + goto err; + } + + if (voldef->capacity) { + VIR_ERROR("%s", "Capacity cannot be empty."); + goto err; + } + + if (phypBuildVolume + (pool->conn, voldef->name, spdef->name, voldef->capacity, + key) == -1) + goto err; + + if ((vol = + virGetStorageVol(pool->conn, pool->name, voldef->name, + key)) == NULL) + goto err; + + return vol; + + err: + virStorageVolDefFree(voldef); + virStoragePoolDefFree(spdef); + if (vol) + virUnrefStorageVol(vol); + return NULL; +} + +static char * +phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) +{ + virConnectPtr conn = vol->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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lssp -detail -sp %s -field pvname'", sp); + } else { + virBufferVSprintf(&buf, "lssp -detail -sp %s -field pvname", sp); + } + virBufferVSprintf(&buf, "|sed 1d"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; + +} + +virStorageVolPtr +phypVolumeLookupByPath(virConnectPtr conn, const char *volname) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *spname = NULL; + char *key = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field vgname'", volname); + } else { + virBufferVSprintf(&buf, "lslv %s -field vgname", volname); + } + virBufferVSprintf(&buf, + "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + spname = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || spname == NULL) + return NULL; + + char *char_ptr = strchr(spname, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (VIR_ALLOC_N(key, PATH_MAX) < 0) { + virReportOOMError(); + return NULL; + } + + if (phypVolumeGetKey(conn, key, volname) == -1) + return NULL; + + return virGetStorageVol(conn, spname, volname, key); +} + +char * +phypVolumeGetXMLDesc(virStorageVolPtr vol, + unsigned int flags ATTRIBUTE_UNUSED) +{ + virStorageVolDef voldef; + memset(&voldef, 0, sizeof(virStorageVolDef)); + + virStoragePoolPtr sp = + phypStoragePoolLookupByName(vol->conn, vol->pool); + + if (!sp) + goto err; + + virStoragePoolDef pool; + memset(&pool, 0, sizeof(virStoragePoolDef)); + + if (VIR_ALLOC_N(voldef.key, PATH_MAX) < 0) { + virReportOOMError(); + return NULL; + } + + if (sp->name != NULL) { + pool.name = sp->name; + } else { + VIR_ERROR("%s", "Unable to determine storage sp's name."); + goto err; + } + + if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR("%s", "Unable to determine storage sp's uuid."); + goto err; + } + + if ((pool.capacity = phypGetStoragePoolSize(sp->conn, sp->name)) == -1) { + VIR_ERROR("%s", "Unable to determine storage sps's size."); + goto err; + } + + /* Information not avaliable */ + pool.allocation = 0; + pool.available = 0; + + pool.source.ndevice = 1; + + if ((pool.source.adapter = + phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { + VIR_ERROR("%s", + "Unable to determine storage sps's source adapter."); + goto err; + } + + if (vol->name != NULL) + voldef.name = vol->name; + else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(voldef.key, vol->key, PATH_MAX) == NULL) { + VIR_ERROR("%s", "Unable to determine volume's key."); + goto err; + } + + voldef.type = VIR_STORAGE_POOL_LOGICAL; + + return virStorageVolDefFormat(&pool, &voldef); + + err: + return NULL; +} + +/* The Volume Group path here will be treated as suggested in the + * email on the libvirt mailling list. As soon as I can't get the + * path for every volume, the path will be a representation in + * the form: + * + * /physical_volume/storage_pool/logical_volume + * + * */ +char * +phypVolumeGetPath(virStorageVolPtr vol) +{ + virConnectPtr conn = vol->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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *sp = NULL; + char *path = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field vgname'", vol->name); + } else { + virBufferVSprintf(&buf, "lslv %s -field vgname", vol->name); + } + virBufferVSprintf(&buf, + "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + sp = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || sp == NULL) + goto err; + + char *char_ptr = strchr(sp, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + char *pv = phypVolumeGetPhysicalVolumeByStoragePool(vol, sp); + + if (pv) { + if (virAsprintf(&path, "/%s/%s/%s", pv, sp, vol->name) < 0) { + virReportOOMError(); + goto err; + } + } else { + goto err; + } + + VIR_FREE(cmd); + return path; + + err: + VIR_FREE(cmd); + VIR_FREE(sp); + VIR_FREE(path); + return NULL; + +} + +virStorageVolPtr +phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname) +{ + + char key[PATH_MAX]; + + if (phypVolumeGetKey(pool->conn, key, volname) == -1) + return NULL; + + return virGetStorageVol(pool->conn, pool->name, volname, key); +} + +int +phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, + int nvolumes) +{ + virConnectPtr conn = pool->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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int got = 0; + int i; + char *cmd = NULL; + char *ret = NULL; + char *volumes_list = NULL; + char *char_ptr2 = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsvg -lv %s -field lvname'", pool->name); + } else { + virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + /* I need to parse the textual return in order to get the volumes */ + if (exit_status < 0 || ret == NULL) + goto err; + else { + volumes_list = ret; + + while (got < nvolumes) { + char_ptr2 = strchr(volumes_list, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((volumes[got++] = strdup(volumes_list)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + volumes_list = char_ptr2; + } else + break; + } + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return got; + + err: + for (i = 0; i < got; i++) + VIR_FREE(volumes[i]); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +int +phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) +{ + virConnectPtr conn = pool->conn; + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int nvolumes = 0; + char *cmd = NULL; + char *ret = NULL; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + char *char_ptr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsvg -lv %s -field lvname'", pool->name); + } else { + virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'|grep -c '^.*$'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return nvolumes; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +int +phypDestroyStoragePool(virStoragePoolPtr pool) +{ + virConnectPtr conn = pool->conn; + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int vios_id = phyp_driver->vios_id; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + char *cmd = NULL; + char *ret = NULL; + int exit_status = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'rmsp %s'", pool->name); + } else { + virBufferVSprintf(&buf, "'rmsp %s'", pool->name); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + if (virAsprintf(&cmd, + "viosvrcmd -m %s --id %d -c " + "'rmsp %s'", managed_system, vios_id, + pool->name) < 0) { + virReportOOMError(); + goto err; + } + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", + ret); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virStoragePoolSource source = def->source; + int vios_id = phyp_driver->vios_id; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + char *cmd = NULL; + char *ret = NULL; + int exit_status = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'mksp -f %schild %s'", def->name, + source.adapter); + } else { + virBufferVSprintf(&buf, "mksp -f %schild %s", def->name, + source.adapter); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", + ret); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +virStoragePoolPtr +phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + + virStoragePoolDefPtr def = NULL; + virStoragePoolPtr sp = NULL; + + if (!(def = virStoragePoolDefParseString(xml))) + goto err; + + /* checking if this name already exists on this system */ + if (phypStoragePoolLookupByName(conn, def->name) != NULL) { + VIR_WARN0("StoragePool name already exists."); + goto err; + } + + /* checking if ID or UUID already exists on this system */ + if (phypGetStoragePoolLookUpByUUID(conn, def->uuid) != NULL) { + VIR_WARN0("StoragePool uuid already exists."); + goto err; + } + + if ((sp = virGetStoragePool(conn, def->name, def->uuid)) == NULL) + goto err; + + if (phypBuildStoragePool(conn, def) == -1) + goto err; + + return sp; + + err: + virStoragePoolDefFree(def); + if (sp) + virUnrefStoragePool(sp); + return NULL; +} + +char * +phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, + unsigned int flags ATTRIBUTE_UNUSED) +{ + virStoragePoolDef def; + memset(&def, 0, sizeof(virStoragePoolDef)); + + if (pool->name != NULL) + def.name = pool->name; + else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(def.uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR("%s", "Unable to determine storage pool's uuid."); + goto err; + } + + if ((def.capacity = + phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { + VIR_ERROR("%s", "Unable to determine storage pools's size."); + goto err; + } + + /* Information not avaliable */ + def.allocation = 0; + def.available = 0; + + def.source.ndevice = 1; + + /*XXX source adapter not working properly, should show hdiskX */ + if ((def.source.adapter = + phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { + VIR_ERROR("%s", + "Unable to determine storage pools's source adapter."); + goto err; + } + + return virStoragePoolDefFormat(&def); + + err: + return NULL; +} + +static int +phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, + const char *name) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsdev -dev %s -attr vgserial_id'", name); + } else { + virBufferVSprintf(&buf, "lsdev -dev %s -attr vgserial_id", name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (memmove(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; +} + +virStoragePoolPtr +phypGetStoragePoolLookUpByUUID(virConnectPtr conn, + const unsigned char *uuid) +{ + virStoragePoolPtr sp = NULL; + int npools = 0; + int gotpools = 0; + char **pools = NULL; + unsigned int i = 0; + unsigned char *local_uuid = NULL; + + if (VIR_ALLOC_N(local_uuid, VIR_UUID_BUFLEN) < 0) { + virReportOOMError(); + goto err; + } + + if ((npools = phypNumOfStoragePools(conn)) == -1) { + virReportOOMError(); + goto err; + } + + if (VIR_ALLOC_N(pools, npools) < 0) { + virReportOOMError(); + goto err; + } + + if ((gotpools = phypListStoragePools(conn, pools, npools)) == -1) { + virReportOOMError(); + goto err; + } + + if (gotpools != npools) { + virReportOOMError(); + goto err; + } + + for (i = 0; i < gotpools; i++) { + if (phypGetStoragePoolUUID(conn, local_uuid, pools[i]) == -1) + continue; + + if (STREQLEN((char *) local_uuid, (char *) uuid, VIR_UUID_BUFLEN)) { + sp = virGetStoragePool(conn, pools[i], uuid); + VIR_FREE(local_uuid); + VIR_FREE(pools); + + if (sp) + return sp; + else + goto err; + } + } + + err: + VIR_FREE(local_uuid); + VIR_FREE(pools); + return NULL; +} + +virStoragePoolPtr +phypStoragePoolLookupByName(virConnectPtr conn, const char *name) +{ + unsigned char uuid[VIR_UUID_BUFLEN]; + + if (phypGetStoragePoolUUID(conn, uuid, name) == -1) + return NULL; + + return virGetStoragePool(conn, name, uuid); +} + +int +phypNumOfStoragePools(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int nsp = 0; + char *cmd = NULL; + char *ret = NULL; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + char *char_ptr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsvg'"); + } else { + virBufferVSprintf(&buf, "lsvg"); + } + virBufferVSprintf(&buf, "grep -c '^.*$'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + 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; + + 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) +{ + 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 system_type = phyp_driver->system_type; + 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; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsvg'"); + } else { + virBufferVSprintf(&buf, "lsvg"); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + 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) +{ + return VIR_DRV_OPEN_SUCCESS; +} + +int +phypStorageClose(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 0; +} + int phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) { @@ -2360,6 +3990,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 80ff0c3..2606fe4 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -75,6 +75,58 @@ struct _phyp_driver { char *managed_system; }; + +/* + * Storage functions + * */ +virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xmldesc, + unsigned int flags ATTRIBUTE_UNUSED); + +virStorageVolPtr phypVolumeLookupByPath (virConnectPtr pool, const char *path); + +char *phypVolumeGetXMLDesc(virStorageVolPtr vol, + unsigned int flags ATTRIBUTE_UNUSED); + +char *phypVolumeGetPath(virStorageVolPtr vol); + +virStorageVolPtr phypVolumeLookupByName(virStoragePoolPtr pool, + const char *name); + +int phypStoragePoolListVolumes(virStoragePoolPtr pool, + char **const volumes, int maxvolumes); + +int phypStoragePoolNumOfVolumes(virStoragePoolPtr pool); + +int phypDestroyStoragePool(virStoragePoolPtr pool); + +virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags + ATTRIBUTE_UNUSED); + +int phypNumOfStoragePools(virConnectPtr conn); + +int phypListStoragePools(virConnectPtr conn, char **const pools, + int npools); + +virStoragePoolPtr phypStoragePoolLookupByName(virConnectPtr conn, const char *name); + +virStoragePoolPtr phypGetStoragePoolLookUpByUUID(virConnectPtr conn, const unsigned char *uuid); + +char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags); + +virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED); + +int phypStorageClose(virConnectPtr conn); + +/* + * Driver functions + * */ +int phypAttachDevice(virDomainPtr domain, const char *xml); + int phypCheckSPFreeSapce(virConnectPtr conn, int required_size, char *sp); int phypGetSystemType(virConnectPtr conn); -- 1.7.0.4
On 06/18/2010 03:11 PM, Eduardo Otubo wrote:
Hello Folks,
This is the result of a couple of months of hard work. I added the storage management driver to the Power Hypervisor driver. This is a big but simple patch, it's just a new set of functions, nothing more. I could split it into multiple commits, but the feature freeze starts in some hours and I really reed this feature to be included in the next release.
This patch includes: * Storage driver: The set of pool-* and vol-* functions. * attach-disk function. * Support for IVM on the new functions.
I've been looking at this code for a long time, so I apologize now for the silly mistakes that might be present. Looking forward to see the comments.
Thanks!
Yes, it's quite involved, but it's all code addition restricted to just the phyp backend, so a single commit is acceptable to me. However, breaking it into smaller commits may help getting a particular commit through the review process faster (since most of your APIs don't depend on each other, it would be reasonable to break it into 3-4 driver additions per patch, rather than all 15 new driver callbacks in one go).
--- src/phyp/phyp_driver.c | 1638 +++++++++++++++++++++++++++++++++++++++++++++++- src/phyp/phyp_driver.h | 52 ++ 2 files changed, 1688 insertions(+), 2 deletions(-)
diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h
It's a shame that git lists files alphabetically, because I like to review the headers first. I've reordered my reply accordingly ;)
index 80ff0c3..2606fe4 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -75,6 +75,58 @@ struct _phyp_driver { char *managed_system; };
+ +/* + * Storage functions + * */ +virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xmldesc, + unsigned int flags ATTRIBUTE_UNUSED);
Do these new functions really need to be exported (by modifying src/libvirt_private.syms or some such)? I'd almost rather see them just declared private within phyp_driver.h, since the only time they will be invoked outside of phyp_driver.c is via the driver context. I only found two instances of the string phypStorageVolCreateXML in your patch - this declaration, and its implementation. But no one calls it, and you haven't yet registered it in a driver callback array. Did you forget to set .volCreateXML properly in your new virStorageDriver?
+virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags + ATTRIBUTE_UNUSED);
Technically, new code should probably not be using ATTRIBUTE_UNUSED on flags; more on that at the implementation.
+char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags);
Formatting nit: no space between the * and the function name.
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index cefb8be..77a74ef 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -56,6 +56,7 @@ #include "virterror_internal.h" #include "uuid.h" #include "domain_conf.h" +#include "storage_conf.h" #include "nodeinfo.h"
#include "phyp_driver.h" @@ -1680,6 +1681,466 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
}
+static char * +phypGetLparProfile(virConnectPtr conn, int lpar_id) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + " -r prof --filter lpar_ids=%d -F name|head -n 1", + lpar_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; +
Hmm. The 'head -n 1' you appended to cmd will guarantee that there will be at most one newline, and if present, it will be the last byte in the returned string. Meanwhile, some older systems didn't support 'head -n 1' (even though POSIX now requires it). How much more output does lssyscfg typically produce? In other words, are we saving ourselves some malloc overhead by trimming the stdout via head, or is it just simpler to avoid the portability hassle, and skip piping the results through head, since this strchr does the same thing anyway?
+static int +phypGetVIOSNextSlotNumber(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + virBufferAddLit(&buf, "echo $((`lssyscfg");
$(()) is another construct guaranteed by POSIX but not portable to all current /bin/sh. Are we assured that phyp can only run on systems with decent /bin/sh, or should we be worried about portability to Solaris (in which case, use expr instead of $(()).)
+ if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof --filter " + "profile_names=%s -F virtual_eth_adapters," + "virtual_opti_pool_id,virtual_scsi_adapters," + "virtual_serial_adapters|sed -e 's/\"//g' -e " + "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" + "|sort|tail -n 1` +1 ))", profile);
Ouch. If the result of the `` command substitution begins with 0, you have a problem with octal numbers. Remember, $((010 + 1)) is not the same as $((10 + 1)). Perhaps you can modify the sed commands used in your script to strip leading 0?
+ if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err;
And, rather than doing $(()) (or expr) to do +1 in the shell, where you have to worry about octal in the first place, why not just output the last value as-is (that is, drop "echo $((`" and "` +1 ))" from cmd), then do +1 in C code?
+static int +phypCreateServerSCSIAdapter(virConnectPtr conn) +{ ... + virBufferVSprintf(&buf, "-r prof --filter lpar_ids=%d,profile_names=%s" + " -F virtual_scsi_adapters|sed -e s/\"//g",
Won't work. In addition to escaping " for inclusion in a C string, you also need to quote it from unbalanced use in shell code. In other words, you want to execute ...|sed -e 's/"//g' which requires a C string of: "...|sed -e 's/\"//g'" or you want to execute ...|sed -e s/\"//g which requires a C string of: "...|sed -e s/\\\"//g"
+static char * +phypGetVIOSFreeSCSIAdapter(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsmap -all -field svsa backing -fmt ,'"); + } else { + virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt ,"); + } + virBufferVSprintf(&buf, "|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'");
Here, you can save a couple of processes, and avoid any portability problems with 'head -n 1' at the same time. ...|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g' says to exclude lines that contain ,. or ,*, then print the first remaining line with commas removed. But sed can do all of that: ...|sed -n '/,[^.*]/! { s/,//g p q }' Yes, the embedded newlines are required for strict POSIX compatibility. But, since this is not the critical path, and not everyone is a sed wizard, I'm okay with the 3-process approach instead of the do-it-all-in-one sed script.
+int +phypAttachDevice(virDomainPtr domain, const char *xml) +{ ... + if (! + (vios_name = + phypGetLparNAME(session, managed_system, vios_id, conn))) { + VIR_ERROR("%s", "Unable to get VIOS name");
Here, you could use VIR_ERROR0(str) instead of VIR_ERROR("%s", str), since your string has no % symbols. But you do need to translate the string (with _() markings), ...
+ if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'mkvdev -vdev %s -vadapter %s'", + dev->data.disk->src, scsi_adapter); + } else { + virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s", + dev->data.disk->src, scsi_adapter); + }
Here's some ideas for sharing more of the code (making it easier to adjust the command in just one place, if you find down the road that mkvdev needs an argument change): char *end = ""; if (system_type == HMC) { virBufferVSprintf(&buf, "viosvrcmd ... -c '", ...); end = "'"; } virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s%s", dev->data.disk->src, scsi_adapter, end); OR if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd ... -c '", ...); virBufferVSprintf(&buf, "mkvdev ... -vadapter %s", ... scsi_adapter); if (system_type == HMC) virBufferAddChar(&buf, '\'');
+ if (!(profile = phypGetLparProfile(conn, domain->id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + }
Another case of a missing string translation. How come 'make syntax-check' isn't catching it?
+ virBufferVSprintf(&buf, + "-r prof -i 'name=%s,lpar_id=%d," + "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", + domain->name, domain->id, ret, slot,
Since we're passing a lot of things through shell, are you sure that domain->name (and all the other strings that you substitute via %s) will never contain ", $, or any other character special to shell processing that would throw your cmd for a loop? I know we have various XML schema requirements, but I haven't checked whether a domain name is something that libvirt has already guaranteed to be restricted to a reasonable subset (alphanumeric, ., _, -) or if it allows absolute freedom in naming with consequences to everyone else dealing with arbitrary names.
+static int +phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) ... + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field lvid'", name); + } else { + virBufferVSprintf(&buf, "lslv %s -field lvid", name);
Another example where you could simplify to one "lslv ..." string with a little bit of refactoring.
+ } + virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memmove(key, ret, PATH_MAX) == NULL)
key and ret don't overlap (since ret was freshly malloc'd). Use memcpy instead for speed.
+static char * +phypGetStoragePoolDevice(virConnectPtr conn, char *name) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lssp -detail -sp %s -field name'", name); + } else { + virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'");
Two sed processes: ...|sed '1d'|sed -e 's/\\ //g' can be simplified to one: ...|sed '1d; s/\\ //g' Also, '\ ' is not a portable sed escape sequence. Did you mean to use the C string "s/\\\\ //g" for the shell line 's/\\ //g', in order to delete backslash-space sequences from the output? (multiple instances of this pattern in your patch)
+static int +phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, + unsigned int capacity, char *key) +{ ... + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret);
Missing translation, and unusual capititalization in the middle of the sentence. I'd just use: VIR_ERROR(_("unable to create volume: %s"), ret); (GNU Coding Standards demand starting with lower case; however libvirt is not a GNU project, so we aren't bound by that rule. Right now, we aren't very consistent yet on whether error messages start with upper or lower case, so although I use lower, I won't reject a patch that uses upper.)
+virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + + virStorageVolDefPtr voldef = NULL; + virStoragePoolDefPtr spdef = NULL; + virStorageVolPtr vol = NULL; + char *key = NULL;
I'm wary of any new code that declares flags with ATTRIBUTE_UNUSED - it means we aren't checking for unknown flags, which makes it harder to actually define a new flag down the road. To get around that, you should be using: virCheckFlags(0, NULL); which checks that flags is explicitly 0.
+ + if (VIR_ALLOC_N(key, PATH_MAX) < 0) { + virReportOOMError(); + return NULL; + }
Ouch. On GNU/Hurd, PATH_MAX is unlimited, so it is not a size that you can safely malloc. Is there a better bound for the maximum key size that you expect, even if that means adding #define MAX_KEY_SIZE (1024*4), or whatever value is a better reasonable limit? At any rate, there's a reason that 'git grep "ALLOC.*PATH_MAX"' returns no hits, so this needs adjusting.
+ + /* Filling spdef manually + * */ + if (pool->name != NULL) { + spdef->name = pool->name; + } else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) {
Again, spdef and pool don't overlap, so you should use memcpy().
+ if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { + VIR_ERROR("%s", "Error parsing volume XML."); + goto err; + } + + /* checking if this name already exists on this system */ + if (phypVolumeLookupByName(pool, voldef->name) != NULL) { + VIR_ERROR("%s", "StoragePool name already exists.");
More strings to mark for translation (I'll quit commenting on this for this round of review, although there's probably more instances of it throughout the patch).
+virStorageVolPtr +phypVolumeLookupByPath(virConnectPtr conn, const char *volname) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *spname = NULL; + char *key = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field vgname'", volname); + } else { + virBufferVSprintf(&buf, "lslv %s -field vgname", volname); + } + virBufferVSprintf(&buf, + "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'");
Another instance of a non-portable sed escape '\ '. But this time, I see enough context that it looks like you are trying to just delete spaces (and not backslash-space sequences). In which case, use either: shell: ...|sed -e s/\^VOLUME\ GROUP://g -e... C-string: "...|sed -e s/\\^VOLUME\\ GROUP://g -e..." or shell: ...|sed -e 's/^VOLUME GROUP://g' -e... C-string: "...|sed -e '/^VOLUME GROUP://g' -e..."
+char * +phypVolumeGetXMLDesc(virStorageVolPtr vol, + unsigned int flags ATTRIBUTE_UNUSED) +{ + virStorageVolDef voldef; + memset(&voldef, 0, sizeof(virStorageVolDef));
Another place for virCheckFlags(0, NULL)
+ + if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) {
memcpy
+ +virStorageVolPtr +phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname) +{ + + char key[PATH_MAX];
Stack-allocating PATH_MAX bytes is just as non-portable as malloc'ing that many bytes; but here, you've got precedence in existing libvirt source (ultimately, I mean to sweep the source code to clean that all up). Are you sure we don't have a better bound on the maximum key length? And actually, looking at src/datatypes.c, the const char *key argument is a bit of a misnomer; the docs call it uuid, and we DO have a fixed bound for UUID length, which is much smaller than PATH_MAX (we also have VIR_UUID_STRING_BUFLEN defined in libvirt.h).
+int +phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, + int nvolumes) +{ ... + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'");
Save a process: ...|sed '1d'|sed '1d' is equivalent to: ...|sed 2d
+int +phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) +{ ... + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'|grep -c '^.*$'");
Here, rather than using sed '1d' twice,...
+ + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) + goto err;
...just subtract 2 from nvolumes here in C code.
+int +phypDestroyStoragePool(virStoragePoolPtr pool) +{ ... + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'rmsp %s'", pool->name); + } else { + virBufferVSprintf(&buf, "'rmsp %s'", pool->name); + }
Looks like you typo'd the IVM line (left the '' in, so you will fail trying to call the command "rmsp name" rather than the intended command "rmsp" with argument "name"). Another reason why refactoring your code to share just a single rmsp C string will help avoid mistakes like this.
+virStoragePoolPtr +phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + + virStoragePoolDefPtr def = NULL; + virStoragePoolPtr sp = NULL;
virCheckFlags(0, NULL) (won't comment on it any more for this round of review...)
+virStoragePoolPtr +phypGetStoragePoolLookUpByUUID(virConnectPtr conn, + const unsigned char *uuid) +{ ... + if (STREQLEN((char *) local_uuid, (char *) uuid, VIR_UUID_BUFLEN)) {
Here, it is simpler to avoid the casts, by doing: if (!memcmp(local_uuid, uuid, VIR_UUID_BUFLEN)) { It also matches existing style in the rest of libvirt ('git grep "STREQLEN.*VIR_UUID"' vs. 'git grep -i "cmp.*VIR_UUID_BUFLEN"').
+int +phypNumOfStoragePools(virConnectPtr conn) +{ + } else { + virBufferVSprintf(&buf, "lsvg"); + } + virBufferVSprintf(&buf, "grep -c '^.*$'");
Missing a '|' before the grep.
+} +virDrvOpenStatus +phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED)
Add a newline between the closing } of one function and the start of another. Overall, my review was mainly focused on style and shell portability. I don't have access to a phyp setup at the moment, so I can't really test if your various command lines make sense (I'm assuming they do). Towards the end, I kept on spotting (and ignoring) the same issues again, so remember to do global searches when addressing a comment, rather than just at the place where I made the comment. But in general, this looks promising, and hopefully we can get things turned around fast enough to decide whether this is worth including in libvirt 0.8.2. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/19/2010 09:33 AM, Eric Blake wrote: <snip>
+ if (!(profile = phypGetLparProfile(conn, domain->id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + }
Another case of a missing string translation. How come 'make syntax-check' isn't catching it?
It also didn't catch some untranslated "yes" and "no" strings in virsh.c that were recently fixed (after being manually noticed). Guess that needs to be looked at if it's not just virsh that it's not working correctly on. :/ Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
Justin Clift wrote:
On 06/19/2010 09:33 AM, Eric Blake wrote: <snip>
+ if (!(profile = phypGetLparProfile(conn, domain->id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + }
Another case of a missing string translation. How come 'make syntax-check' isn't catching it?
It also didn't catch some untranslated "yes" and "no" strings in virsh.c that were recently fixed (after being manually noticed).
Guess that needs to be looked at if it's not just virsh that it's not working correctly on. :/
The sc_libvirt_unmarked_diagnostics rule uses heuristics to detect uses of error-printing functions -- for which we want translations -- with a literal string argument, but with *no* use of _("...") in the immediate vicinity. The heuristics fail when: - the unmarked string is too far from the function name, either preceding it, as in the case of yes/no in virsh.c, or after it, as in two or more lines after the function name. - there is already one marked string, but there is another that is not marked. - a new function of this type is added to libvirt, but its name is not added to the msg_gen_function list in cfg.mk. If the penalty for an unmarked string were higher, we would invest more in automating the check, but for now, manually checking for violations (while very tedious) will probably turn up more than a few.
index 80ff0c3..2606fe4 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -75,6 +75,58 @@ struct _phyp_driver { char *managed_system; };
+ +/* + * Storage functions + * */ +virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xmldesc, + unsigned int flags ATTRIBUTE_UNUSED);
Do these new functions really need to be exported (by modifying src/libvirt_private.syms or some such)? I'd almost rather see them just declared private within phyp_driver.h, since the only time they will be invoked outside of phyp_driver.c is via the driver context.
I only found two instances of the string phypStorageVolCreateXML in your patch - this declaration, and its implementation. But no one calls it, and you haven't yet registered it in a driver callback array. Did you forget to set .volCreateXML properly in your new virStorageDriver?
Yes, I just forgot to insert these functions to the callback array.
+virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags + ATTRIBUTE_UNUSED);
Technically, new code should probably not be using ATTRIBUTE_UNUSED on flags; more on that at the implementation.
As said further ahead in the email, I removed all the ATTRIBUTE_UNUSED and added the virCheckFlags(0, NULL);
+char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags);
Formatting nit: no space between the * and the function name.
The last commit on my local branch is to indent all the code with GNU Indent, I don't know what happened here, but I'll fix it.
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index cefb8be..77a74ef 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -56,6 +56,7 @@ #include "virterror_internal.h" #include "uuid.h" #include "domain_conf.h" +#include "storage_conf.h" #include "nodeinfo.h"
#include "phyp_driver.h" @@ -1680,6 +1681,466 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
}
+static char * +phypGetLparProfile(virConnectPtr conn, int lpar_id) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + " -r prof --filter lpar_ids=%d -F name|head -n 1", + lpar_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; +
Hmm. The 'head -n 1' you appended to cmd will guarantee that there will be at most one newline, and if present, it will be the last byte in the returned string. Meanwhile, some older systems didn't support 'head -n 1' (even though POSIX now requires it). How much more output does lssyscfg typically produce? In other words, are we saving ourselves some malloc overhead by trimming the stdout via head, or is it just simpler to avoid the portability hassle, and skip piping the results through head, since this strchr does the same thing anyway?
I run my tests on old versions of HMC, IVM (both run Bash) and VIOS (which run rksh). Both versions are compatible with "head -n 1", then newer versions should not have problem with this. Truth is, the reason for me to be using "head -n 1" is because a LPAR can have more than one profile (but at least one), and it doesn't matter which one I modify to change the configuration, since I actually do it. So I pick the first one just for a matter of simplicity. I could pick the second or third, but it could get me some more trouble on parsing.
+static int +phypGetVIOSNextSlotNumber(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + virBufferAddLit(&buf, "echo $((`lssyscfg");
$(()) is another construct guaranteed by POSIX but not portable to all current /bin/sh. Are we assured that phyp can only run on systems with decent /bin/sh, or should we be worried about portability to Solaris (in which case, use expr instead of $(()).)
Since this commands run only on HMC's and IVM's (which are IBM AIX) I don't need to worry about the portability to Solaris. I am not aware about any HMC or IVM implememted on Solaris or Linux.
+ if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof --filter " + "profile_names=%s -F virtual_eth_adapters," + "virtual_opti_pool_id,virtual_scsi_adapters," + "virtual_serial_adapters|sed -e 's/\"//g' -e " + "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" + "|sort|tail -n 1` +1 ))", profile);
Ouch. If the result of the `` command substitution begins with 0, you have a problem with octal numbers. Remember, $((010 + 1)) is not the same as $((10 + 1)). Perhaps you can modify the sed commands used in your script to strip leading 0?
Yes, I can strip the leading zero using sed, but I hardly believe that would be a such a return. But better fix this now than in the client screen. :)
+ if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&slot) == -1) + goto err;
And, rather than doing $(()) (or expr) to do +1 in the shell, where you have to worry about octal in the first place, why not just output the last value as-is (that is, drop "echo $((`" and "` +1 ))" from cmd), then do +1 in C code?
This is an option, but I really like to isolate the most I can on the shell side, returning the final value for the function. I've been keeping this pattern over all the code, so did the same here
+static int +phypCreateServerSCSIAdapter(virConnectPtr conn) +{ ... + virBufferVSprintf(&buf, "-r prof --filter lpar_ids=%d,profile_names=%s" + " -F virtual_scsi_adapters|sed -e s/\"//g",
Won't work. In addition to escaping " for inclusion in a C string, you also need to quote it from unbalanced use in shell code. In other words, you want to execute ...|sed -e 's/"//g' which requires a C string of: "...|sed -e 's/\"//g'"
or you want to execute ...|sed -e s/\"//g which requires a C string of: "...|sed -e s/\\\"//g"
Yes, you're right. Fixed.
+static char * +phypGetVIOSFreeSCSIAdapter(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsmap -all -field svsa backing -fmt ,'"); + } else { + virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt ,"); + } + virBufferVSprintf(&buf, "|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'");
Here, you can save a couple of processes, and avoid any portability problems with 'head -n 1' at the same time. ...|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g' says to exclude lines that contain ,. or ,*, then print the first remaining line with commas removed. But sed can do all of that: ...|sed -n '/,[^.*]/! { s/,//g p q }'
Yes, the embedded newlines are required for strict POSIX compatibility. But, since this is not the critical path, and not everyone is a sed wizard, I'm okay with the 3-process approach instead of the do-it-all-in-one sed script.
Acutally, I gotta check all the versions of shell in all the versions of HMC/VIOS and IVM to get it all in a better way.
+int +phypAttachDevice(virDomainPtr domain, const char *xml) +{ ... + if (! + (vios_name = + phypGetLparNAME(session, managed_system, vios_id, conn))) { + VIR_ERROR("%s", "Unable to get VIOS name");
Here, you could use VIR_ERROR0(str) instead of VIR_ERROR("%s", str), since your string has no % symbols. But you do need to translate the string (with _() markings),
Ok, fixed!
...
+ if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'mkvdev -vdev %s -vadapter %s'", + dev->data.disk->src, scsi_adapter); + } else { + virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s", + dev->data.disk->src, scsi_adapter); + }
Here's some ideas for sharing more of the code (making it easier to adjust the command in just one place, if you find down the road that mkvdev needs an argument change):
char *end = ""; if (system_type == HMC) { virBufferVSprintf(&buf, "viosvrcmd ... -c '", ...); end = "'"; } virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s%s", dev->data.disk->src, scsi_adapter, end);
OR
if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd ... -c '", ...); virBufferVSprintf(&buf, "mkvdev ... -vadapter %s", ... scsi_adapter); if (system_type == HMC) virBufferAddChar(&buf, '\'');
I spent some time thinking on how to optimize the calls like this one. I think I like the second one. Fixing all the commands like this.
+ if (!(profile = phypGetLparProfile(conn, domain->id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + }
Another case of a missing string translation. How come 'make syntax-check' isn't catching it?
+ virBufferVSprintf(&buf, + "-r prof -i 'name=%s,lpar_id=%d," + "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", + domain->name, domain->id, ret, slot,
Since we're passing a lot of things through shell, are you sure that domain->name (and all the other strings that you substitute via %s) will never contain ", $, or any other character special to shell processing that would throw your cmd for a loop? I know we have various XML schema requirements, but I haven't checked whether a domain name is something that libvirt has already guaranteed to be restricted to a reasonable subset (alphanumeric, ., _, -) or if it allows absolute freedom in naming with consequences to everyone else dealing with arbitrary names.
Yes, you're right. I used escape_special_characters() to scape some occasional trash.
+static int +phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) ... + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field lvid'", name); + } else { + virBufferVSprintf(&buf, "lslv %s -field lvid", name);
Another example where you could simplify to one "lslv ..." string with a little bit of refactoring.
Yes, I refactored all of this kind.
+ } + virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memmove(key, ret, PATH_MAX) == NULL)
key and ret don't overlap (since ret was freshly malloc'd). Use memcpy instead for speed.
Ok.
+static char * +phypGetStoragePoolDevice(virConnectPtr conn, char *name) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lssp -detail -sp %s -field name'", name); + } else { + virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name); + } + virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'");
Two sed processes: ...|sed '1d'|sed -e 's/\\ //g' can be simplified to one: ...|sed '1d; s/\\ //g'
Also, '\ ' is not a portable sed escape sequence. Did you mean to use the C string "s/\\\\ //g" for the shell line 's/\\ //g', in order to delete backslash-space sequences from the output? (multiple instances of this pattern in your patch)
No, I meant to use the C string 's/\\ //g' for the shell line 's/\ //g' in order to delete white spaces.
+static int +phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, + unsigned int capacity, char *key) +{ ... + if (exit_status< 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret);
Missing translation, and unusual capititalization in the middle of the sentence. I'd just use:
VIR_ERROR(_("unable to create volume: %s"), ret);
(GNU Coding Standards demand starting with lower case; however libvirt is not a GNU project, so we aren't bound by that rule. Right now, we aren't very consistent yet on whether error messages start with upper or lower case, so although I use lower, I won't reject a patch that uses upper.)
Ok.
+virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + + virStorageVolDefPtr voldef = NULL; + virStoragePoolDefPtr spdef = NULL; + virStorageVolPtr vol = NULL; + char *key = NULL;
I'm wary of any new code that declares flags with ATTRIBUTE_UNUSED - it means we aren't checking for unknown flags, which makes it harder to actually define a new flag down the road. To get around that, you should be using:
virCheckFlags(0, NULL);
I removed all the occurrences of ATTRIBUTE_UNUSED and added virCheckFlags(0, NULL);
which checks that flags is explicitly 0.
+ + if (VIR_ALLOC_N(key, PATH_MAX)< 0) { + virReportOOMError(); + return NULL; + }
Ouch. On GNU/Hurd, PATH_MAX is unlimited, so it is not a size that you can safely malloc. Is there a better bound for the maximum key size that you expect, even if that means adding #define MAX_KEY_SIZE (1024*4), or whatever value is a better reasonable limit? At any rate, there's a reason that 'git grep "ALLOC.*PATH_MAX"' returns no hits, so this needs adjusting.
I remember that somewhere in the past I saw this PATH_MAX in the libvirt, perhaps now you have optimized this value and git grep wont show anything. I defined in phyp_driver.h MAX_KEY_SIZE as being (1024*4), I believe this value is enough to handle the Power Storage keys.
+ + /* Filling spdef manually + * */ + if (pool->name != NULL) { + spdef->name = pool->name; + } else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) {
Again, spdef and pool don't overlap, so you should use memcpy().
Ok.
+ if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { + VIR_ERROR("%s", "Error parsing volume XML."); + goto err; + } + + /* checking if this name already exists on this system */ + if (phypVolumeLookupByName(pool, voldef->name) != NULL) { + VIR_ERROR("%s", "StoragePool name already exists.");
More strings to mark for translation (I'll quit commenting on this for this round of review, although there's probably more instances of it throughout the patch).
All fixed.
+virStorageVolPtr +phypVolumeLookupByPath(virConnectPtr conn, const char *volname) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *spname = NULL; + char *key = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lslv %s -field vgname'", volname); + } else { + virBufferVSprintf(&buf, "lslv %s -field vgname", volname); + } + virBufferVSprintf(&buf, + "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'");
Another instance of a non-portable sed escape '\ '. But this time, I see enough context that it looks like you are trying to just delete spaces (and not backslash-space sequences). In which case, use either: shell: ...|sed -e s/\^VOLUME\ GROUP://g -e... C-string: "...|sed -e s/\\^VOLUME\\ GROUP://g -e..." or shell: ...|sed -e 's/^VOLUME GROUP://g' -e... C-string: "...|sed -e '/^VOLUME GROUP://g' -e..."
I choose the first one, just get all calls in the same pattern.
+char * +phypVolumeGetXMLDesc(virStorageVolPtr vol, + unsigned int flags ATTRIBUTE_UNUSED) +{ + virStorageVolDef voldef; + memset(&voldef, 0, sizeof(virStorageVolDef));
Another place for virCheckFlags(0, NULL)
+ + if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) {
memcpy
+ +virStorageVolPtr +phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname) +{ + + char key[PATH_MAX];
Stack-allocating PATH_MAX bytes is just as non-portable as malloc'ing that many bytes; but here, you've got precedence in existing libvirt source (ultimately, I mean to sweep the source code to clean that all up). Are you sure we don't have a better bound on the maximum key length? And actually, looking at src/datatypes.c, the const char *key argument is a bit of a misnomer; the docs call it uuid, and we DO have a fixed bound for UUID length, which is much smaller than PATH_MAX (we also have VIR_UUID_STRING_BUFLEN defined in libvirt.h).
As you told above, I replaced all PATH_MAX for MAX_KEY_SIZE
+int +phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, + int nvolumes) +{ ... + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'");
Save a process: ...|sed '1d'|sed '1d' is equivalent to: ...|sed 2d
This is an odd behaviour. On my bash, (Ubuntu 10.04), sed '2d' and sed '1d'|sed '1d' removes the second line of the stream. But the on the HMC, sed '1d'|sed '1' removes the first two lines of the stream and sed '2d' removes the second line. And what I need is to remove the first two lines, so keeping sed '1d'|sed '1d'.
+int +phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) +{ ... + virBufferVSprintf(&buf, "|sed '1d'|sed '1d'|grep -c '^.*$'");
Here, rather than using sed '1d' twice,...
+ + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn); + + if (exit_status< 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret,&char_ptr, 10,&nvolumes) == -1) + goto err;
...just subtract 2 from nvolumes here in C code.
Wise!
+int +phypDestroyStoragePool(virStoragePoolPtr pool) +{ ... + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'rmsp %s'", pool->name); + } else { + virBufferVSprintf(&buf, "'rmsp %s'", pool->name); + }
Looks like you typo'd the IVM line (left the '' in, so you will fail trying to call the command "rmsp name" rather than the intended command "rmsp" with argument "name"). Another reason why refactoring your code to share just a single rmsp C string will help avoid mistakes like this.
Fixed as you suggested up above.
+virStoragePoolPtr +phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + + virStoragePoolDefPtr def = NULL; + virStoragePoolPtr sp = NULL;
virCheckFlags(0, NULL) (won't comment on it any more for this round of review...)
+virStoragePoolPtr +phypGetStoragePoolLookUpByUUID(virConnectPtr conn, + const unsigned char *uuid) +{ ... + if (STREQLEN((char *) local_uuid, (char *) uuid, VIR_UUID_BUFLEN)) {
Here, it is simpler to avoid the casts, by doing:
if (!memcmp(local_uuid, uuid, VIR_UUID_BUFLEN)) {
It also matches existing style in the rest of libvirt ('git grep "STREQLEN.*VIR_UUID"' vs. 'git grep -i "cmp.*VIR_UUID_BUFLEN"').
Ok.
+int +phypNumOfStoragePools(virConnectPtr conn) +{ + } else { + virBufferVSprintf(&buf, "lsvg"); + } + virBufferVSprintf(&buf, "grep -c '^.*$'");
Missing a '|' before the grep.
Ok.
+} + +phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED)
Add a newline between the closing } of one function and the start of another.
Overall, my review was mainly focused on style and shell portability. I don't have access to a phyp setup at the moment, so I can't really test if your various command lines make sense (I'm assuming they do). Towards the end, I kept on spotting (and ignoring) the same issues again, so remember to do global searches when addressing a comment, rather than just at the place where I made the comment. But in general, this looks promising, and hopefully we can get things turned around fast enough to decide whether this is worth including in libvirt 0.8.2.
All the commands are tested and studied directly on the HMC/VIOS and IVM befores inserting on C code, so they really work :) All the issues you pointed were fixed in all worldwide code (not just the storage driver) Will send another patch right away after reading all the comments and making all the properly fixes. Thanks for all the comments! -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
On 06/21/2010 12:32 PM, Eduardo Otubo wrote:
Ouch. If the result of the `` command substitution begins with 0, you have a problem with octal numbers. Remember, $((010 + 1)) is not the same as $((10 + 1)). Perhaps you can modify the sed commands used in your script to strip leading 0?
Yes, I can strip the leading zero using sed, but I hardly believe that would be a such a return. But better fix this now than in the client screen. :)
And, rather than doing $(()) (or expr) to do +1 in the shell, where you have to worry about octal in the first place, why not just output the last value as-is (that is, drop "echo $((`" and "` +1 ))" from cmd), then do +1 in C code?
This is an option, but I really like to isolate the most I can on the shell side, returning the final value for the function. I've been keeping this pattern over all the code, so did the same here
I would much rather see it shift in the other direction - do as LITTLE work in shell as possible (since you have to carefully audit for exploits, and because it is SO expensive to fork processes), and as MUCH work in C as possible. Shortcuts like 'cmd | head -n 1' => C processing are one of the few things where doing more in shell can actually be faster, as it can lead to much less I/O, and even stop a cmd with lots of output early (due to EPIPE/SIGPIPE) rather than wasting processing power in running cmd to completion when we only need the first line in C code. But anything as complex as massaging octal strings into known binary is going to be orders of magnitude faster in C than in shell.
Also, '\ ' is not a portable sed escape sequence. Did you mean to use the C string "s/\\\\ //g" for the shell line 's/\\ //g', in order to delete backslash-space sequences from the output? (multiple instances of this pattern in your patch)
No, I meant to use the C string 's/\\ //g' for the shell line 's/\ //g' in order to delete white spaces.
But that's my point. Some versions of sed treat '\ ' as the single byte space, while others treat it as the two-byte sequence backslash-space. In short: sed 's/\ //g' _is not portable_. The only portable shell command lines for deleting spaces are: sed 's/ //g' sed s/\ //g That is, either quote the space using '' in shell, or quote the space using \ in shell, but do NOT escape the space for sed.
Save a process: ...|sed '1d'|sed '1d' is equivalent to: ...|sed 2d
This is an odd behaviour. On my bash, (Ubuntu 10.04), sed '2d' and sed '1d'|sed '1d' removes the second line of the stream. But the on the HMC, sed '1d'|sed '1' removes the first two lines of the stream and sed '2d' removes the second line. And what I need is to remove the first two lines, so keeping sed '1d'|sed '1d'.
My apologies. I mistyped the equivalence (easy to do with complicated sed scripts). ...|sed '1d'|sed '1d' is really equivalent to: ...|sed 1,2d (2d strips just the second line, but 1,2d strips the range between the first and second lines). Sorry for my confusion.
All the commands are tested and studied directly on the HMC/VIOS and IVM befores inserting on C code, so they really work :) All the issues you pointed were fixed in all worldwide code (not just the storage driver)
Will send another patch right away after reading all the comments and making all the properly fixes.
Looking forward to it.
Thanks for all the comments!
You're welcome. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/21/2010 05:05 PM, Eric Blake wrote:
On 06/21/2010 12:32 PM, Eduardo Otubo wrote:
Ouch. If the result of the `` command substitution begins with 0, you have a problem with octal numbers. Remember, $((010 + 1)) is not the same as $((10 + 1)). Perhaps you can modify the sed commands used in your script to strip leading 0?
Yes, I can strip the leading zero using sed, but I hardly believe that would be a such a return. But better fix this now than in the client screen. :)
And, rather than doing $(()) (or expr) to do +1 in the shell, where you have to worry about octal in the first place, why not just output the last value as-is (that is, drop "echo $((`" and "` +1 ))" from cmd), then do +1 in C code?
This is an option, but I really like to isolate the most I can on the shell side, returning the final value for the function. I've been keeping this pattern over all the code, so did the same here
I would much rather see it shift in the other direction - do as LITTLE work in shell as possible (since you have to carefully audit for exploits, and because it is SO expensive to fork processes), and as MUCH work in C as possible. Shortcuts like 'cmd | head -n 1' => C processing are one of the few things where doing more in shell can actually be faster, as it can lead to much less I/O, and even stop a cmd with lots of output early (due to EPIPE/SIGPIPE) rather than wasting processing power in running cmd to completion when we only need the first line in C code. But anything as complex as massaging octal strings into known binary is going to be orders of magnitude faster in C than in shell.
Yes, you're right on this point. I was just thinking on the easy side for the one who writes the code. Parsing strings in C is not exactly an easy job, mainly if you have all the facilities of a shell script on hand. This surely will be a separate patch in the future.
Also, '\ ' is not a portable sed escape sequence. Did you mean to use the C string "s/\\\\ //g" for the shell line 's/\\ //g', in order to delete backslash-space sequences from the output? (multiple instances of this pattern in your patch)
No, I meant to use the C string 's/\\ //g' for the shell line 's/\ //g' in order to delete white spaces.
But that's my point. Some versions of sed treat '\ ' as the single byte space, while others treat it as the two-byte sequence backslash-space. In short: sed 's/\ //g' _is not portable_. The only portable shell command lines for deleting spaces are: sed 's/ //g' sed s/\ //g
That is, either quote the space using '' in shell, or quote the space using \ in shell, but do NOT escape the space for sed.
Save a process: ...|sed '1d'|sed '1d' is equivalent to: ...|sed 2d
Ok, fixed. The next patch is right away. -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
All the comments from the previous email from Eric Blake are now fixed. Also fixed some styling by using indent on the whole file. Hope we can get this patch pushed to 0.8.2. Any additional comments are always welcome. []'s --- src/phyp/phyp_driver.c | 395 +++++++++++++++++++++++++++--------------------- src/phyp/phyp_driver.h | 23 ++-- 2 files changed, 239 insertions(+), 179 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 77a74ef..f39c8fb 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -656,7 +656,8 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, virBufferAddLit(&buf, "lshwres"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d", + virBufferVSprintf(&buf, + " -r mem --level lpar -F %s --filter lpar_ids=%d", type ? "curr_mem" : "curr_max_mem", lpar_id); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -722,7 +723,8 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, virBufferAddLit(&buf, "lshwres"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -r proc --level lpar -F %s --filter lpar_ids=%d", + virBufferVSprintf(&buf, + " -r proc --level lpar -F %s --filter lpar_ids=%d", type ? "curr_max_procs" : "curr_procs", lpar_id); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -773,8 +775,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F " - "remote_slot_num --filter lpar_names=%s", - lpar_name); + "remote_slot_num --filter lpar_names=%s", lpar_name); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -1067,7 +1068,8 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", state); + virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", + state); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -1141,7 +1143,8 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -F lpar_id,state %s | sed -e 's/,.*$//'", state); + virBufferVSprintf(&buf, " -F lpar_id,state %s | sed -e 's/,.*$//'", + state); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -1744,24 +1747,28 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) virBuffer buf = VIR_BUFFER_INITIALIZER; if (!(profile = phypGetLparProfile(conn, vios_id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name.")); goto err; } virBufferAddLit(&buf, "echo $((`lssyscfg"); + if (system_type == HMC) virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof --filter " "profile_names=%s -F virtual_eth_adapters," "virtual_opti_pool_id,virtual_scsi_adapters," "virtual_serial_adapters|sed -e 's/\"//g' -e " "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" - "|sort|tail -n 1` +1 ))", profile); + "|sort|tail -n 1|sed -s 's/^[0]//'` +1 ))", profile); + if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -1802,17 +1809,17 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) if (! (vios_name = phypGetLparNAME(session, managed_system, vios_id, conn))) { - VIR_ERROR("%s", "Unable to get VIOS name"); + VIR_ERROR0(_("Unable to get VIOS name")); goto err; } if (!(profile = phypGetLparProfile(conn, vios_id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name.")); goto err; } if ((slot = phypGetVIOSNextSlotNumber(conn)) == -1) { - VIR_ERROR("%s", "Unable to get free slot number"); + VIR_ERROR0(_("Unable to get free slot number")); goto err; } @@ -1823,7 +1830,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) if (system_type == HMC) virBufferVSprintf(&buf, " -m %s ", managed_system); virBufferVSprintf(&buf, "-r prof --filter lpar_ids=%d,profile_names=%s" - " -F virtual_scsi_adapters|sed -e s/\"//g", + " -F virtual_scsi_adapters|sed -e s/\\\"//g", vios_id, profile); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -1907,13 +1914,15 @@ phypGetVIOSFreeSCSIAdapter(virConnectPtr conn) char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lsmap -all -field svsa backing -fmt ,'"); - } else { - virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt ,"); - } + + virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt , "); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + virBufferVSprintf(&buf, "|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'"); if (virBufferError(&buf)) { @@ -1965,6 +1974,18 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virDomainDeviceDefPtr dev = NULL; virDomainDefPtr def = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + char *domain_name = NULL; + + if (VIR_ALLOC_N(domain_name, sizeof(domain->name)) < 0) { + virReportOOMError(); + goto err; + } + + if (escape_specialcharacters + (domain->name, domain_name, strlen(domain->name)) == -1) { + virReportOOMError(); + goto err; + } def->os.type = strdup("aix"); @@ -1983,7 +2004,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (! (vios_name = phypGetLparNAME(session, managed_system, vios_id, conn))) { - VIR_ERROR("%s", "Unable to get VIOS name"); + VIR_ERROR0(_("Unable to get VIOS name")); goto err; } @@ -1993,31 +2014,32 @@ phypAttachDevice(virDomainPtr domain, const char *xml) /* If not found, let's create one. * */ if (phypCreateServerSCSIAdapter(conn) == -1) { - VIR_ERROR("%s", "Unable to create new virtual adapter"); + VIR_ERROR0(_("Unable to create new virtual adapter")); goto err; } else { if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { - VIR_ERROR("%s", "Unable to create new virtual adapter"); + VIR_ERROR0(_("Unable to create new virtual adapter")); goto err; } } } - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'mkvdev -vdev %s -vadapter %s'", - dev->data.disk->src, scsi_adapter); - } else { - virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s", - dev->data.disk->src, scsi_adapter); - } + + virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s", + dev->data.disk->src, scsi_adapter); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2026,7 +2048,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) goto err; if (!(profile = phypGetLparProfile(conn, domain->id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name.")); goto err; } @@ -2084,7 +2106,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, "-r prof -i 'name=%s,lpar_id=%d," "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", - domain->name, domain->id, ret, slot, + domain_name, domain->id, ret, slot, vios_id, vios_name); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -2106,7 +2128,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " -m %s ", managed_system); virBufferVSprintf(&buf, " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", - domain->name, slot); + domain_name, slot); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -2271,7 +2293,7 @@ virStorageDriver phypStorageDriver = { .volLookupByName = phypVolumeLookupByName, .volLookupByKey = NULL, .volLookupByPath = phypVolumeLookupByPath, - .volCreateXML = NULL, + .volCreateXML = phypStorageVolCreateXML, .volCreateXMLFrom = NULL, .volDelete = NULL, .volGetInfo = NULL, @@ -2296,13 +2318,15 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lslv %s -field lvid'", name); - } else { - virBufferVSprintf(&buf, "lslv %s -field lvid", name); - } + + virBufferVSprintf(&buf, "lslv %s -field lvid", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'"); if (virBufferError(&buf)) { @@ -2310,6 +2334,7 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2322,7 +2347,7 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) if (char_ptr) *char_ptr = '\0'; - if (memmove(key, ret, PATH_MAX) == NULL) + if (memcpy(key, ret, MAX_KEY_SIZE) == NULL) goto err; VIR_FREE(cmd); @@ -2349,20 +2374,23 @@ phypGetStoragePoolDevice(virConnectPtr conn, char *name) char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lssp -detail -sp %s -field name'", name); - } else { - virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name); - } - virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'"); + + virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1d; s/\\ //g'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2400,20 +2428,23 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) char *char_ptr; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lssp -detail -sp %s -field size'", name); - } else { - virBufferVSprintf(&buf, "lssp -detail -sp %s -field size", name); - } - virBufferVSprintf(&buf, "|sed '1d'|sed -e 's/\\ //g'"); + + virBufferVSprintf(&buf, "lssp -detail -sp %s -field size", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1d; s/\\ //g'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2449,27 +2480,27 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'mklv -lv %s %s %d'", lvname, spname, - capacity); - } else { - virBufferVSprintf(&buf, "mklv -lv %s %s %d", lvname, spname, - capacity); - } + + virBufferVSprintf(&buf, "mklv -lv %s %s %d", lvname, spname, capacity); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret); + VIR_ERROR(_("unable to create Volume. Reason: %s"), ret); goto err; } @@ -2487,9 +2518,10 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, } virStorageVolPtr -phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, - unsigned int flags ATTRIBUTE_UNUSED) +phypStorageVolCreateXML(virStoragePoolPtr pool, + const char *xml, unsigned int flags) { + virCheckFlags(0, NULL); virStorageVolDefPtr voldef = NULL; virStoragePoolDefPtr spdef = NULL; @@ -2501,7 +2533,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, return NULL; } - if (VIR_ALLOC_N(key, PATH_MAX) < 0) { + if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) { virReportOOMError(); return NULL; } @@ -2511,18 +2543,18 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, if (pool->name != NULL) { spdef->name = pool->name; } else { - VIR_ERROR("%s", "Unable to determine storage pool's name."); + VIR_ERROR0(_("Unable to determine storage pool's name.")); goto err; } - if (memmove(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { - VIR_ERROR("%s", "Unable to determine storage pool's uuid."); + if (memcpy(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR0(_("Unable to determine storage pool's uuid.")); goto err; } if ((spdef->capacity = phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { - VIR_ERROR("%s", "Unable to determine storage pools's size."); + VIR_ERROR0(_("Unable to determine storage pools's size.")); goto err; } @@ -2535,19 +2567,19 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, /*XXX source adapter not working properly, should show hdiskX */ if ((spdef->source.adapter = phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { - VIR_ERROR("%s", - "Unable to determine storage pools's source adapter."); + VIR_ERROR0(_ + ("Unable to determine storage pools's source adapter.")); goto err; } if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { - VIR_ERROR("%s", "Error parsing volume XML."); + VIR_ERROR0(_("Error parsing volume XML.")); goto err; } /* checking if this name already exists on this system */ if (phypVolumeLookupByName(pool, voldef->name) != NULL) { - VIR_ERROR("%s", "StoragePool name already exists."); + VIR_ERROR0(_("StoragePool name already exists.")); goto err; } @@ -2555,13 +2587,13 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, * in the moment you create the volume. * */ if (voldef->key) { - VIR_ERROR("%s", - "Key must be empty, Power Hypervisor will create one for you."); + VIR_ERROR0(_ + ("Key must be empty, Power Hypervisor will create one for you.")); goto err; } if (voldef->capacity) { - VIR_ERROR("%s", "Capacity cannot be empty."); + VIR_ERROR0(_("Capacity cannot be empty.")); goto err; } @@ -2600,13 +2632,15 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lssp -detail -sp %s -field pvname'", sp); - } else { - virBufferVSprintf(&buf, "lssp -detail -sp %s -field pvname", sp); - } + + virBufferVSprintf(&buf, "lssp -detail -sp %s -field pvname", sp); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + virBufferVSprintf(&buf, "|sed 1d"); if (virBufferError(&buf)) { @@ -2614,6 +2648,7 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2651,21 +2686,23 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) char *key = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lslv %s -field vgname'", volname); - } else { - virBufferVSprintf(&buf, "lslv %s -field vgname", volname); - } - virBufferVSprintf(&buf, - "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'"); + + virBufferVSprintf(&buf, "lslv %s -field vgname", volname); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed -e 's/^VOLUME GROUP://g' -e 's/\\ //g'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); spname = phypExec(session, cmd, &exit_status, conn); @@ -2678,7 +2715,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) if (char_ptr) *char_ptr = '\0'; - if (VIR_ALLOC_N(key, PATH_MAX) < 0) { + if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) { virReportOOMError(); return NULL; } @@ -2690,9 +2727,10 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) } char * -phypVolumeGetXMLDesc(virStorageVolPtr vol, - unsigned int flags ATTRIBUTE_UNUSED) +phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) { + virCheckFlags(0, NULL); + virStorageVolDef voldef; memset(&voldef, 0, sizeof(virStorageVolDef)); @@ -2705,7 +2743,7 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, virStoragePoolDef pool; memset(&pool, 0, sizeof(virStoragePoolDef)); - if (VIR_ALLOC_N(voldef.key, PATH_MAX) < 0) { + if (VIR_ALLOC_N(voldef.key, MAX_KEY_SIZE) < 0) { virReportOOMError(); return NULL; } @@ -2713,17 +2751,17 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, if (sp->name != NULL) { pool.name = sp->name; } else { - VIR_ERROR("%s", "Unable to determine storage sp's name."); + VIR_ERROR0(_("Unable to determine storage sp's name.")); goto err; } if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) { - VIR_ERROR("%s", "Unable to determine storage sp's uuid."); + VIR_ERROR0(_("Unable to determine storage sp's uuid.")); goto err; } if ((pool.capacity = phypGetStoragePoolSize(sp->conn, sp->name)) == -1) { - VIR_ERROR("%s", "Unable to determine storage sps's size."); + VIR_ERROR0(_("Unable to determine storage sps's size.")); goto err; } @@ -2735,20 +2773,19 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, if ((pool.source.adapter = phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { - VIR_ERROR("%s", - "Unable to determine storage sps's source adapter."); + VIR_ERROR0(_("Unable to determine storage sps's source adapter.")); goto err; } if (vol->name != NULL) voldef.name = vol->name; else { - VIR_ERROR("%s", "Unable to determine storage pool's name."); + VIR_ERROR0(_("Unable to determine storage pool's name.")); goto err; } - if (memmove(voldef.key, vol->key, PATH_MAX) == NULL) { - VIR_ERROR("%s", "Unable to determine volume's key."); + if (memmove(voldef.key, vol->key, MAX_KEY_SIZE) == NULL) { + VIR_ERROR0(_("Unable to determine volume's key.")); goto err; } @@ -2784,13 +2821,15 @@ phypVolumeGetPath(virStorageVolPtr vol) char *path = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lslv %s -field vgname'", vol->name); - } else { - virBufferVSprintf(&buf, "lslv %s -field vgname", vol->name); - } + + virBufferVSprintf(&buf, "lslv %s -field vgname", vol->name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + virBufferVSprintf(&buf, "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'"); @@ -2799,6 +2838,7 @@ phypVolumeGetPath(virStorageVolPtr vol) virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); sp = phypExec(session, cmd, &exit_status, conn); @@ -2837,7 +2877,7 @@ virStorageVolPtr phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname) { - char key[PATH_MAX]; + char key[MAX_KEY_SIZE]; if (phypVolumeGetKey(pool->conn, key, volname) == -1) return NULL; @@ -2865,14 +2905,16 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, char *char_ptr2 = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lsvg -lv %s -field lvname'", pool->name); - } else { - virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); - } - virBufferVSprintf(&buf, "|sed '1d'|sed '1d'"); + + virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1,2d'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -2941,7 +2983,7 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) } else { virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); } - virBufferVSprintf(&buf, "|sed '1d'|sed '1d'|grep -c '^.*$'"); + virBufferVSprintf(&buf, "grep -c '^.*$'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -2958,6 +3000,9 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) goto err; + /* We need to remove 2 line from the header text output */ + nvolumes -= 2; + VIR_FREE(cmd); VIR_FREE(ret); return nvolumes; @@ -2983,19 +3028,21 @@ phypDestroyStoragePool(virStoragePoolPtr pool) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'rmsp %s'", pool->name); - } else { - virBufferVSprintf(&buf, "'rmsp %s'", pool->name); - } + + virBufferVSprintf(&buf, "rmsp %s", pool->name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); if (virAsprintf(&cmd, @@ -3009,8 +3056,7 @@ phypDestroyStoragePool(virStoragePoolPtr pool) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", - ret); + VIR_ERROR(_("Unable to create Storage Pool. Reason: %s"), ret); goto err; } @@ -3039,28 +3085,28 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'mksp -f %schild %s'", def->name, - source.adapter); - } else { - virBufferVSprintf(&buf, "mksp -f %schild %s", def->name, - source.adapter); - } + + virBufferVSprintf(&buf, "mksp -f %schild %s", def->name, + source.adapter); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", - ret); + VIR_ERROR(_("Unable to create Storage Pool. Reason: %s"), ret); goto err; } @@ -3077,9 +3123,9 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn, - const char *xml, - unsigned int flags ATTRIBUTE_UNUSED) + const char *xml, unsigned int flags) { + virCheckFlags(0, NULL); virStoragePoolDefPtr def = NULL; virStoragePoolPtr sp = NULL; @@ -3115,31 +3161,32 @@ phypStoragePoolCreateXML(virConnectPtr conn, } char * -phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, - unsigned int flags ATTRIBUTE_UNUSED) +phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) { + virCheckFlags(0, NULL); + virStoragePoolDef def; memset(&def, 0, sizeof(virStoragePoolDef)); if (pool->name != NULL) def.name = pool->name; else { - VIR_ERROR("%s", "Unable to determine storage pool's name."); + VIR_ERROR0(_("Unable to determine storage pool's name.")); goto err; } if (memmove(def.uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { - VIR_ERROR("%s", "Unable to determine storage pool's uuid."); + VIR_ERROR0(_("Unable to determine storage pool's uuid.")); goto err; } if ((def.capacity = phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { - VIR_ERROR("%s", "Unable to determine storage pools's size."); + VIR_ERROR0(_("Unable to determine storage pools's size.")); goto err; } - /* Information not avaliable */ + /* Information not available */ def.allocation = 0; def.available = 0; @@ -3148,8 +3195,8 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, /*XXX source adapter not working properly, should show hdiskX */ if ((def.source.adapter = phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { - VIR_ERROR("%s", - "Unable to determine storage pools's source adapter."); + VIR_ERROR0(_ + ("Unable to determine storage pools's source adapter.")); goto err; } @@ -3174,20 +3221,23 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lsdev -dev %s -attr vgserial_id'", name); - } else { - virBufferVSprintf(&buf, "lsdev -dev %s -attr vgserial_id", name); - } - virBufferVSprintf(&buf, "|sed '1d'|sed '1d'"); + + virBufferVSprintf(&buf, "lsdev -dev %s -attr vgserial_id", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1,2d'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -3248,7 +3298,7 @@ phypGetStoragePoolLookUpByUUID(virConnectPtr conn, if (phypGetStoragePoolUUID(conn, local_uuid, pools[i]) == -1) continue; - if (STREQLEN((char *) local_uuid, (char *) uuid, VIR_UUID_BUFLEN)) { + if (!memcmp(local_uuid, uuid, VIR_UUID_BUFLEN)) { sp = virGetStoragePool(conn, pools[i], uuid); VIR_FREE(local_uuid); VIR_FREE(pools); @@ -3293,20 +3343,23 @@ phypNumOfStoragePools(virConnectPtr conn) char *char_ptr; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lsvg'"); - } else { - virBufferVSprintf(&buf, "lsvg"); - } - virBufferVSprintf(&buf, "grep -c '^.*$'"); + + virBufferVSprintf(&buf, "lsvg"); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|grep -c '^.*$'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -3345,19 +3398,21 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) char *char_ptr2 = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (system_type == HMC) { - virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); - virBufferVSprintf(&buf, "'lsvg'"); - } else { - virBufferVSprintf(&buf, "lsvg"); - } + + virBufferVSprintf(&buf, "lsvg"); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -3395,6 +3450,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) VIR_FREE(ret); return -1; } + virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -3428,7 +3484,8 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) virBufferVSprintf(&buf, " -r lpar -p %s -i min_mem=%d,desired_mem=%d," "max_mem=%d,desired_procs=%d,virtual_scsi_adapters=%s", def->name, (int) def->memory, (int) def->memory, - (int) def->maxmem, (int) def->vcpus, def->disks[0]->src); + (int) def->maxmem, (int) def->vcpus, + def->disks[0]->src); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -3755,7 +3812,7 @@ phypUUIDTable_Push(virConnectPtr conn) /* end of file */ break; } else { - VIR_ERROR(_("Failed to read from '%s'"), local_file); + VIR_ERROR(_("Failed to read from %s"), local_file); goto err; } } diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h index 2606fe4..c83ccd1 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -27,6 +27,7 @@ #include <config.h> #include <libssh2.h> +#define MAX_KEY_SIZE (1024*4) #define LPAR_EXEC_ERR -1 #define SSH_CONN_ERR -2 /* error while trying to connect to remote host */ #define SSH_CMD_ERR -3 /* error while trying to execute the remote cmd */ @@ -80,13 +81,13 @@ struct _phyp_driver { * Storage functions * */ virStorageVolPtr -phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xmldesc, - unsigned int flags ATTRIBUTE_UNUSED); +phypStorageVolCreateXML(virStoragePoolPtr pool, + const char *xml, unsigned int flags); -virStorageVolPtr phypVolumeLookupByPath (virConnectPtr pool, const char *path); +virStorageVolPtr phypVolumeLookupByPath(virConnectPtr pool, + const char *path); -char *phypVolumeGetXMLDesc(virStorageVolPtr vol, - unsigned int flags ATTRIBUTE_UNUSED); +char *phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags); char *phypVolumeGetPath(virStorageVolPtr vol); @@ -102,19 +103,21 @@ int phypDestroyStoragePool(virStoragePoolPtr pool); virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn, const char *xml, - unsigned int flags - ATTRIBUTE_UNUSED); + unsigned int flags); int phypNumOfStoragePools(virConnectPtr conn); int phypListStoragePools(virConnectPtr conn, char **const pools, int npools); -virStoragePoolPtr phypStoragePoolLookupByName(virConnectPtr conn, const char *name); +virStoragePoolPtr phypStoragePoolLookupByName(virConnectPtr conn, + const char *name); -virStoragePoolPtr phypGetStoragePoolLookUpByUUID(virConnectPtr conn, const unsigned char *uuid); +virStoragePoolPtr phypGetStoragePoolLookUpByUUID(virConnectPtr conn, const unsigned char + *uuid); -char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags); +char *phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, + unsigned int flags); virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, virConnectAuthPtr auth ATTRIBUTE_UNUSED, -- 1.7.0.4
On 06/21/2010 03:37 PM, Eduardo Otubo wrote:
All the comments from the previous email from Eric Blake are now fixed. Also fixed some styling by using indent on the whole file. Hope we can get this patch pushed to 0.8.2.
Any additional comments are always welcome.
How hard would it be for you to separate styling changes from content changes into two different commits? That is, it would make your patch a little easier to review if the first stage were fixing indentation issues in existing code, with no semantic changes, and the second change is limited to semantic changes only, instead of the current mix of two types of changes in one patch.
- virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d", + virBufferVSprintf(&buf, + " -r mem --level lpar -F %s --filter lpar_ids=%d", type ? "curr_mem" : "curr_max_mem", lpar_id);
For example, this was an indentation-only change.
@@ -1744,24 +1747,28 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) virBuffer buf = VIR_BUFFER_INITIALIZER;
if (!(profile = phypGetLparProfile(conn, vios_id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name."));
This is not an indentation change, but it is fixing a pre-existing area of the code, so it would still fit better as a separate cleanup patch, apart from the real meat.
virBufferVSprintf(&buf, "-r prof --filter " "profile_names=%s -F virtual_eth_adapters," "virtual_opti_pool_id,virtual_scsi_adapters," "virtual_serial_adapters|sed -e 's/\"//g' -e " "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" - "|sort|tail -n 1` +1 ))", profile); + "|sort|tail -n 1|sed -s 's/^[0]//'` +1 ))", profile);
Here's the first real meat I found in the patch (70 lines in!), but it still has an issue - you are only stripping one, instead of all, leading zeroes. I still think that doing the conversion to int, then the +1 operation, in C, will be much better than trying to do it in shell with $(()). My quick glance at spot checks in the rest of the file did see that you are making progress; you did incorporate what looks like quite a few of my comments. However, I did not complete my review this time around because of the mix of multiple changes in one patch. On the other hand, I agree that this still seems like something worth getting in 0.8.2 if we can clean it up fast enough. Do you need any help separating the patch into separate stages? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
virBufferVSprintf(&buf, "-r prof --filter " "profile_names=%s -F virtual_eth_adapters," "virtual_opti_pool_id,virtual_scsi_adapters," "virtual_serial_adapters|sed -e 's/\"//g' -e " "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" - "|sort|tail -n 1` +1 ))", profile); + "|sort|tail -n 1|sed -s 's/^[0]//'` +1 ))", profile);
Here's the first real meat I found in the patch (70 lines in!), but it still has an issue - you are only stripping one, instead of all, leading zeroes. I still think that doing the conversion to int, then the +1 operation, in C, will be much better than trying to do it in shell with $(()).
Ok I'll fix that.
My quick glance at spot checks in the rest of the file did see that you are making progress; you did incorporate what looks like quite a few of my comments. However, I did not complete my review this time around because of the mix of multiple changes in one patch.
On the other hand, I agree that this still seems like something worth getting in 0.8.2 if we can clean it up fast enough. Do you need any help separating the patch into separate stages?
I'll separate the identation from the semantic code right away. Thanks for the suggestion :) -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
This is the first patch, just over the semantic changes over the code itself. Added the whole storage management stack and fixed the echo $(( expr)) to just increment the variable in the return of the function. Hope we can get it acknowledged in time for the next release :) Thanks for all the comments so far! --- src/phyp/phyp_driver.c | 1683 +++++++++++++++++++++++++++++++++++++++++++++++- src/phyp/phyp_driver.h | 51 ++ 2 files changed, 1731 insertions(+), 3 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index cefb8be..51bab2c 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -56,6 +56,7 @@ #include "virterror_internal.h" #include "uuid.h" #include "domain_conf.h" +#include "storage_conf.h" #include "nodeinfo.h" #include "phyp_driver.h" @@ -1680,6 +1681,484 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus) } +static char * +phypGetLparProfile(virConnectPtr conn, int lpar_id) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + " -r prof --filter lpar_ids=%d -F name|head -n 1", + lpar_id); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static int +phypGetVIOSNextSlotNumber(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + virBufferAddLit(&buf, "lssyscfg"); + + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + + virBufferVSprintf(&buf, "-r prof --filter " + "profile_names=%s -F virtual_eth_adapters," + "virtual_opti_pool_id,virtual_scsi_adapters," + "virtual_serial_adapters|sed -e 's/\"//g' -e " + "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" + "|sort|tail -n 1", profile); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return ++slot; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypCreateServerSCSIAdapter(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + char *vios_name = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (! + (vios_name = + phypGetLparNAME(session, managed_system, vios_id, conn))) { + VIR_ERROR("%s", "Unable to get VIOS name"); + goto err; + } + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + if ((slot = phypGetVIOSNextSlotNumber(conn)) == -1) { + VIR_ERROR("%s", "Unable to get free slot number"); + goto err; + } + + /* Listing all the virtual_scsi_adapter interfaces, the new adapter must + * be appended to this list + * */ + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof --filter lpar_ids=%d,profile_names=%s" + " -F virtual_scsi_adapters|sed -e s/\\\"//g", + vios_id, profile); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + /* Here I change the VIOS configuration to append the new adapter + * with the free slot I got with phypGetVIOSNextSlotNumber. + * */ + virBufferAddLit(&buf, "chsyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, "-r prof -i 'name=%s,lpar_id=%d," + "\"virtual_scsi_adapters=%s,%d/server/any/any/1\"'", + vios_name, vios_id, ret, slot); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + /* Finally I add the new scsi adapter to VIOS using the same slot + * I used in the VIOS configuration. + * */ + virBufferAddLit(&buf, "chhwres -r virtualio --rsubtype scsi "); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + "-p %s -o a -s %d -d 0 -a \"adapter_type=server\"", + vios_name, slot); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + VIR_FREE(profile); + VIR_FREE(vios_name); + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(profile); + VIR_FREE(vios_name); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static char * +phypGetVIOSFreeSCSIAdapter(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lsmap -all -field svsa backing -fmt , "); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|grep -v ',[^.*]'|head -n 1|sed -e 's/,//g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + + +int +phypAttachDevice(virDomainPtr domain, const char *xml) +{ + + virConnectPtr conn = domain->conn; + ConnectionData *connection_data = domain->conn->networkPrivateData; + phyp_driverPtr phyp_driver = domain->conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr = NULL; + char *cmd = NULL; + char *ret = NULL; + char *scsi_adapter = NULL; + int slot = 0; + char *vios_name = NULL; + char *profile = NULL; + virDomainDeviceDefPtr dev = NULL; + virDomainDefPtr def = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *domain_name = NULL; + + if (VIR_ALLOC_N(domain_name, sizeof(domain->name)) < 0) { + virReportOOMError(); + goto err; + } + + if (escape_specialcharacters + (domain->name, domain_name, strlen(domain->name)) == -1) { + virReportOOMError(); + goto err; + } + + def->os.type = strdup("aix"); + + if (def->os.type == NULL) { + virReportOOMError(); + goto err; + } + + dev = virDomainDeviceDefParse(phyp_driver->caps, def, xml, + VIR_DOMAIN_XML_INACTIVE); + if (!dev) { + virReportOOMError(); + goto err; + } + + if (! + (vios_name = + phypGetLparNAME(session, managed_system, vios_id, conn))) { + VIR_ERROR("%s", "Unable to get VIOS name"); + goto err; + } + + /* First, let's look for a free SCSI Adapter + * */ + if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { + /* If not found, let's create one. + * */ + if (phypCreateServerSCSIAdapter(conn) == -1) { + VIR_ERROR("%s", "Unable to create new virtual adapter"); + goto err; + } else { + if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { + VIR_ERROR("%s", "Unable to create new virtual adapter"); + goto err; + } + } + } + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "mkvdev -vdev %s -vadapter %s", + dev->data.disk->src, scsi_adapter); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (!(profile = phypGetLparProfile(conn, domain->id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name."); + goto err; + } + + /* Let's get the slot number for the adapter we just created + * */ + virBufferAddLit(&buf, "lshwres -r virtualio --rsubtype scsi "); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s", managed_system); + virBufferVSprintf(&buf, + "slot_num,backing_device|grep %s|cut -d, -f1", + dev->data.disk->src); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* Listing all the virtual_scsi_adapter interfaces, the new adapter must + * be appended to this list + * */ + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + "-r prof --filter lpar_ids=%d,profile_names=%s" + " -F virtual_scsi_adapters|sed -e s/\"//g", + vios_id, profile); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + /* Here I change the LPAR configuration to append the new adapter + * with the new slot we just created + * */ + virBufferAddLit(&buf, "chsyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + "-r prof -i 'name=%s,lpar_id=%d," + "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", + domain_name, domain->id, ret, slot, + vios_id, vios_name); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* Finally I add the new scsi adapter to VIOS using the same slot + * I used in the VIOS configuration. + * */ + virBufferAddLit(&buf, "chhwres -r virtualio --rsubtype scsi "); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + virBufferVSprintf(&buf, + " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", + domain_name, slot); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) { + VIR_ERROR0(_ + ("Possibly you don't have IBM Tools installed in your LPAR." + "Contact your support to enable this feature.")); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + VIR_FREE(def); + VIR_FREE(dev); + VIR_FREE(vios_name); + VIR_FREE(scsi_adapter); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + VIR_FREE(def); + VIR_FREE(dev); + VIR_FREE(vios_name); + VIR_FREE(scsi_adapter); + return -1; +} + virDriver phypDriver = { VIR_DRV_PHYP, "PHYP", phypOpen, /* open */ phypClose, /* close */ @@ -1725,7 +2204,7 @@ virDriver phypDriver = { NULL, /* domainCreateWithFlags */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ + phypAttachDevice, /* domainAttachDevice */ NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ NULL, /* domainDetachDeviceFlags */ @@ -1779,6 +2258,1199 @@ virDriver phypDriver = { NULL, /* domainSnapshotDelete */ }; +virStorageDriver phypStorageDriver = { + .name = "PHYP", + .open = phypStorageOpen, + .close = phypStorageClose, + + .numOfPools = phypNumOfStoragePools, + .listPools = phypListStoragePools, + .numOfDefinedPools = NULL, + .listDefinedPools = NULL, + .findPoolSources = NULL, + .poolLookupByName = phypStoragePoolLookupByName, + .poolLookupByUUID = phypGetStoragePoolLookUpByUUID, + .poolLookupByVolume = NULL, + .poolCreateXML = phypStoragePoolCreateXML, + .poolDefineXML = NULL, + .poolBuild = NULL, + .poolUndefine = NULL, + .poolCreate = NULL, + .poolDestroy = phypDestroyStoragePool, + .poolDelete = NULL, + .poolRefresh = NULL, + .poolGetInfo = NULL, + .poolGetXMLDesc = phypGetStoragePoolXMLDesc, + .poolGetAutostart = NULL, + .poolSetAutostart = NULL, + .poolNumOfVolumes = phypStoragePoolNumOfVolumes, + .poolListVolumes = phypStoragePoolListVolumes, + + .volLookupByName = phypVolumeLookupByName, + .volLookupByKey = NULL, + .volLookupByPath = phypVolumeLookupByPath, + .volCreateXML = phypStorageVolCreateXML, + .volCreateXMLFrom = NULL, + .volDelete = NULL, + .volGetInfo = NULL, + .volGetXMLDesc = phypVolumeGetXMLDesc, + .volGetPath = phypVolumeGetPath, + .poolIsActive = NULL, + .poolIsPersistent = NULL +}; + +static int +phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) +{ + + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lslv %s -field lvid", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memcpy(key, ret, MAX_KEY_SIZE) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static char * +phypGetStoragePoolDevice(virConnectPtr conn, char *name) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lssp -detail -sp %s -field name", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1d; s/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static unsigned long int +phypGetStoragePoolSize(virConnectPtr conn, char *name) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + int vios_id = phyp_driver->vios_id; + char *cmd = NULL; + char *ret = NULL; + int sp_size = 0; + char *char_ptr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lssp -detail -sp %s -field size", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1d; s/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return sp_size; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, + unsigned int capacity, char *key) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int vios_id = phyp_driver->vios_id; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + char *cmd = NULL; + char *ret = NULL; + int exit_status = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "mklv -lv %s %s %d", lvname, spname, capacity); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret); + goto err; + } + + if (phypVolumeGetKey(conn, key, lvname) == -1) + goto err;; + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, + const char *xml, unsigned int flags) +{ + virCheckFlags(0, NULL); + + virStorageVolDefPtr voldef = NULL; + virStoragePoolDefPtr spdef = NULL; + virStorageVolPtr vol = NULL; + char *key = NULL; + + if (VIR_ALLOC(spdef) < 0) { + virReportOOMError(); + return NULL; + } + + if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) { + virReportOOMError(); + return NULL; + } + + /* Filling spdef manually + * */ + if (pool->name != NULL) { + spdef->name = pool->name; + } else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memcpy(spdef->uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR0(_("Unable to determine storage pool's uuid.")); + goto err; + } + + if ((spdef->capacity = + phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { + VIR_ERROR("%s", "Unable to determine storage pools's size."); + goto err; + } + + /* Information not avaliable */ + spdef->allocation = 0; + spdef->available = 0; + + spdef->source.ndevice = 1; + + /*XXX source adapter not working properly, should show hdiskX */ + if ((spdef->source.adapter = + phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { + VIR_ERROR("%s", + "Unable to determine storage pools's source adapter."); + goto err; + } + + if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { + VIR_ERROR("%s", "Error parsing volume XML."); + goto err; + } + + /* checking if this name already exists on this system */ + if (phypVolumeLookupByName(pool, voldef->name) != NULL) { + VIR_ERROR("%s", "StoragePool name already exists."); + goto err; + } + + /* The key must be NULL, the Power Hypervisor creates a key + * in the moment you create the volume. + * */ + if (voldef->key) { + VIR_ERROR("%s", + "Key must be empty, Power Hypervisor will create one for you."); + goto err; + } + + if (voldef->capacity) { + VIR_ERROR("%s", "Capacity cannot be empty."); + goto err; + } + + if (phypBuildVolume + (pool->conn, voldef->name, spdef->name, voldef->capacity, + key) == -1) + goto err; + + if ((vol = + virGetStorageVol(pool->conn, pool->name, voldef->name, + key)) == NULL) + goto err; + + return vol; + + err: + virStorageVolDefFree(voldef); + virStoragePoolDefFree(spdef); + if (vol) + virUnrefStorageVol(vol); + return NULL; +} + +static char * +phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) +{ + virConnectPtr conn = vol->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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lssp -detail -sp %s -field pvname", sp); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed 1d"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char *char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + return ret; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; + +} + +virStorageVolPtr +phypVolumeLookupByPath(virConnectPtr conn, const char *volname) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *spname = NULL; + char *key = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lslv %s -field vgname", volname); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed -e 's/^VOLUME GROUP://g' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + spname = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || spname == NULL) + return NULL; + + char *char_ptr = strchr(spname, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) { + virReportOOMError(); + return NULL; + } + + if (phypVolumeGetKey(conn, key, volname) == -1) + return NULL; + + return virGetStorageVol(conn, spname, volname, key); +} + +char * +phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) +{ + virCheckFlags(0, NULL); + + virStorageVolDef voldef; + memset(&voldef, 0, sizeof(virStorageVolDef)); + + virStoragePoolPtr sp = + phypStoragePoolLookupByName(vol->conn, vol->pool); + + if (!sp) + goto err; + + virStoragePoolDef pool; + memset(&pool, 0, sizeof(virStoragePoolDef)); + + if (VIR_ALLOC_N(voldef.key, MAX_KEY_SIZE) < 0) { + virReportOOMError(); + return NULL; + } + + if (sp->name != NULL) { + pool.name = sp->name; + } else { + VIR_ERROR("%s", "Unable to determine storage sp's name."); + goto err; + } + + if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR("%s", "Unable to determine storage sp's uuid."); + goto err; + } + + if ((pool.capacity = phypGetStoragePoolSize(sp->conn, sp->name)) == -1) { + VIR_ERROR("%s", "Unable to determine storage sps's size."); + goto err; + } + + /* Information not avaliable */ + pool.allocation = 0; + pool.available = 0; + + pool.source.ndevice = 1; + + if ((pool.source.adapter = + phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { + VIR_ERROR("%s", + "Unable to determine storage sps's source adapter."); + goto err; + } + + if (vol->name != NULL) + voldef.name = vol->name; + else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(voldef.key, vol->key, PATH_MAX) == NULL) { + VIR_ERROR("%s", "Unable to determine volume's key."); + goto err; + } + + voldef.type = VIR_STORAGE_POOL_LOGICAL; + + return virStorageVolDefFormat(&pool, &voldef); + + err: + return NULL; +} + +/* The Volume Group path here will be treated as suggested in the + * email on the libvirt mailling list. As soon as I can't get the + * path for every volume, the path will be a representation in + * the form: + * + * /physical_volume/storage_pool/logical_volume + * + * */ +char * +phypVolumeGetPath(virStorageVolPtr vol) +{ + virConnectPtr conn = vol->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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *sp = NULL; + char *path = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lslv %s -field vgname", vol->name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, + "|sed -e 's/^VOLUME\\ GROUP://g' -e 's/\\ //g'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + sp = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || sp == NULL) + goto err; + + char *char_ptr = strchr(sp, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + char *pv = phypVolumeGetPhysicalVolumeByStoragePool(vol, sp); + + if (pv) { + if (virAsprintf(&path, "/%s/%s/%s", pv, sp, vol->name) < 0) { + virReportOOMError(); + goto err; + } + } else { + goto err; + } + + VIR_FREE(cmd); + return path; + + err: + VIR_FREE(cmd); + VIR_FREE(sp); + VIR_FREE(path); + return NULL; + +} + +virStorageVolPtr +phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname) +{ + + char key[MAX_KEY_SIZE]; + + if (phypVolumeGetKey(pool->conn, key, volname) == -1) + return NULL; + + return virGetStorageVol(pool->conn, pool->name, volname, key); +} + +int +phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, + int nvolumes) +{ + virConnectPtr conn = pool->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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + int got = 0; + int i; + char *cmd = NULL; + char *ret = NULL; + char *volumes_list = NULL; + char *char_ptr2 = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1,2d'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + /* I need to parse the textual return in order to get the volumes */ + if (exit_status < 0 || ret == NULL) + goto err; + else { + volumes_list = ret; + + while (got < nvolumes) { + char_ptr2 = strchr(volumes_list, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((volumes[got++] = strdup(volumes_list)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + volumes_list = char_ptr2; + } else + break; + } + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return got; + + err: + for (i = 0; i < got; i++) + VIR_FREE(volumes[i]); + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +int +phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) +{ + virConnectPtr conn = pool->conn; + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int nvolumes = 0; + char *cmd = NULL; + char *ret = NULL; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + char *char_ptr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsvg -lv %s -field lvname'", pool->name); + } else { + virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); + } + virBufferVSprintf(&buf, "grep -c '^.*$'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) + goto err; + + /* We need to remove 2 line from the header text output */ + nvolumes -= 2; + + VIR_FREE(cmd); + VIR_FREE(ret); + return nvolumes; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +int +phypDestroyStoragePool(virStoragePoolPtr pool) +{ + virConnectPtr conn = pool->conn; + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int vios_id = phyp_driver->vios_id; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + char *cmd = NULL; + char *ret = NULL; + int exit_status = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "rmsp %s", pool->name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + if (virAsprintf(&cmd, + "viosvrcmd -m %s --id %d -c " + "'rmsp %s'", managed_system, vios_id, + pool->name) < 0) { + virReportOOMError(); + goto err; + } + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", + ret); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} + +static int +phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virStoragePoolSource source = def->source; + int vios_id = phyp_driver->vios_id; + int system_type = phyp_driver->system_type; + char *managed_system = phyp_driver->managed_system; + char *cmd = NULL; + char *ret = NULL; + int exit_status = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "mksp -f %schild %s", def->name, + source.adapter); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", + ret); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; + +} + +virStoragePoolPtr +phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, unsigned int flags) +{ + virCheckFlags(0, NULL); + + virStoragePoolDefPtr def = NULL; + virStoragePoolPtr sp = NULL; + + if (!(def = virStoragePoolDefParseString(xml))) + goto err; + + /* checking if this name already exists on this system */ + if (phypStoragePoolLookupByName(conn, def->name) != NULL) { + VIR_WARN0("StoragePool name already exists."); + goto err; + } + + /* checking if ID or UUID already exists on this system */ + if (phypGetStoragePoolLookUpByUUID(conn, def->uuid) != NULL) { + VIR_WARN0("StoragePool uuid already exists."); + goto err; + } + + if ((sp = virGetStoragePool(conn, def->name, def->uuid)) == NULL) + goto err; + + if (phypBuildStoragePool(conn, def) == -1) + goto err; + + return sp; + + err: + virStoragePoolDefFree(def); + if (sp) + virUnrefStoragePool(sp); + return NULL; +} + +char * +phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) +{ + virCheckFlags(0, NULL); + + virStoragePoolDef def; + memset(&def, 0, sizeof(virStoragePoolDef)); + + if (pool->name != NULL) + def.name = pool->name; + else { + VIR_ERROR("%s", "Unable to determine storage pool's name."); + goto err; + } + + if (memmove(def.uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { + VIR_ERROR("%s", "Unable to determine storage pool's uuid."); + goto err; + } + + if ((def.capacity = + phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { + VIR_ERROR("%s", "Unable to determine storage pools's size."); + goto err; + } + + /* Information not avaliable */ + def.allocation = 0; + def.available = 0; + + def.source.ndevice = 1; + + /*XXX source adapter not working properly, should show hdiskX */ + if ((def.source.adapter = + phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { + VIR_ERROR("%s", + "Unable to determine storage pools's source adapter."); + goto err; + } + + return virStoragePoolDefFormat(&def); + + err: + return NULL; +} + +static int +phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, + const char *name) +{ + 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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lsdev -dev %s -attr vgserial_id", name); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|sed '1,2d'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (memmove(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; +} + +virStoragePoolPtr +phypGetStoragePoolLookUpByUUID(virConnectPtr conn, + const unsigned char *uuid) +{ + virStoragePoolPtr sp = NULL; + int npools = 0; + int gotpools = 0; + char **pools = NULL; + unsigned int i = 0; + unsigned char *local_uuid = NULL; + + if (VIR_ALLOC_N(local_uuid, VIR_UUID_BUFLEN) < 0) { + virReportOOMError(); + goto err; + } + + if ((npools = phypNumOfStoragePools(conn)) == -1) { + virReportOOMError(); + goto err; + } + + if (VIR_ALLOC_N(pools, npools) < 0) { + virReportOOMError(); + goto err; + } + + if ((gotpools = phypListStoragePools(conn, pools, npools)) == -1) { + virReportOOMError(); + goto err; + } + + if (gotpools != npools) { + virReportOOMError(); + goto err; + } + + for (i = 0; i < gotpools; i++) { + if (phypGetStoragePoolUUID(conn, local_uuid, pools[i]) == -1) + continue; + + if (!memcmp(local_uuid, uuid, VIR_UUID_BUFLEN)) { + sp = virGetStoragePool(conn, pools[i], uuid); + VIR_FREE(local_uuid); + VIR_FREE(pools); + + if (sp) + return sp; + else + goto err; + } + } + + err: + VIR_FREE(local_uuid); + VIR_FREE(pools); + return NULL; +} + +virStoragePoolPtr +phypStoragePoolLookupByName(virConnectPtr conn, const char *name) +{ + unsigned char uuid[VIR_UUID_BUFLEN]; + + if (phypGetStoragePoolUUID(conn, uuid, name) == -1) + return NULL; + + return virGetStoragePool(conn, name, uuid); +} + +int +phypNumOfStoragePools(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + int system_type = phyp_driver->system_type; + int exit_status = 0; + int nsp = 0; + char *cmd = NULL; + char *ret = NULL; + char *managed_system = phyp_driver->managed_system; + int vios_id = phyp_driver->vios_id; + char *char_ptr; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lsvg"); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + virBufferVSprintf(&buf, "|grep -c '^.*$'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + 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; + + 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) +{ + 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 system_type = phyp_driver->system_type; + 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; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (system_type == HMC) + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", + managed_system, vios_id); + + virBufferVSprintf(&buf, "lsvg"); + + if (system_type == HMC) + virBufferVSprintf(&buf, "\'"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + 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) +{ + return VIR_DRV_OPEN_SUCCESS; +} + +int +phypStorageClose(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 0; +} + int phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) { @@ -1798,7 +3470,8 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) virBufferVSprintf(&buf, " -r lpar -p %s -i min_mem=%d,desired_mem=%d," "max_mem=%d,desired_procs=%d,virtual_scsi_adapters=%s", def->name, (int) def->memory, (int) def->memory, - (int) def->maxmem, (int) def->vcpus, def->disks[0]->src); + (int) def->maxmem, (int) def->vcpus, + def->disks[0]->src); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -2360,6 +4033,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 80ff0c3..e738927 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -27,6 +27,7 @@ #include <config.h> #include <libssh2.h> +#define MAX_KEY_SIZE (1024*4) #define LPAR_EXEC_ERR -1 #define SSH_CONN_ERR -2 /* error while trying to connect to remote host */ #define SSH_CMD_ERR -3 /* error while trying to execute the remote cmd */ @@ -75,6 +76,56 @@ struct _phyp_driver { char *managed_system; }; + +/* + * Storage functions + * */ +virStorageVolPtr +phypStorageVolCreateXML(virStoragePoolPtr pool, + const char *xml, unsigned int flags); + +virStorageVolPtr phypVolumeLookupByPath (virConnectPtr pool, const char *path); + +char *phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags); + +char *phypVolumeGetPath(virStorageVolPtr vol); + +virStorageVolPtr phypVolumeLookupByName(virStoragePoolPtr pool, + const char *name); + +int phypStoragePoolListVolumes(virStoragePoolPtr pool, + char **const volumes, int maxvolumes); + +int phypStoragePoolNumOfVolumes(virStoragePoolPtr pool); + +int phypDestroyStoragePool(virStoragePoolPtr pool); + +virStoragePoolPtr phypStoragePoolCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags); + +int phypNumOfStoragePools(virConnectPtr conn); + +int phypListStoragePools(virConnectPtr conn, char **const pools, + int npools); + +virStoragePoolPtr phypStoragePoolLookupByName(virConnectPtr conn, const char *name); + +virStoragePoolPtr phypGetStoragePoolLookUpByUUID(virConnectPtr conn, const unsigned char *uuid); + +char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags); + +virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED); + +int phypStorageClose(virConnectPtr conn); + +/* + * Driver functions + * */ +int phypAttachDevice(virDomainPtr domain, const char *xml); + int phypCheckSPFreeSapce(virConnectPtr conn, int required_size, char *sp); int phypGetSystemType(virConnectPtr conn); -- 1.7.0.4
On 06/23/2010 04:49 PM, Eduardo Otubo wrote:
This is the first patch, just over the semantic changes over the code itself. Added the whole storage management stack and fixed the echo $(( expr)) to just increment the variable in the return of the function.
Hope we can get it acknowledged in time for the next release :) Thanks for all the comments so far!
+static char * +phypGetLparProfile(virConnectPtr conn, int lpar_id) +{ + 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 system_type = phyp_driver->system_type; + int exit_status = 0; + char *cmd = NULL; + char *ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "lssyscfg"); + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system);
I removed a trailing space here...
+ virBufferVSprintf(&buf, + " -r prof --filter lpar_ids=%d -F name|head -n 1",
...since this line always provides a space.
+static int +phypGetVIOSNextSlotNumber(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 system_type = phyp_driver->system_type; + int vios_id = phyp_driver->vios_id; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + char *ret = NULL; + char *profile = NULL; + int slot = 0; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!(profile = phypGetLparProfile(conn, vios_id))) { + VIR_ERROR("%s", "Unable to get VIOS profile name.");
New code should start clean, so I added _() here (basically, I squashed in all the formatting changes from your original 2/2 patch that were left out when rebasing your two patches to play 2/2 first into my commit stream). I had to add it in a lot of places, to silence 'make syntax-check'.
+ goto err; + } + + virBufferAddLit(&buf, "lssyscfg"); + + if (system_type == HMC) + virBufferVSprintf(&buf, " -m %s ", managed_system); + + virBufferVSprintf(&buf, "-r prof --filter "
Even worse, for the non-HMC case, you try to execute "lssyscfg-r prof ..." (there is no lssyscfg-r program, you meant lssyscfg with a first argument of -r), so I did a global search and replace to clean up all of these commands to have consistent spacing.
+ "profile_names=%s -F virtual_eth_adapters," + "virtual_opti_pool_id,virtual_scsi_adapters," + "virtual_serial_adapters|sed -e 's/\"//g' -e " + "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" + "|sort|tail -n 1", profile); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return ++slot;
I did s/++slot/slot + 1/ since there is no need to modify slot in place - it goes out of scope after this return.
+int +phypAttachDevice(virDomainPtr domain, const char *xml) +{
I saw no reason for this to be a public function, so I removed it (and many like it) from phyp_driver.h, and marked it static here. If some file outside of phyp_driver.c needs to use it in the future, we can worry about exporting it then (and proper exporting, even if it is an internal-use only symbol, probably involves touching some .syms files).
virDriver phypDriver = { VIR_DRV_PHYP, "PHYP", phypOpen, /* open */ phypClose, /* close */ @@ -1725,7 +2204,7 @@ virDriver phypDriver = { NULL, /* domainCreateWithFlags */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ + phypAttachDevice, /* domainAttachDevice */ NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ NULL, /* domainDetachDeviceFlags */ @@ -1779,6 +2258,1199 @@ virDriver phypDriver = { NULL, /* domainSnapshotDelete */ };
+virStorageDriver phypStorageDriver = {
I moved this (and phypDriver) later in the file, and marked them static, to match with the fact that I marked a lot of functions static as part of reducing scope. Then it got to be quite unwieldy, so I'm now focusing on preparing a patch series that tackles this project in a more incremental manner. I'll post them for review soon (it no longer looks enough like your original for me to feel justified with acking your version but pushing mine, so I have to wait for you to confirm that my refactoring works). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/23/2010 04:49 PM, Eduardo Otubo wrote:
+ virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/\\ //g'");
You still missed some of these instances of non-portable '\ ' escape to sed. I've cleaned it up in my refactoring.
+ virBufferVSprintf(&buf, "\'");
Minor optimization - it is rather more efficient to use virBufferAddChar(&buf, '\'') than virBufferVSprintf(&buf, "\'") - I've added that in my refactoring.
+ if (system_type == HMC) { + virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c ", + managed_system, vios_id); + virBufferVSprintf(&buf, "'lsvg -lv %s -field lvname'", pool->name); + } else { + virBufferVSprintf(&buf, "lsvg -lv %s -field lvname", pool->name); + } + virBufferVSprintf(&buf, "grep -c '^.*$'");
And this was still a disaster waiting to happen for non-HVMC (passing -field lvnamegrep to lsvg instead of piping lsvg -field name to grep). Are you sure you caught all of my review issues from the first time around? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This is just the indentation, style and cleanup patch. --- src/phyp/phyp_driver.c | 100 +++++++++++++++++++++++++++-------------------- src/phyp/phyp_driver.h | 13 ++++-- 2 files changed, 66 insertions(+), 47 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 51bab2c..7e39dfd 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -656,7 +656,8 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, virBufferAddLit(&buf, "lshwres"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d", + virBufferVSprintf(&buf, + " -r mem --level lpar -F %s --filter lpar_ids=%d", type ? "curr_mem" : "curr_max_mem", lpar_id); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -722,7 +723,8 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, virBufferAddLit(&buf, "lshwres"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -r proc --level lpar -F %s --filter lpar_ids=%d", + virBufferVSprintf(&buf, + " -r proc --level lpar -F %s --filter lpar_ids=%d", type ? "curr_max_procs" : "curr_procs", lpar_id); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -773,8 +775,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F " - "remote_slot_num --filter lpar_names=%s", - lpar_name); + "remote_slot_num --filter lpar_names=%s", lpar_name); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -1067,7 +1068,8 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", state); + virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", + state); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -1141,7 +1143,8 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -F lpar_id,state %s | sed -e 's/,.*$//'", state); + virBufferVSprintf(&buf, " -F lpar_id,state %s | sed -e 's/,.*$//'", + state); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); @@ -1744,7 +1747,7 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) virBuffer buf = VIR_BUFFER_INITIALIZER; if (!(profile = phypGetLparProfile(conn, vios_id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name.")); goto err; } @@ -1806,17 +1809,17 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) if (! (vios_name = phypGetLparNAME(session, managed_system, vios_id, conn))) { - VIR_ERROR("%s", "Unable to get VIOS name"); + VIR_ERROR0(_("Unable to get VIOS name")); goto err; } if (!(profile = phypGetLparProfile(conn, vios_id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name.")); goto err; } if ((slot = phypGetVIOSNextSlotNumber(conn)) == -1) { - VIR_ERROR("%s", "Unable to get free slot number"); + VIR_ERROR0(_("Unable to get free slot number")); goto err; } @@ -2001,7 +2004,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (! (vios_name = phypGetLparNAME(session, managed_system, vios_id, conn))) { - VIR_ERROR("%s", "Unable to get VIOS name"); + VIR_ERROR0(_("Unable to get VIOS name")); goto err; } @@ -2011,11 +2014,11 @@ phypAttachDevice(virDomainPtr domain, const char *xml) /* If not found, let's create one. * */ if (phypCreateServerSCSIAdapter(conn) == -1) { - VIR_ERROR("%s", "Unable to create new virtual adapter"); + VIR_ERROR0(_("Unable to create new virtual adapter")); goto err; } else { if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { - VIR_ERROR("%s", "Unable to create new virtual adapter"); + VIR_ERROR0(_("Unable to create new virtual adapter")); goto err; } } @@ -2036,6 +2039,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2044,7 +2048,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) goto err; if (!(profile = phypGetLparProfile(conn, domain->id))) { - VIR_ERROR("%s", "Unable to get VIOS profile name."); + VIR_ERROR0(_("Unable to get VIOS profile name.")); goto err; } @@ -2330,6 +2334,7 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2385,6 +2390,7 @@ phypGetStoragePoolDevice(virConnectPtr conn, char *name) virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2438,6 +2444,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2487,12 +2494,13 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR("%s\"%s\"", "Unable to create Volume. Reason: ", ret); + VIR_ERROR(_("unable to create Volume. Reason: %s"), ret); goto err; } @@ -2535,7 +2543,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, if (pool->name != NULL) { spdef->name = pool->name; } else { - VIR_ERROR("%s", "Unable to determine storage pool's name."); + VIR_ERROR0(_("Unable to determine storage pool's name.")); goto err; } @@ -2546,7 +2554,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, if ((spdef->capacity = phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { - VIR_ERROR("%s", "Unable to determine storage pools's size."); + VIR_ERROR0(_("Unable to determine storage pools's size.")); goto err; } @@ -2559,19 +2567,19 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, /*XXX source adapter not working properly, should show hdiskX */ if ((spdef->source.adapter = phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { - VIR_ERROR("%s", - "Unable to determine storage pools's source adapter."); + VIR_ERROR0(_ + ("Unable to determine storage pools's source adapter.")); goto err; } if ((voldef = virStorageVolDefParseString(spdef, xml)) == NULL) { - VIR_ERROR("%s", "Error parsing volume XML."); + VIR_ERROR0(_("Error parsing volume XML.")); goto err; } /* checking if this name already exists on this system */ if (phypVolumeLookupByName(pool, voldef->name) != NULL) { - VIR_ERROR("%s", "StoragePool name already exists."); + VIR_ERROR0(_("StoragePool name already exists.")); goto err; } @@ -2579,13 +2587,13 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, * in the moment you create the volume. * */ if (voldef->key) { - VIR_ERROR("%s", - "Key must be empty, Power Hypervisor will create one for you."); + VIR_ERROR0(_ + ("Key must be empty, Power Hypervisor will create one for you.")); goto err; } if (voldef->capacity) { - VIR_ERROR("%s", "Capacity cannot be empty."); + VIR_ERROR0(_("Capacity cannot be empty.")); goto err; } @@ -2640,6 +2648,7 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -2693,6 +2702,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); spname = phypExec(session, cmd, &exit_status, conn); @@ -2741,17 +2751,17 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) if (sp->name != NULL) { pool.name = sp->name; } else { - VIR_ERROR("%s", "Unable to determine storage sp's name."); + VIR_ERROR0(_("Unable to determine storage sp's name.")); goto err; } if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) { - VIR_ERROR("%s", "Unable to determine storage sp's uuid."); + VIR_ERROR0(_("Unable to determine storage sp's uuid.")); goto err; } if ((pool.capacity = phypGetStoragePoolSize(sp->conn, sp->name)) == -1) { - VIR_ERROR("%s", "Unable to determine storage sps's size."); + VIR_ERROR0(_("Unable to determine storage sps's size.")); goto err; } @@ -2763,20 +2773,19 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) if ((pool.source.adapter = phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { - VIR_ERROR("%s", - "Unable to determine storage sps's source adapter."); + VIR_ERROR0(_("Unable to determine storage sps's source adapter.")); goto err; } if (vol->name != NULL) voldef.name = vol->name; else { - VIR_ERROR("%s", "Unable to determine storage pool's name."); + VIR_ERROR0(_("Unable to determine storage pool's name.")); goto err; } - if (memmove(voldef.key, vol->key, PATH_MAX) == NULL) { - VIR_ERROR("%s", "Unable to determine volume's key."); + if (memmove(voldef.key, vol->key, MAX_KEY_SIZE) == NULL) { + VIR_ERROR0(_("Unable to determine volume's key.")); goto err; } @@ -2829,6 +2838,7 @@ phypVolumeGetPath(virStorageVolPtr vol) virReportOOMError(); return NULL; } + cmd = virBufferContentAndReset(&buf); sp = phypExec(session, cmd, &exit_status, conn); @@ -3032,6 +3042,7 @@ phypDestroyStoragePool(virStoragePoolPtr pool) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); if (virAsprintf(&cmd, @@ -3045,8 +3056,7 @@ phypDestroyStoragePool(virStoragePoolPtr pool) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", - ret); + VIR_ERROR(_("Unable to create Storage Pool. Reason: %s"), ret); goto err; } @@ -3090,13 +3100,13 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR("%s\"%s\"", "Unable to create Storage Pool. Reason: ", - ret); + VIR_ERROR(_("Unable to create Storage Pool. Reason: %s"), ret); goto err; } @@ -3161,22 +3171,22 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) if (pool->name != NULL) def.name = pool->name; else { - VIR_ERROR("%s", "Unable to determine storage pool's name."); + VIR_ERROR0(_("Unable to determine storage pool's name.")); goto err; } if (memmove(def.uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { - VIR_ERROR("%s", "Unable to determine storage pool's uuid."); + VIR_ERROR0(_("Unable to determine storage pool's uuid.")); goto err; } if ((def.capacity = phypGetStoragePoolSize(pool->conn, pool->name)) == -1) { - VIR_ERROR("%s", "Unable to determine storage pools's size."); + VIR_ERROR0(_("Unable to determine storage pools's size.")); goto err; } - /* Information not avaliable */ + /* Information not available */ def.allocation = 0; def.available = 0; @@ -3185,8 +3195,8 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) /*XXX source adapter not working properly, should show hdiskX */ if ((def.source.adapter = phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { - VIR_ERROR("%s", - "Unable to determine storage pools's source adapter."); + VIR_ERROR0(_ + ("Unable to determine storage pools's source adapter.")); goto err; } @@ -3227,6 +3237,7 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -3348,6 +3359,7 @@ phypNumOfStoragePools(virConnectPtr conn) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -3400,6 +3412,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) virReportOOMError(); return -1; } + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); @@ -3437,6 +3450,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) VIR_FREE(ret); return -1; } + virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -3798,7 +3812,7 @@ phypUUIDTable_Push(virConnectPtr conn) /* end of file */ break; } else { - VIR_ERROR(_("Failed to read from '%s'"), local_file); + VIR_ERROR(_("Failed to read from %s"), local_file); goto err; } } diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h index e738927..14103f2 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -84,7 +84,8 @@ virStorageVolPtr phypStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, unsigned int flags); -virStorageVolPtr phypVolumeLookupByPath (virConnectPtr pool, const char *path); +virStorageVolPtr phypVolumeLookupByPath(virConnectPtr pool, + const char *path); char *phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags); @@ -109,11 +110,15 @@ int phypNumOfStoragePools(virConnectPtr conn); int phypListStoragePools(virConnectPtr conn, char **const pools, int npools); -virStoragePoolPtr phypStoragePoolLookupByName(virConnectPtr conn, const char *name); +virStoragePoolPtr phypStoragePoolLookupByName(virConnectPtr conn, + const char *name); -virStoragePoolPtr phypGetStoragePoolLookUpByUUID(virConnectPtr conn, const unsigned char *uuid); +virStoragePoolPtr phypGetStoragePoolLookUpByUUID(virConnectPtr conn, + const unsigned char + *uuid); -char * phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags); +char *phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, + unsigned int flags); virDrvOpenStatus phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED, virConnectAuthPtr auth ATTRIBUTE_UNUSED, -- 1.7.0.4
On 06/23/2010 04:49 PM, Eduardo Otubo wrote:
This is just the indentation, style and cleanup patch.
--- src/phyp/phyp_driver.c | 100 +++++++++++++++++++++++++++-------------------- src/phyp/phyp_driver.h | 13 ++++-- 2 files changed, 66 insertions(+), 47 deletions(-)
I'm actually going to focus on this one first. Any style changes to new code should have been folded in with the new code. In particular, this patch touches a lot of VIR_ERROR() calls added in your other patch, but if the other patch is committed first, then you have created a situation where 'git bisect' would needlessly fail 'make syntax-check' when visiting that commit. Therefore, I'm pruning out the changes to new code and squashing them into the 1/2 patch, which leaves:
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 51bab2c..7e39dfd 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -656,7 +656,8 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, virBufferAddLit(&buf, "lshwres"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); - virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d", + virBufferVSprintf(&buf, + " -r mem --level lpar -F %s --filter lpar_ids=%d", type ? "curr_mem" : "curr_max_mem", lpar_id); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf);
A few lines like this of indentation wrapping...
@@ -3798,7 +3812,7 @@ phypUUIDTable_Push(virConnectPtr conn) /* end of file */ break; } else { - VIR_ERROR(_("Failed to read from '%s'"), local_file); + VIR_ERROR(_("Failed to read from %s"), local_file); goto err; } }
And this change in the use of quotes of this message.
diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h index e738927..14103f2 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h
All of the changes to phyp_driver.h were in code introduced in 1/2, so I'm just squashing those. ACK to these minor cleanups that were not tied to 1/2, so I pushed them as attached: -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eduardo Otubo -
Eric Blake -
Jim Meyering -
Justin Clift