[libvirt] [PATCHv2 0/9] round 2 of phyp cleanups
Smaller code and fewer bugs. Rebase of earlier version at: https://www.redhat.com/archives/libvir-list/2011-April/msg00677.html Eric Blake (9): maint: use lighter-weight function for straight appends phyp: avoid a logic bug phyp: avoid memory leak on failure phyp: more return handling cleanup phyp: use consistent style for labels phyp: prefer memcpy over memmove when legal phyp: use consistent return string handling phyp: avoid memory leaks in command values phyp: another simplification src/conf/nwfilter_conf.c | 16 +- src/phyp/phyp_driver.c | 1355 ++++++++--------------------------------- src/qemu/qemu_command.c | 30 +- src/security/virt-aa-helper.c | 2 +- src/util/sexpr.c | 4 +- src/xen/xend_internal.c | 6 +- src/xenxs/xen_sxpr.c | 2 +- src/xenxs/xen_xm.c | 4 +- 8 files changed, 290 insertions(+), 1129 deletions(-) -- 1.7.4.2
It costs quite a few processor cycles to go through printf parsing just to determine that we only meant to append. * src/xen/xend_internal.c (xend_op_ext): Consolidate multiple printfs into one. * src/qemu/qemu_command.c (qemuBuildWatchdogDevStr) (qemuBuildUSBInputDevStr, qemuBuildSoundDevStr) (qemuBuildSoundCodecStr, qemuBuildVideoDevStr): Likewise. (qemuBuildCpuArgStr, qemuBuildCommandLine): Prefer virBufferAdd over virBufferVsprintf for trivial appends. * src/phyp/phyp_driver.c (phypExec, phypUUIDTable_Push) (phypUUIDTable_Pull): Likewise. * src/conf/nwfilter_conf.c (macProtocolIDFormatter) (arpOpcodeFormatter, formatIPProtocolID, printStringItems) (virNWFilterPrintStateMatchFlags, virNWIPAddressFormat) (virNWFilterDefFormat): Likewise. * src/security/virt-aa-helper.c (main): Likewise. * src/util/sexpr.c (sexpr2string): Likewise. * src/xenxs/xen_sxpr.c (xenFormatSxprChr): Likewise. * src/xenxs/xen_xm.c (xenFormatXMDisk): Likewise. --- src/conf/nwfilter_conf.c | 16 ++++++++-------- src/phyp/phyp_driver.c | 7 +++---- src/qemu/qemu_command.c | 30 +++++++++++------------------- src/security/virt-aa-helper.c | 2 +- src/util/sexpr.c | 4 ++-- src/xen/xend_internal.c | 6 ++---- src/xenxs/xen_sxpr.c | 2 +- src/xenxs/xen_xm.c | 4 ++-- 8 files changed, 30 insertions(+), 41 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 0732322..327fab3 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -488,7 +488,7 @@ macProtocolIDFormatter(virBufferPtr buf, if (intMapGetByInt(macProtoMap, nwf->p.ethHdrFilter.dataProtocolID.u.u16, &str)) { - virBufferVSprintf(buf, "%s", str); + virBufferAdd(buf, str, -1); } else { if (nwf->p.ethHdrFilter.dataProtocolID.datatype == DATATYPE_UINT16) asHex = false; @@ -591,7 +591,7 @@ arpOpcodeFormatter(virBufferPtr buf, if (intMapGetByInt(arpOpcodeMap, nwf->p.arpHdrFilter.dataOpcode.u.u16, &str)) { - virBufferVSprintf(buf, "%s", str); + virBufferAdd(buf, str, -1); } else { virBufferVSprintf(buf, "%d", nwf->p.arpHdrFilter.dataOpcode.u.u16); } @@ -653,7 +653,7 @@ formatIPProtocolID(virBufferPtr buf, if (intMapGetByInt(ipProtoMap, nwf->p.ipHdrFilter.ipHdr.dataProtocolID.u.u8, &str)) { - virBufferVSprintf(buf, "%s", str); + virBufferAdd(buf, str, -1); } else { if (nwf->p.ipHdrFilter.ipHdr.dataProtocolID.datatype == DATATYPE_UINT8) asHex = false; @@ -734,8 +734,8 @@ printStringItems(virBufferPtr buf, const struct int_map *int_map, for (i = 0; int_map[i].val; i++) { if (mask == int_map[i].attr) { if (c >= 1) - virBufferVSprintf(buf, "%s", sep); - virBufferVSprintf(buf, "%s", int_map[i].val); + virBufferAdd(buf, sep, -1); + virBufferAdd(buf, int_map[i].val, -1); c++; } } @@ -769,7 +769,7 @@ virNWFilterPrintStateMatchFlags(virBufferPtr buf, const char *prefix, if (!disp_none && (flags & RULE_FLAG_STATE_NONE)) return; - virBufferVSprintf(buf, "%s", prefix); + virBufferAdd(buf, prefix, -1); printStringItems(buf, stateMatchMap, flags, ","); } @@ -2699,7 +2699,7 @@ virNWIPAddressFormat(virBufferPtr buf, virSocketAddrPtr ipaddr) char *output = virSocketFormatAddr(ipaddr); if (output) { - virBufferVSprintf(buf, "%s", output); + virBufferAdd(buf, output, -1); VIR_FREE(output); } } @@ -2936,7 +2936,7 @@ virNWFilterDefFormat(virNWFilterDefPtr def) xml = virNWFilterEntryFormat(def->filterEntries[i]); if (!xml) goto err_exit; - virBufferVSprintf(&buf, "%s", xml); + virBufferAdd(&buf, xml, -1); VIR_FREE(xml); } diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3862c9c..5742d95 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1,4 +1,3 @@ - /* * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright IBM Corp. 2009 @@ -156,7 +155,7 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, rc = libssh2_channel_read(channel, buffer, buffer_size); if (rc > 0) { bytecount += rc; - virBufferVSprintf(&tex_ret, "%s", buffer); + virBufferAdd(&tex_ret, buffer, -1); } } while (rc > 0); @@ -494,7 +493,7 @@ phypUUIDTable_Push(virConnectPtr conn) char *remote_file = NULL; if (conn->uri->user != NULL) { - virBufferVSprintf(&username, "%s", conn->uri->user); + virBufferAdd(&username, conn->uri->user, -1); if (virBufferError(&username)) { virBufferFreeAndReset(&username); @@ -711,7 +710,7 @@ phypUUIDTable_Pull(virConnectPtr conn) char *remote_file = NULL; if (conn->uri->user != NULL) { - virBufferVSprintf(&username, "%s", conn->uri->user); + virBufferAdd(&username, conn->uri->user, -1); if (virBufferError(&username)) { virBufferFreeAndReset(&username); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 71e5062..2205ed1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1798,8 +1798,7 @@ qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, goto error; } - virBufferVSprintf(&buf, "%s", model); - virBufferVSprintf(&buf, ",id=%s", dev->info.alias); + virBufferVSprintf(&buf, "%s,id=%s", model, dev->info.alias); if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) goto error; @@ -1845,10 +1844,9 @@ qemuBuildUSBInputDevStr(virDomainInputDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; - virBufferVSprintf(&buf, "%s", + virBufferVSprintf(&buf, "%s,id=%s", dev->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ? - "usb-mouse" : "usb-tablet"); - virBufferVSprintf(&buf, ",id=%s", dev->info.alias); + "usb-mouse" : "usb-tablet", dev->info.alias); if (virBufferError(&buf)) { virReportOOMError(); @@ -1884,8 +1882,7 @@ qemuBuildSoundDevStr(virDomainSoundDefPtr sound, else if (STREQ(model, "ich6")) model = "intel-hda"; - virBufferVSprintf(&buf, "%s", model); - virBufferVSprintf(&buf, ",id=%s", sound->info.alias); + virBufferVSprintf(&buf, "%s,id=%s", model, sound->info.alias); if (qemuBuildDeviceAddressStr(&buf, &sound->info, qemuCaps) < 0) goto error; @@ -1908,10 +1905,8 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound, virBuffer buf = VIR_BUFFER_INITIALIZER; int cad = 0; - virBufferVSprintf(&buf, "%s", codec); - virBufferVSprintf(&buf, ",id=%s-codec%d", sound->info.alias, cad); - virBufferVSprintf(&buf, ",bus=%s.0", sound->info.alias); - virBufferVSprintf(&buf, ",cad=%d", cad); + virBufferVSprintf(&buf, "%s,id=%s-codec%d,bus=%s.0,cad=%d", + codec, sound->info.alias, cad, sound->info.alias, cad); if (virBufferError(&buf)) { virReportOOMError(); @@ -1938,8 +1933,7 @@ qemuBuildVideoDevStr(virDomainVideoDefPtr video, goto error; } - virBufferVSprintf(&buf, "%s", model); - virBufferVSprintf(&buf, ",id=%s", video->info.alias); + virBufferVSprintf(&buf, "%s,id=%s", model, video->info.alias); if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { if (video->vram > (UINT_MAX / 1024)) { @@ -2572,7 +2566,7 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup; *hasHwVirt = hasSVM > 0 ? true : false; - virBufferVSprintf(&buf, "%s", guest->model); + virBufferAdd(&buf, guest->model, -1); for (i = 0; i < guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) @@ -3128,7 +3122,7 @@ qemuBuildCommandLine(virConnectPtr conn, else if (def->os.bootmenu == VIR_DOMAIN_BOOT_MENU_DISABLED) virBufferVSprintf(&boot_buf, "order=%s,menu=off", boot); } else { - virBufferVSprintf(&boot_buf, "%s", boot); + virBufferAdd(&boot_buf, boot, -1); } virCommandAddArgBuffer(cmd, &boot_buf); @@ -3291,8 +3285,7 @@ qemuBuildCommandLine(virConnectPtr conn, host->name, host->port); } else { - virBufferVSprintf(&rbd_hosts, "%s", - host->name); + virBufferAdd(&rbd_hosts, host->name, -1); } } } @@ -3414,8 +3407,7 @@ qemuBuildCommandLine(virConnectPtr conn, host->name, host->port); } else { - virBufferVSprintf(&rbd_hosts, "%s", - host->name); + virBufferAdd(&rbd_hosts, host->name, -1); } } break; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index bb716e6..08ff53c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1204,7 +1204,7 @@ main(int argc, char **argv) virBufferVSprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); if (ctl->files) - virBufferVSprintf(&buf, "%s", ctl->files); + virBufferAdd(&buf, ctl->files, -1); } if (virBufferError(&buf)) { diff --git a/src/util/sexpr.c b/src/util/sexpr.c index ae1692a..da3d4b3 100644 --- a/src/util/sexpr.c +++ b/src/util/sexpr.c @@ -1,7 +1,7 @@ /* * sexpr.c : S-Expression routines to communicate with the Xen Daemon * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2005 Anthony Liguori <aliguori@us.ibm.com> * * This file is subject to the terms and conditions of the GNU Lesser General @@ -234,7 +234,7 @@ sexpr2string(const struct sexpr *sexpr, virBufferPtr buffer) strchr(sexpr->u.value, '(')) virBufferVSprintf(buffer, "'%s'", sexpr->u.value); else - virBufferVSprintf(buffer, "%s", sexpr->u.value); + virBufferAdd(buffer, sexpr->u.value, -1); break; case SEXPR_NIL: diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 04122ba..57422d3 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -487,13 +487,11 @@ xend_op_ext(virConnectPtr xend, const char *path, const char *key, va_list ap) while (k) { v = va_arg(ap, const char *); - virBufferVSprintf(&buf, "%s", k); - virBufferVSprintf(&buf, "%s", "="); - virBufferVSprintf(&buf, "%s", v); + virBufferVSprintf(&buf, "%s=%s", k, v); k = va_arg(ap, const char *); if (k) - virBufferVSprintf(&buf, "%s", "&"); + virBufferAddChar(&buf, '&'); } if (virBufferError(&buf)) { diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index b590517..d2ec370 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1514,7 +1514,7 @@ xenFormatSxprChr(virDomainChrDefPtr def, case VIR_DOMAIN_CHR_TYPE_STDIO: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_PTY: - virBufferVSprintf(buf, "%s", type); + virBufferAdd(buf, type, -1); break; case VIR_DOMAIN_CHR_TYPE_FILE: diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 89f75a5..dbcaf15 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1115,13 +1115,13 @@ static int xenFormatXMDisk(virConfValuePtr list, goto cleanup; } } - virBufferVSprintf(&buf, "%s", disk->src); + virBufferAdd(&buf, disk->src, -1); } virBufferAddLit(&buf, ","); if (hvm && xendConfigVersion == 1) virBufferAddLit(&buf, "ioemu:"); - virBufferVSprintf(&buf, "%s", disk->dst); + virBufferAdd(&buf, disk->dst, -1); if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) virBufferAddLit(&buf, ":cdrom"); -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
It costs quite a few processor cycles to go through printf parsing just to determine that we only meant to append.
- virBufferVSprintf(&buf, "%s", k); - virBufferVSprintf(&buf, "%s", "="); - virBufferVSprintf(&buf, "%s", v); + virBufferVSprintf(&buf, "%s=%s", k, v);
Nice one :) ACK. Matthias
Ever since commit ebc46f, the destroy function built two command variants but only used one. I went with the variant that matches the idiom used in the counterpart of phypBuildStoragePool. * src/phyp/phyp_driver.c (phypDestroyStoragePool): Avoid clobbering cmd. Fix error message typo. --- src/phyp/phyp_driver.c | 11 +---------- 1 files changed, 1 insertions(+), 10 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 5742d95..7aa494d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2936,19 +2936,10 @@ phypDestroyStoragePool(virStoragePoolPtr pool) 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 cleanup; - } - ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR(_("Unable to create Storage Pool: %s"), ret); + VIR_ERROR(_("Unable to destroy Storage Pool: %s"), ret); goto cleanup; } -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
Ever since commit ebc46f, the destroy function built two command variants but only used one. I went with the variant that matches the idiom used in the counterpart of phypBuildStoragePool.
* src/phyp/phyp_driver.c (phypDestroyStoragePool): Avoid clobbering cmd. Fix error message typo. --- src/phyp/phyp_driver.c | 11 +---------- 1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 5742d95..7aa494d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2936,19 +2936,10 @@ phypDestroyStoragePool(virStoragePoolPtr pool) 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 cleanup; - } - ret = phypExec(session, cmd, &exit_status, conn);
if (exit_status < 0) { - VIR_ERROR(_("Unable to create Storage Pool: %s"), ret); + VIR_ERROR(_("Unable to destroy Storage Pool: %s"), ret); goto cleanup; }
I just noticed that the driver uses VIR_ERROR in many places where PHYP_ERROR should be used to report an actual libvirt API level error. But that a problem for another patch. ACK, to this one. Matthias
* src/phyp/phyp_driver.c (phypUUIDTable_Init): Avoid memory leak on error. --- src/phyp/phyp_driver.c | 46 +++++++++++++++++++++++++++------------------- 1 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 7aa494d..0c69d4f 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -807,31 +807,35 @@ phypUUIDTable_Pull(virConnectPtr conn) static int phypUUIDTable_Init(virConnectPtr conn) { - uuid_tablePtr uuid_table; + uuid_tablePtr uuid_table = NULL; phyp_driverPtr phyp_driver; int nids_numdomains = 0; int nids_listdomains = 0; int *ids = NULL; unsigned int i = 0; + int ret = -1; + bool table_created = false; if ((nids_numdomains = phypNumDomainsGeneric(conn, 2)) < 0) - goto err; + goto cleanup; if (VIR_ALLOC_N(ids, nids_numdomains) < 0) { virReportOOMError(); - goto err; + goto cleanup; } if ((nids_listdomains = phypListDomainsGeneric(conn, ids, nids_numdomains, 1)) < 0) - goto err; + goto cleanup; /* exit early if there are no domains */ - if (nids_numdomains == 0 && nids_listdomains == 0) - goto exit; - else if (nids_numdomains != nids_listdomains) { + if (nids_numdomains == 0 && nids_listdomains == 0) { + ret = 0; + goto cleanup; + } + if (nids_numdomains != nids_listdomains) { VIR_ERROR0(_("Unable to determine number of domains.")); - goto err; + goto cleanup; } phyp_driver = conn->privateData; @@ -841,11 +845,12 @@ phypUUIDTable_Init(virConnectPtr conn) /* try to get the table from server */ if (phypUUIDTable_Pull(conn) == -1) { /* file not found in the server, creating a new one */ + table_created = true; if (VIR_ALLOC_N(uuid_table->lpars, uuid_table->nlpars) >= 0) { for (i = 0; i < uuid_table->nlpars; i++) { if (VIR_ALLOC(uuid_table->lpars[i]) < 0) { virReportOOMError(); - goto err; + goto cleanup; } uuid_table->lpars[i]->id = ids[i]; @@ -855,27 +860,30 @@ phypUUIDTable_Init(virConnectPtr conn) } } else { virReportOOMError(); - goto err; + goto cleanup; } if (phypUUIDTable_WriteFile(conn) == -1) - goto err; + goto cleanup; if (phypUUIDTable_Push(conn) == -1) - goto err; + goto cleanup; } else { if (phypUUIDTable_ReadFile(conn) == -1) - goto err; - goto exit; + goto cleanup; } - exit: - VIR_FREE(ids); - return 0; + ret = 0; - err: +cleanup: + if (ret < 0 && table_created) { + for (i = 0; i < uuid_table->nlpars; i++) { + VIR_FREE(uuid_table->lpars[i]); + } + VIR_FREE(uuid_table->lpars); + } VIR_FREE(ids); - return -1; + return ret; } static void -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
* src/phyp/phyp_driver.c (phypUUIDTable_Init): Avoid memory leak on error. --- src/phyp/phyp_driver.c | 46 +++++++++++++++++++++++++++------------------- 1 files changed, 27 insertions(+), 19 deletions(-)
ACK. Matthias
* src/phyp/phyp_driver.c (phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypListInterfaces, phypNumOfInterfaces): Clean up return handling of recent additions. --- src/phyp/phyp_driver.c | 130 +++++++++++++++++++++--------------------------- 1 files changed, 56 insertions(+), 74 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 0c69d4f..37395c6 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3294,6 +3294,7 @@ phypInterfaceDestroy(virInterfacePtr iface, char *char_ptr; char *cmd = NULL; char *ret = NULL; + int rv = -1; /* Getting the remote slot number */ @@ -3309,17 +3310,17 @@ phypInterfaceDestroy(virInterfacePtr iface, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, iface->conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1) - goto err; + goto cleanup; /* Getting the remote slot number */ VIR_FREE(cmd); @@ -3337,7 +3338,7 @@ phypInterfaceDestroy(virInterfacePtr iface, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); @@ -3346,10 +3347,10 @@ phypInterfaceDestroy(virInterfacePtr iface, ret = phypExec(session, cmd, &exit_status, iface->conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) - goto err; + goto cleanup; /* excluding interface */ VIR_FREE(cmd); @@ -3366,23 +3367,21 @@ phypInterfaceDestroy(virInterfacePtr iface, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, iface->conn); if (exit_status < 0 || ret != NULL) - goto err; + goto cleanup; - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + rv = 0; - err: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + return rv; } static virInterfacePtr @@ -3405,9 +3404,10 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, char name[PHYP_IFACENAME_SIZE]; char mac[PHYP_MAC_SIZE]; virInterfaceDefPtr def; + virInterfacePtr result = NULL; if (!(def = virInterfaceDefParseString(xml))) - goto err; + goto cleanup; /* Now need to get the next free slot number */ virBufferAddLit(&buf, "lshwres "); @@ -3422,17 +3422,17 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto err; + goto cleanup; /* The next free slot itself: */ slot++; @@ -3453,14 +3453,14 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret != NULL) - goto err; + goto cleanup; /* Need to sleep a little while to wait for the HMC to * complete the execution of the command. @@ -3483,7 +3483,7 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); @@ -3505,13 +3505,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); - goto err; + goto cleanup; } memcpy(name, ret, PHYP_IFACENAME_SIZE-1); @@ -3532,27 +3532,24 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; memcpy(mac, ret, PHYP_MAC_SIZE-1); - VIR_FREE(cmd); - VIR_FREE(ret); - virInterfaceDefFree(def); - return virGetInterface(conn, name, mac); + result = virGetInterface(conn, name, mac); - err: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); virInterfaceDefFree(def); - return NULL; + return result; } static virInterfacePtr @@ -3586,17 +3583,17 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto err; + goto cleanup; /*Getting the lpar_id for the interface */ VIR_FREE(cmd); @@ -3621,10 +3618,10 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) - goto err; + goto cleanup; /*Getting the interface mac */ virBufferAddLit(&buf, "lshwres "); @@ -3639,7 +3636,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return NULL; + goto cleanup; } cmd = virBufferContentAndReset(&buf); @@ -3648,21 +3645,16 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; memcpy(mac, ret, PHYP_MAC_SIZE-1); result = virGetInterface(conn, name, ret); +cleanup: VIR_FREE(cmd); VIR_FREE(ret); return result; - - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; - } static int @@ -3675,7 +3667,7 @@ phypInterfaceIsActive(virInterfacePtr iface) char *managed_system = phyp_driver->managed_system; int system_type = phyp_driver->system_type; int exit_status = 0; - int state = 0; + int state = -1; char *char_ptr; char *cmd = NULL; char *ret = NULL; @@ -3692,27 +3684,22 @@ phypInterfaceIsActive(virInterfacePtr iface) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, iface->conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) - goto err; + goto cleanup; +cleanup: VIR_FREE(cmd); VIR_FREE(ret); return state; - - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; - } static int @@ -3732,6 +3719,7 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) char *networks = NULL; char *char_ptr2 = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + bool success = false; virBufferAddLit(&buf, "lshwres"); if (system_type == HMC) @@ -3742,15 +3730,16 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); - /* I need to parse the textual return in order to get the network interfaces */ + /* I need to parse the textual return in order to get the network + * interfaces */ if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; networks = ret; @@ -3761,7 +3750,7 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) *char_ptr2 = '\0'; if ((names[got++] = strdup(networks)) == NULL) { virReportOOMError(); - goto err; + goto cleanup; } char_ptr2++; networks = char_ptr2; @@ -3770,16 +3759,14 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) } } +cleanup: + if (!success) { + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + } VIR_FREE(cmd); VIR_FREE(ret); return got; - - err: - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; } static int @@ -3792,7 +3779,7 @@ phypNumOfInterfaces(virConnectPtr conn) int system_type = phyp_driver->system_type; int vios_id = phyp_driver->vios_id; int exit_status = 0; - int nnets = 0; + int nnets = -1; char *char_ptr; char *cmd = NULL; char *ret = NULL; @@ -3809,27 +3796,22 @@ phypNumOfInterfaces(virConnectPtr conn) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - goto err; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) - goto err; + goto cleanup; +cleanup: VIR_FREE(cmd); VIR_FREE(ret); return nnets; - - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; - } static int -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
* src/phyp/phyp_driver.c (phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypListInterfaces, phypNumOfInterfaces): Clean up return handling of recent additions. --- src/phyp/phyp_driver.c | 130 +++++++++++++++++++++--------------------------- 1 files changed, 56 insertions(+), 74 deletions(-)
ACK. Mathias
* src/phyp/phyp_driver.c: Match label style of rest of project. (phypExec, phypUUIDTable_Pull): Drop an extra label. --- src/phyp/phyp_driver.c | 129 +++++++++++++++++++++++------------------------- 1 files changed, 62 insertions(+), 67 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 37395c6..a952875 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -182,15 +182,6 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, (*exit_status) = exitcode; libssh2_channel_free(channel); channel = NULL; - goto exit; - - err: - (*exit_status) = SSH_CMD_ERR; - virBufferFreeAndReset(&tex_ret); - VIR_FREE(buffer); - return NULL; - - exit: VIR_FREE(buffer); if (virBufferError(&tex_ret)) { @@ -199,6 +190,12 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, return NULL; } return virBufferContentAndReset(&tex_ret); + +err: + (*exit_status) = SSH_CMD_ERR; + virBufferFreeAndReset(&tex_ret); + VIR_FREE(buffer); + return NULL; } static int @@ -256,7 +253,7 @@ phypGetVIOSPartitionID(virConnectPtr conn) if (virStrToLong_i(ret, &char_ptr, 10, &id) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -302,7 +299,7 @@ phypCapsInit(void) return caps; - no_memory: +no_memory: virCapabilitiesFree(caps); return NULL; } @@ -362,7 +359,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -430,7 +427,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, line++; /* skip \n */ } - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -471,7 +468,7 @@ phypUUIDTable_WriteFile(virConnectPtr conn) } return 0; - err: +err: VIR_FORCE_CLOSE(fd); return -1; } @@ -569,7 +566,7 @@ phypUUIDTable_Push(virConnectPtr conn) virBufferFreeAndReset(&username); return 0; - err: +err: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); @@ -602,7 +599,7 @@ phypUUIDTable_RemLpar(virConnectPtr conn, int id) return 0; - err: +err: return -1; } @@ -637,7 +634,7 @@ phypUUIDTable_AddLpar(virConnectPtr conn, unsigned char *uuid, int id) return 0; - err: +err: return -1; } @@ -686,7 +683,7 @@ phypUUIDTable_ReadFile(virConnectPtr conn) VIR_FORCE_CLOSE(fd); return 0; - err: +err: VIR_FORCE_CLOSE(fd); return -1; } @@ -780,9 +777,7 @@ phypUUIDTable_Pull(virConnectPtr conn) local_file); goto err; } - goto exit; - exit: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); @@ -793,7 +788,7 @@ phypUUIDTable_Pull(virConnectPtr conn) virBufferFreeAndReset(&username); return 0; - err: +err: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); @@ -1041,7 +1036,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, freeaddrinfo(ai); goto err; - connected: +connected: (*internal_socket) = sock; @@ -1074,7 +1069,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, NULL)) == LIBSSH2_ERROR_EAGAIN) ; - keyboard_interactive: +keyboard_interactive: if (rc == LIBSSH2_ERROR_SOCKET_NONE || rc == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || rc == LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) { @@ -1112,10 +1107,10 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, goto err; } - disconnect: +disconnect: libssh2_session_disconnect(session, "Disconnecting..."); libssh2_session_free(session); - err: +err: VIR_FREE(userhome); VIR_FREE(pubkey); VIR_FREE(pvtkey); @@ -1123,7 +1118,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, VIR_FREE(password); return NULL; - exit: +exit: VIR_FREE(userhome); VIR_FREE(pubkey); VIR_FREE(pvtkey); @@ -1235,7 +1230,7 @@ phypOpen(virConnectPtr conn, return VIR_DRV_OPEN_SUCCESS; - failure: +failure: if (phyp_driver != NULL) { virCapabilitiesFree(phyp_driver->caps); VIR_FREE(phyp_driver->managed_system); @@ -1327,7 +1322,7 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -1370,7 +1365,7 @@ phypGetLparNAME(LIBSSH2_SESSION * session, const char *managed_system, if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); return ret; @@ -1450,7 +1445,7 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, if (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -1498,7 +1493,7 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, if (virStrToLong_i(ret, &char_ptr, 10, &vcpus) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -1572,7 +1567,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, if (virStrToLong_i(ret, &char_ptr, 10, &remote_slot) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -1651,7 +1646,7 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -1697,7 +1692,7 @@ phypGetLparProfile(virConnectPtr conn, int lpar_id) if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); return ret; @@ -1755,7 +1750,7 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) slot += 1; - cleanup: +cleanup: VIR_FREE(profile); VIR_FREE(cmd); VIR_FREE(ret); @@ -1869,7 +1864,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) result = 0; - cleanup: +cleanup: VIR_FREE(profile); VIR_FREE(vios_name); VIR_FREE(cmd); @@ -1923,7 +1918,7 @@ phypGetVIOSFreeSCSIAdapter(virConnectPtr conn) if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); return ret; @@ -2133,7 +2128,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); virDomainDeviceDefFree(dev); @@ -2191,7 +2186,7 @@ phypVolumeGetKey(virConnectPtr conn, const char *name) if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); return ret; @@ -2242,7 +2237,7 @@ phypGetStoragePoolDevice(virConnectPtr conn, char *name) if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); return ret; @@ -2290,7 +2285,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) if (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -2341,7 +2336,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, if (key == NULL) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -2454,7 +2449,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, return vol; - err: +err: VIR_FREE(key); virStorageVolDefFree(voldef); virStoragePoolDefFree(spdef); @@ -2509,7 +2504,7 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) if (char_ptr) *char_ptr = '\0'; - cleanup: +cleanup: VIR_FREE(cmd); return ret; @@ -2567,7 +2562,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) vol = virGetStorageVol(conn, spname, volname, key); - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(spname); VIR_FREE(key); @@ -2619,7 +2614,7 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -2706,7 +2701,7 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) return xml; - err: +err: return NULL; } @@ -2775,7 +2770,7 @@ phypVolumeGetPath(virStorageVolPtr vol) goto cleanup; } - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(sp); VIR_FREE(path); @@ -2849,7 +2844,7 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, success = true; - cleanup: +cleanup: if (!success) { for (i = 0; i < got; i++) VIR_FREE(volumes[i]); @@ -2906,7 +2901,7 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) /* We need to remove 2 line from the header text output */ nvolumes -= 2; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -2953,7 +2948,7 @@ phypDestroyStoragePool(virStoragePoolPtr pool) result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -3002,7 +2997,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -3052,7 +3047,7 @@ phypNumOfStoragePools(virConnectPtr conn) if (virStrToLong_i(ret, &char_ptr, 10, &nsp) == -1) goto cleanup; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -3120,7 +3115,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) success = true; - cleanup: +cleanup: if (!success) { for (i = 0; i < got; i++) VIR_FREE(pools[i]); @@ -3186,7 +3181,7 @@ phypGetStoragePoolLookUpByUUID(virConnectPtr conn, } } - err: +err: VIR_FREE(local_uuid); VIR_FREE(pools); return NULL; @@ -3224,7 +3219,7 @@ phypStoragePoolCreateXML(virConnectPtr conn, return sp; - err: +err: virStoragePoolDefFree(def); if (sp) virUnrefStoragePool(sp); @@ -3272,7 +3267,7 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) return virStoragePoolDefFormat(&def); - err: +err: return NULL; } @@ -3857,7 +3852,7 @@ phypGetLparState(virConnectPtr conn, unsigned int lpar_id) else if (STREQ(ret, "Shutting Down")) state = VIR_DOMAIN_SHUTDOWN; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); return state; @@ -3909,7 +3904,7 @@ phypDiskType(virConnectPtr conn, char *backing_device) else if (STREQ(ret, "FBPOOL")) disk_type = VIR_DOMAIN_DISK_TYPE_FILE; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); return disk_type; @@ -3989,7 +3984,7 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) success = true; - cleanup: +cleanup: if (!success) { for (i = 0; i < got; i++) VIR_FREE(names[i]); @@ -4054,7 +4049,7 @@ phypDomainLookupByID(virConnectPtr conn, int lpar_id) if (dom) dom->id = lpar_id; - cleanup: +cleanup: VIR_FREE(lpar_name); return dom; @@ -4107,7 +4102,7 @@ phypDomainDumpXML(virDomainPtr dom, int flags) return virDomainDefFormat(&def, flags); - err: +err: return NULL; } @@ -4144,7 +4139,7 @@ phypDomainResume(virDomainPtr dom) result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -4184,7 +4179,7 @@ phypDomainShutdown(virDomainPtr dom) result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -4250,7 +4245,7 @@ phypDomainDestroy(virDomainPtr dom) dom->id = -1; result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -4327,7 +4322,7 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) result = 0; - cleanup: +cleanup: VIR_FREE(cmd); VIR_FREE(ret); @@ -4381,7 +4376,7 @@ phypDomainCreateAndStart(virConnectPtr conn, return dom; - err: +err: virDomainDefFree(def); if (dom) virUnrefDomain(dom); -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
* src/phyp/phyp_driver.c: Match label style of rest of project. (phypExec, phypUUIDTable_Pull): Drop an extra label. --- src/phyp/phyp_driver.c | 129 +++++++++++++++++++++++------------------------- 1 files changed, 62 insertions(+), 67 deletions(-)
ACK. Matthias
* src/phyp/phyp_driver.c (phypUUIDTable_AddLpar) (phypGetLparUUID, phypGetStoragePoolUUID, phypVolumeGetXMLDesc) (phypGetStoragePoolXMLDesc): Use faster method. --- src/phyp/phyp_driver.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index a952875..27536eb 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -624,7 +624,7 @@ phypUUIDTable_AddLpar(virConnectPtr conn, unsigned char *uuid, int id) } uuid_table->lpars[i]->id = id; - memmove(uuid_table->lpars[i]->uuid, uuid, VIR_UUID_BUFLEN); + memcpy(uuid_table->lpars[i]->uuid, uuid, VIR_UUID_BUFLEN); if (phypUUIDTable_WriteFile(conn) == -1) goto err; @@ -1388,7 +1388,7 @@ phypGetLparUUID(unsigned char *uuid, int lpar_id, virConnectPtr conn) for (i = 0; i < uuid_table->nlpars; i++) { if (lpars[i]->id == lpar_id) { - memmove(uuid, lpars[i]->uuid, VIR_UUID_BUFLEN); + memcpy(uuid, lpars[i]->uuid, VIR_UUID_BUFLEN); return 0; } } @@ -2609,7 +2609,7 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, if (exit_status < 0 || ret == NULL) goto cleanup; - if (memmove(uuid, ret, VIR_UUID_BUFLEN) == NULL) + if (memcpy(uuid, ret, VIR_UUID_BUFLEN) == NULL) goto cleanup; result = 0; @@ -2657,7 +2657,7 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) goto err; } - if (memmove(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) { + if (memcpy(pool.uuid, sp->uuid, VIR_UUID_BUFLEN) == NULL) { VIR_ERROR0(_("Unable to determine storage sp's uuid.")); goto err; } @@ -3241,7 +3241,7 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) goto err; } - if (memmove(def.uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { + if (memcpy(def.uuid, pool->uuid, VIR_UUID_BUFLEN) == NULL) { VIR_ERROR0(_("Unable to determine storage pool's uuid.")); goto err; } -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
* src/phyp/phyp_driver.c (phypUUIDTable_AddLpar) (phypGetLparUUID, phypGetStoragePoolUUID, phypVolumeGetXMLDesc) (phypGetStoragePoolXMLDesc): Use faster method. --- src/phyp/phyp_driver.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
ACK. Matthias
Use the name 'ret' for all phypExec results, to make it easier to wrap phypExec. Don't allow a possibly NULL ret through printf. * src/phyp/phyp_driver.c (phypBuildVolume, phypDestroyStoragePool) (phypBuildStoragePool, phypBuildLpar): Avoid NULL dereference. (phypInterfaceDestroy): Avoid redundant free. (phypVolumeLookupByPath, phypVolumeGetPath): Use consistent naming. --- src/phyp/phyp_driver.c | 37 +++++++++++++++++-------------------- 1 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 27536eb..56909af 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2327,7 +2327,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR(_("Unable to create Volume: %s"), ret); + VIR_ERROR(_("Unable to create Volume: %s"), NULLSTR(ret)); goto cleanup; } @@ -2521,7 +2521,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) int vios_id = phyp_driver->vios_id; int exit_status = 0; char *cmd = NULL; - char *spname = NULL; + char *ret = NULL; char *char_ptr; char *key = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2545,12 +2545,12 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) } cmd = virBufferContentAndReset(&buf); - spname = phypExec(session, cmd, &exit_status, conn); + ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || spname == NULL) + if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(spname, '\n'); + char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; @@ -2560,11 +2560,11 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) if (key == NULL) goto cleanup; - vol = virGetStorageVol(conn, spname, volname, key); + vol = virGetStorageVol(conn, ret, volname, key); cleanup: VIR_FREE(cmd); - VIR_FREE(spname); + VIR_FREE(ret); VIR_FREE(key); return vol; @@ -2725,7 +2725,7 @@ phypVolumeGetPath(virStorageVolPtr vol) int vios_id = phyp_driver->vios_id; int exit_status = 0; char *cmd = NULL; - char *sp = NULL; + char *ret = NULL; char *path = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; char *char_ptr; @@ -2750,33 +2750,32 @@ phypVolumeGetPath(virStorageVolPtr vol) } cmd = virBufferContentAndReset(&buf); - sp = phypExec(session, cmd, &exit_status, conn); + ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || sp == NULL) + if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(sp, '\n'); + char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; - pv = phypVolumeGetPhysicalVolumeByStoragePool(vol, sp); + pv = phypVolumeGetPhysicalVolumeByStoragePool(vol, ret); if (!pv) goto cleanup; - if (virAsprintf(&path, "/%s/%s/%s", pv, sp, vol->name) < 0) { + if (virAsprintf(&path, "/%s/%s/%s", pv, ret, vol->name) < 0) { virReportOOMError(); goto cleanup; } cleanup: VIR_FREE(cmd); - VIR_FREE(sp); + VIR_FREE(ret); VIR_FREE(path); return path; - } static int @@ -2942,7 +2941,7 @@ phypDestroyStoragePool(virStoragePoolPtr pool) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR(_("Unable to destroy Storage Pool: %s"), ret); + VIR_ERROR(_("Unable to destroy Storage Pool: %s"), NULLSTR(ret)); goto cleanup; } @@ -2991,7 +2990,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR(_("Unable to create Storage Pool: %s"), ret); + VIR_ERROR(_("Unable to create Storage Pool: %s"), NULLSTR(ret)); goto cleanup; } @@ -3337,8 +3336,6 @@ phypInterfaceDestroy(virInterfacePtr iface, } cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - ret = phypExec(session, cmd, &exit_status, iface->conn); if (exit_status < 0 || ret == NULL) @@ -4311,7 +4308,7 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { - VIR_ERROR(_("Unable to create LPAR. Reason: '%s'"), ret); + VIR_ERROR(_("Unable to create LPAR. Reason: '%s'"), NULLSTR(ret)); goto cleanup; } -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
Use the name 'ret' for all phypExec results, to make it easier to wrap phypExec. Don't allow a possibly NULL ret through printf.
* src/phyp/phyp_driver.c (phypBuildVolume, phypDestroyStoragePool) (phypBuildStoragePool, phypBuildLpar): Avoid NULL dereference. (phypInterfaceDestroy): Avoid redundant free. (phypVolumeLookupByPath, phypVolumeGetPath): Use consistent naming. --- src/phyp/phyp_driver.c | 37 +++++++++++++++++-------------------- 1 files changed, 17 insertions(+), 20 deletions(-)
ACK. Matthias
* src/phyp/phyp_driver.c (phypExecBuffer): New function. Use it throughout file for less code, and for plugging a few leaks. --- Amazing how much copy-and-paste code this consolidated. src/phyp/phyp_driver.c | 803 ++++++------------------------------------------ 1 files changed, 92 insertions(+), 711 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 56909af..bc24b76 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -110,6 +110,9 @@ waitsocket(int socket_fd, LIBSSH2_SESSION * session) /* this function is the layer that manipulates the ssh channel itself * and executes the commands on the remote machine */ +static char *phypExec(LIBSSH2_SESSION *, const char *, int *, virConnectPtr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); static char * phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, virConnectPtr conn) @@ -198,6 +201,33 @@ err: return NULL; } +/* Convenience wrapper function */ +static char *phypExecBuffer(LIBSSH2_SESSION *, virBufferPtr buf, int *, + virConnectPtr, bool) ATTRIBUTE_NONNULL(1) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +static char * +phypExecBuffer(LIBSSH2_SESSION *session, virBufferPtr buf, int *exit_status, + virConnectPtr conn, bool strip_newline) +{ + char *cmd; + char *ret; + + if (virBufferError(buf)) { + virBufferFreeAndReset(buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(buf); + ret = phypExec(session, cmd, exit_status, conn); + VIR_FREE(cmd); + if (ret && *exit_status == 0 && strip_newline) { + char *nl = strchr(ret, '\n'); + if (nl) + *nl = '\0'; + } + return ret; +} + static int phypGetSystemType(virConnectPtr conn) { @@ -225,7 +255,6 @@ phypGetVIOSPartitionID(virConnectPtr conn) phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; int exit_status = 0; int id = -1; @@ -238,14 +267,7 @@ phypGetVIOSPartitionID(virConnectPtr conn) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferAddLit(&buf, " -r lpar -F lpar_id,lpar_env" "|sed -n '/vioserver/ {\n s/,.*$//\n p\n}'"); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -254,7 +276,6 @@ phypGetVIOSPartitionID(virConnectPtr conn) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return id; @@ -322,7 +343,6 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) int exit_status = 0; int ndom = -1; char *char_ptr; - char *cmd = NULL; char *ret = NULL; char *managed_system = phyp_driver->managed_system; const char *state; @@ -344,14 +364,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", state); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -360,7 +373,6 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return ndom; @@ -384,7 +396,6 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, char *managed_system = phyp_driver->managed_system; int exit_status = 0; int got = -1; - char *cmd = NULL; char *ret = NULL; char *line, *next_line; const char *state; @@ -400,14 +411,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F lpar_id,state %s | sed -e 's/,.*$//'", state); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -428,9 +432,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, } cleanup: - VIR_FREE(cmd); VIR_FREE(ret); - return got; } @@ -1299,7 +1301,6 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, int exit_status = 0; int lpar_id = -1; char *char_ptr; - char *cmd = NULL; char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1307,14 +1308,7 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " --filter lpar_names=%s -F lpar_id", name); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1323,7 +1317,6 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return lpar_id; @@ -1336,38 +1329,18 @@ phypGetLparNAME(LIBSSH2_SESSION * session, const char *managed_system, { phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; - char *char_ptr; virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " --filter lpar_ids=%d -F name", lpar_id); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); - if (exit_status < 0 || ret == NULL) { + if (exit_status < 0) VIR_FREE(ret); - goto cleanup; - } - - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - -cleanup: - VIR_FREE(cmd); - return ret; } @@ -1409,7 +1382,6 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; char *char_ptr; int memory = 0; @@ -1425,28 +1397,15 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, 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); - virReportOOMError(); - return 0; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - if (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return memory; @@ -1460,7 +1419,6 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; char *char_ptr; int exit_status = 0; @@ -1473,28 +1431,15 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, 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); - virReportOOMError(); - return 0; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - if (virStrToLong_i(ret, &char_ptr, 10, &vcpus) == -1) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return vcpus; @@ -1535,7 +1480,6 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; char *char_ptr; int remote_slot = -1; @@ -1547,28 +1491,15 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F " "remote_slot_num --filter lpar_names=%s", lpar_name); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - if (virStrToLong_i(ret, &char_ptr, 10, &remote_slot) == -1) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return remote_slot; @@ -1585,7 +1516,6 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; int remote_slot = 0; int exit_status = 0; @@ -1602,14 +1532,7 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F " "backing_devices --filter slots=%d", remote_slot); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1647,7 +1570,6 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, *char_ptr = '\0'; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return backing_device; @@ -1662,10 +1584,8 @@ phypGetLparProfile(virConnectPtr conn, int lpar_id) 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; - char *char_ptr; virBufferAddLit(&buf, "lssyscfg"); if (system_type == HMC) @@ -1673,28 +1593,10 @@ phypGetLparProfile(virConnectPtr conn, int lpar_id) 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 = phypExecBuffer(session, &buf, &exit_status, conn, true); - ret = phypExec(session, cmd, &exit_status, conn); - - if (exit_status < 0 || ret == NULL) { + if (exit_status < 0) VIR_FREE(ret); - goto cleanup; - } - - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - -cleanup: - VIR_FREE(cmd); - return ret; } @@ -1709,7 +1611,6 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) 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 = -1; @@ -1731,16 +1632,7 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) "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(); - goto cleanup; - } - - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1752,7 +1644,6 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) cleanup: VIR_FREE(profile); - VIR_FREE(cmd); VIR_FREE(ret); return slot; @@ -1769,7 +1660,6 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) 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; @@ -1802,14 +1692,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) 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(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1823,17 +1706,8 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) 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(); - goto cleanup; - } - - VIR_FREE(cmd); VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1847,17 +1721,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) virBufferVSprintf(&buf, " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", vios_name, slot); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1867,7 +1731,6 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) cleanup: VIR_FREE(profile); VIR_FREE(vios_name); - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -1883,10 +1746,8 @@ phypGetVIOSFreeSCSIAdapter(virConnectPtr conn) 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; - char *char_ptr; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -1898,29 +1759,10 @@ phypGetVIOSFreeSCSIAdapter(virConnectPtr conn) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed '/,[^.*]/d; s/,//g; q'"); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); - - if (exit_status < 0 || ret == NULL) { + if (exit_status < 0) VIR_FREE(ret); - goto cleanup; - } - - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - -cleanup: - VIR_FREE(cmd); - return ret; } @@ -1938,7 +1780,6 @@ phypAttachDevice(virDomainPtr domain, const char *xml) 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; @@ -2000,18 +1841,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (system_type == HMC) virBufferAddChar(&buf, '\''); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2029,17 +1859,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " slot_num,backing_device|grep %s|cut -d, -f1", dev->data.disk->src); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2057,17 +1877,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) " -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(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2083,17 +1893,8 @@ phypAttachDevice(virDomainPtr domain, const char *xml) "\"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(); - goto cleanup; - } - - VIR_FREE(cmd); VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) goto cleanup; @@ -2107,17 +1908,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", domain_name, slot); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) { VIR_ERROR0(_ @@ -2129,7 +1920,6 @@ phypAttachDevice(virDomainPtr domain, const char *xml) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); virDomainDeviceDefFree(dev); virDomainDefFree(def); @@ -2151,10 +1941,8 @@ phypVolumeGetKey(virConnectPtr conn, const char *name) 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; - char *char_ptr; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2166,29 +1954,10 @@ phypVolumeGetKey(virConnectPtr conn, const char *name) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed -e 's/^LV IDENTIFIER://' -e 's/ //g'"); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); - - if (exit_status < 0 || ret == NULL) { + if (exit_status < 0) VIR_FREE(ret); - goto cleanup; - } - - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - -cleanup: - VIR_FREE(cmd); - return ret; } @@ -2202,10 +1971,8 @@ phypGetStoragePoolDevice(virConnectPtr conn, char *name) 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; - char *char_ptr; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2217,29 +1984,10 @@ phypGetStoragePoolDevice(virConnectPtr conn, char *name) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed '1d; s/ //g'"); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); - - if (exit_status < 0 || ret == NULL) { + if (exit_status < 0) VIR_FREE(ret); - goto cleanup; - } - - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - -cleanup: - VIR_FREE(cmd); - return ret; } @@ -2253,7 +2001,6 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) 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 = -1; char *char_ptr; @@ -2269,15 +2016,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) virBufferAddChar(&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); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2286,7 +2025,6 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return sp_size; @@ -2302,7 +2040,6 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, 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; @@ -2316,15 +2053,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, if (system_type == HMC) virBufferAddChar(&buf, '\''); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0) { VIR_ERROR(_("Unable to create Volume: %s"), NULLSTR(ret)); @@ -2333,11 +2062,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, key = phypVolumeGetKey(conn, lvname); - if (key == NULL) - goto cleanup; - cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return key; @@ -2469,10 +2194,8 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) 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; - char *char_ptr; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2484,29 +2207,10 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed 1d"); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); - - if (exit_status < 0 || ret == NULL) { + if (exit_status < 0) VIR_FREE(ret); - goto cleanup; - } - - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - -cleanup: - VIR_FREE(cmd); - return ret; } @@ -2520,9 +2224,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) 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 *char_ptr; char *key = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virStorageVolPtr vol = NULL; @@ -2537,24 +2239,11 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed -e 's/^VOLUME GROUP://g' -e 's/ //g'"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - key = phypVolumeGetKey(conn, volname); if (key == NULL) @@ -2563,7 +2252,6 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) vol = virGetStorageVol(conn, ret, volname, key); cleanup: - VIR_FREE(cmd); VIR_FREE(ret); VIR_FREE(key); @@ -2582,7 +2270,6 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, 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; @@ -2596,15 +2283,7 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed '1,2d'"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2615,7 +2294,6 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -2724,11 +2402,9 @@ phypVolumeGetPath(virStorageVolPtr vol) 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 *path = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - char *char_ptr; char *pv; if (system_type == HMC) @@ -2742,24 +2418,11 @@ phypVolumeGetPath(virStorageVolPtr vol) virBufferVSprintf(&buf, "|sed -e 's/^VOLUME GROUP://g' -e 's/ //g'"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - pv = phypVolumeGetPhysicalVolumeByStoragePool(vol, ret); if (!pv) @@ -2771,7 +2434,6 @@ phypVolumeGetPath(virStorageVolPtr vol) } cleanup: - VIR_FREE(cmd); VIR_FREE(ret); VIR_FREE(path); @@ -2793,7 +2455,6 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, int exit_status = 0; int got = 0; int i; - char *cmd = NULL; char *ret = NULL; char *volumes_list = NULL; char *char_ptr2 = NULL; @@ -2809,15 +2470,7 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed '1,2d'"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); /* I need to parse the textual return in order to get the volumes */ if (exit_status < 0 || ret == NULL) @@ -2850,10 +2503,7 @@ cleanup: got = -1; } - - VIR_FREE(cmd); VIR_FREE(ret); - return got; } @@ -2867,7 +2517,6 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) int system_type = phyp_driver->system_type; int exit_status = 0; int nvolumes = -1; - char *cmd = NULL; char *ret = NULL; char *managed_system = phyp_driver->managed_system; int vios_id = phyp_driver->vios_id; @@ -2881,15 +2530,7 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) if (system_type == HMC) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|grep -c '^.*$'"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2901,7 +2542,6 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) nvolumes -= 2; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return nvolumes; @@ -2918,7 +2558,6 @@ phypDestroyStoragePool(virStoragePoolPtr pool) 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; @@ -2931,14 +2570,7 @@ phypDestroyStoragePool(virStoragePoolPtr pool) if (system_type == HMC) virBufferAddChar(&buf, '\''); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0) { VIR_ERROR(_("Unable to destroy Storage Pool: %s"), NULLSTR(ret)); @@ -2948,7 +2580,6 @@ phypDestroyStoragePool(virStoragePoolPtr pool) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -2965,7 +2596,6 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) 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; @@ -2979,15 +2609,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) if (system_type == HMC) virBufferAddChar(&buf, '\''); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0) { VIR_ERROR(_("Unable to create Storage Pool: %s"), NULLSTR(ret)); @@ -2997,7 +2619,6 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -3013,7 +2634,6 @@ phypNumOfStoragePools(virConnectPtr conn) int system_type = phyp_driver->system_type; int exit_status = 0; int nsp = -1; - char *cmd = NULL; char *ret = NULL; char *managed_system = phyp_driver->managed_system; int vios_id = phyp_driver->vios_id; @@ -3030,15 +2650,7 @@ phypNumOfStoragePools(virConnectPtr conn) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|grep -c '^.*$'"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3047,7 +2659,6 @@ phypNumOfStoragePools(virConnectPtr conn) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return nsp; @@ -3066,7 +2677,6 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) int exit_status = 0; int got = 0; int i; - char *cmd = NULL; char *ret = NULL; char *storage_pools = NULL; char *char_ptr2 = NULL; @@ -3080,15 +2690,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) if (system_type == HMC) virBufferAddChar(&buf, '\''); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); /* I need to parse the textual return in order to get the storage pools */ if (exit_status < 0 || ret == NULL) @@ -3121,10 +2723,7 @@ cleanup: got = -1; } - - VIR_FREE(cmd); VIR_FREE(ret); - return got; } @@ -3286,7 +2885,6 @@ phypInterfaceDestroy(virInterfacePtr iface, int slot_num = 0; int lpar_id = 0; char *char_ptr; - char *cmd = NULL; char *ret = NULL; int rv = -1; @@ -3300,15 +2898,7 @@ phypInterfaceDestroy(virInterfacePtr iface, " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,slot_num|" " sed -n '/%s/ s/^.*,//p'", iface->mac); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, iface->conn); + ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3317,7 +2907,6 @@ phypInterfaceDestroy(virInterfacePtr iface, goto cleanup; /* Getting the remote slot number */ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "lshwres "); @@ -3328,15 +2917,7 @@ phypInterfaceDestroy(virInterfacePtr iface, " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,lpar_id|" " sed -n '/%s/ s/^.*,//p'", iface->mac); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, iface->conn); + ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3345,7 +2926,6 @@ phypInterfaceDestroy(virInterfacePtr iface, goto cleanup; /* excluding interface */ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "chhwres "); @@ -3355,15 +2935,7 @@ phypInterfaceDestroy(virInterfacePtr iface, virBufferVSprintf(&buf, " -r virtualio --rsubtype eth" " --id %d -o r -s %d", lpar_id, slot_num); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, iface->conn); + ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); if (exit_status < 0 || ret != NULL) goto cleanup; @@ -3371,7 +2943,6 @@ phypInterfaceDestroy(virInterfacePtr iface, rv = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return rv; } @@ -3390,7 +2961,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, int system_type = phyp_driver->system_type; int exit_status = 0; char *char_ptr; - char *cmd = NULL; int slot = 0; char *ret = NULL; char name[PHYP_IFACENAME_SIZE]; @@ -3410,15 +2980,7 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, " -r virtualio --rsubtype slot --level slot" " -Fslot_num --filter lpar_names=%s" " |sort|tail -n 1", def->name); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3430,7 +2992,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, slot++; /* Now adding the new network interface */ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "chhwres "); @@ -3441,15 +3002,7 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, " -r virtualio --rsubtype eth" " -p %s -o a -s %d -a port_vlan_id=1," "ieee_virtual_eth=0", def->name, slot); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret != NULL) goto cleanup; @@ -3460,7 +3013,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, sleep(1); /* Getting the new interface name */ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "lshwres "); @@ -3471,19 +3023,10 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, " -r virtualio --rsubtype slot --level slot" " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " "s/^.*drc_name=//'", def->name, slot); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) { /* roll back and excluding interface if error*/ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "chhwres "); @@ -3493,23 +3036,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, virBufferVSprintf(&buf, " -r virtualio --rsubtype eth" " -p %s -o r -s %d", def->name, slot); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); goto cleanup; } memcpy(name, ret, PHYP_IFACENAME_SIZE-1); /* Getting the new interface mac addr */ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "lshwres "); @@ -3520,15 +3053,7 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, "-r virtualio --rsubtype eth --level lpar " " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " "s/^.*mac_addr=//'", def->name, slot); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3538,7 +3063,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, result = virGetInterface(conn, name, mac); cleanup: - VIR_FREE(cmd); VIR_FREE(ret); virInterfaceDefFree(def); return result; @@ -3555,7 +3079,6 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) int system_type = phyp_driver->system_type; int exit_status = 0; char *char_ptr; - char *cmd = NULL; char *ret = NULL; int slot = 0; int lpar_id = 0; @@ -3571,15 +3094,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype slot --level slot " " -F drc_name,slot_num |" " sed -n '/%s/ s/^.*,//p'", name); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3588,7 +3103,6 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) goto cleanup; /*Getting the lpar_id for the interface */ - VIR_FREE(cmd); VIR_FREE(ret); virBufferAddLit(&buf, "lshwres "); @@ -3599,15 +3113,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype slot --level slot " " -F drc_name,lpar_id |" " sed -n '/%s/ s/^.*,//p'", name); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3624,17 +3130,8 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype eth --level lpar " " -F lpar_id,slot_num,mac_addr|" " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3644,7 +3141,6 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) result = virGetInterface(conn, name, ret); cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; } @@ -3661,7 +3157,6 @@ phypInterfaceIsActive(virInterfacePtr iface) int exit_status = 0; int state = -1; char *char_ptr; - char *cmd = NULL; char *ret = NULL; virBufferAddLit(&buf, "lshwres "); @@ -3672,15 +3167,7 @@ phypInterfaceIsActive(virInterfacePtr iface) " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,state |" " sed -n '/%s/ s/^.*,//p'", iface->mac); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, iface->conn); + ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3689,7 +3176,6 @@ phypInterfaceIsActive(virInterfacePtr iface) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return state; } @@ -3706,7 +3192,6 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) int exit_status = 0; int got = 0; int i; - char *cmd = NULL; char *ret = NULL; char *networks = NULL; char *char_ptr2 = NULL; @@ -3719,14 +3204,7 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) virBufferVSprintf(&buf, " -r virtualio --rsubtype slot --level slot|" " sed '/eth/!d; /lpar_id=%d/d; s/^.*drc_name=//g'", vios_id); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); /* I need to parse the textual return in order to get the network * interfaces */ @@ -3756,7 +3234,6 @@ cleanup: for (i = 0; i < got; i++) VIR_FREE(names[i]); } - VIR_FREE(cmd); VIR_FREE(ret); return got; } @@ -3773,7 +3250,6 @@ phypNumOfInterfaces(virConnectPtr conn) int exit_status = 0; int nnets = -1; char *char_ptr; - char *cmd = NULL; char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -3784,15 +3260,7 @@ phypNumOfInterfaces(virConnectPtr conn) virBufferVSprintf(&buf, "-r virtualio --rsubtype eth --level lpar|" "grep -v lpar_id=%d|grep -c lpar_name", vios_id); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) goto cleanup; @@ -3801,7 +3269,6 @@ phypNumOfInterfaces(virConnectPtr conn) goto cleanup; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return nnets; } @@ -3813,10 +3280,8 @@ phypGetLparState(virConnectPtr conn, unsigned int lpar_id) phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; int exit_status = 0; - char *char_ptr = NULL; char *managed_system = phyp_driver->managed_system; int state = VIR_DOMAIN_NOSTATE; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -3825,23 +3290,11 @@ phypGetLparState(virConnectPtr conn, unsigned int lpar_id) if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F state --filter lpar_ids=%d", lpar_id); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return state; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - if (STREQ(ret, "Running")) state = VIR_DOMAIN_RUNNING; else if (STREQ(ret, "Not Activated")) @@ -3850,7 +3303,6 @@ phypGetLparState(virConnectPtr conn, unsigned int lpar_id) state = VIR_DOMAIN_SHUTDOWN; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return state; } @@ -3864,10 +3316,8 @@ phypDiskType(virConnectPtr conn, char *backing_device) ConnectionData *connection_data = conn->networkPrivateData; LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; - char *cmd = NULL; char *ret = NULL; int exit_status = 0; - char *char_ptr; char *managed_system = phyp_driver->managed_system; int vios_id = phyp_driver->vios_id; int disk_type = -1; @@ -3879,30 +3329,17 @@ phypDiskType(virConnectPtr conn, char *backing_device) virBufferVSprintf(&buf, " -p %d -c \"lssp -field name type " "-fmt , -all|sed -n '/%s/ {\n s/^.*,//\n p\n}'\"", vios_id, backing_device); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return disk_type; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, true); if (exit_status < 0 || ret == NULL) goto cleanup; - char_ptr = strchr(ret, '\n'); - - if (char_ptr) - *char_ptr = '\0'; - if (STREQ(ret, "LVPOOL")) disk_type = VIR_DOMAIN_DISK_TYPE_BLOCK; else if (STREQ(ret, "FBPOOL")) disk_type = VIR_DOMAIN_DISK_TYPE_FILE; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return disk_type; } @@ -3937,7 +3374,6 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) int exit_status = 0; int got = 0; int i; - char *cmd = NULL; char *ret = NULL; char *domains = NULL; char *char_ptr2 = NULL; @@ -3948,14 +3384,7 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F name,state" "|sed -n '/Not Activated/ {\n s/,.*$//\n p\n}'"); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); /* I need to parse the textual return in order to get the domains */ if (exit_status < 0 || ret == NULL) @@ -3988,10 +3417,7 @@ cleanup: got = -1; } - - VIR_FREE(cmd); VIR_FREE(ret); - return got; } @@ -4113,7 +3539,6 @@ phypDomainResume(virDomainPtr dom) int system_type = phyp_driver->system_type; char *managed_system = phyp_driver->managed_system; int exit_status = 0; - char *cmd = NULL; char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4122,14 +3547,7 @@ phypDomainResume(virDomainPtr dom) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r lpar -o on --id %d -f %s", dom->id, dom->name); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, dom->conn); + ret = phypExecBuffer(session, &buf, &exit_status, dom->conn, false); if (exit_status < 0) goto cleanup; @@ -4137,7 +3555,6 @@ phypDomainResume(virDomainPtr dom) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -4154,7 +3571,6 @@ phypDomainShutdown(virDomainPtr dom) int system_type = phyp_driver->system_type; char *managed_system = phyp_driver->managed_system; int exit_status = 0; - char *cmd = NULL; char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4162,14 +3578,7 @@ phypDomainShutdown(virDomainPtr dom) if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r lpar -o shutdown --id %d", dom->id); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, dom->conn); + ret = phypExecBuffer(session, &buf, &exit_status, dom->conn, false); if (exit_status < 0) goto cleanup; @@ -4177,7 +3586,6 @@ phypDomainShutdown(virDomainPtr dom) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -4216,7 +3624,6 @@ phypDomainDestroy(virDomainPtr dom) int system_type = phyp_driver->system_type; char *managed_system = phyp_driver->managed_system; int exit_status = 0; - char *cmd = NULL; char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4224,14 +3631,7 @@ phypDomainDestroy(virDomainPtr dom) if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r lpar --id %d", dom->id); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, dom->conn); + ret = phypExecBuffer(session, &buf, &exit_status, dom->conn, false); if (exit_status < 0) goto cleanup; @@ -4243,7 +3643,6 @@ phypDomainDestroy(virDomainPtr dom) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -4258,7 +3657,6 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) LIBSSH2_SESSION *session = connection_data->session; 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; @@ -4298,14 +3696,7 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) def->name, (int) def->mem.cur_balloon, (int) def->mem.cur_balloon, (int) def->mem.max_balloon, (int) def->vcpus, def->disks[0]->src); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return -1; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0) { VIR_ERROR(_("Unable to create LPAR. Reason: '%s'"), NULLSTR(ret)); @@ -4320,7 +3711,6 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) result = 0; cleanup: - VIR_FREE(cmd); VIR_FREE(ret); return result; @@ -4402,7 +3792,6 @@ phypDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, int system_type = phyp_driver->system_type; char *managed_system = phyp_driver->managed_system; int exit_status = 0; - char *cmd = NULL; char *ret = NULL; char operation; unsigned long ncpus = 0; @@ -4438,14 +3827,7 @@ phypDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virBufferVSprintf(&buf, " --id %d -o %c --procunits %d 2>&1 |sed " "-e 's/^.*\\([0-9][0-9]*.[0-9][0-9]*\\).*$/\\1/'", dom->id, operation, amount); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return 0; - } - cmd = virBufferContentAndReset(&buf); - - ret = phypExec(session, cmd, &exit_status, dom->conn); + ret = phypExecBuffer(session, &buf, &exit_status, dom->conn, false); if (exit_status < 0) { VIR_ERROR0(_ @@ -4453,7 +3835,6 @@ phypDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, " Contact your support to enable this feature.")); } - VIR_FREE(cmd); VIR_FREE(ret); return 0; -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
* src/phyp/phyp_driver.c (phypExecBuffer): New function. Use it throughout file for less code, and for plugging a few leaks. ---
Amazing how much copy-and-paste code this consolidated.
src/phyp/phyp_driver.c | 803 ++++++------------------------------------------ 1 files changed, 92 insertions(+), 711 deletions(-)
@@ -1823,17 +1706,8 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) 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(); - goto cleanup; - } - - VIR_FREE(cmd); VIR_FREE(ret); - - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (exit_status < 0 || ret == NULL) goto cleanup; @@ -1847,17 +1721,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) virBufferVSprintf(&buf, " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", vios_name, slot); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret);
Why are you killing the VIR_FREE(ret) here, in the hunk before you left it in. It's necessary here too.
- cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (exit_status < 0 || ret == NULL) goto cleanup;
@@ -2000,18 +1841,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
if (system_type == HMC) virBufferAddChar(&buf, '\''); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret);
Same for this VIR_FREE(ret) here, removing it produces a leak, doesn't it.
- - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2029,17 +1859,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " slot_num,backing_device|grep %s|cut -d, -f1", dev->data.disk->src); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret);
And this VIR_FREE(ret) needs to stay too.
- - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2057,17 +1877,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) " -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(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret);
And this VIR_FREE(ret) needs to stay too.
- - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (exit_status < 0 || ret == NULL) goto cleanup; @@ -2083,17 +1893,8 @@ phypAttachDevice(virDomainPtr domain, const char *xml) "\"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(); - goto cleanup; - } - - VIR_FREE(cmd); VIR_FREE(ret);
This one is left in correctly.
- - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) goto cleanup; @@ -2107,17 +1908,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", domain_name, slot); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret);
But this VIR_FREE(ret) needs to stay.
- - cmd = virBufferContentAndReset(&buf); - ret = phypExec(session, cmd, &exit_status, conn); + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
if (exit_status < 0 || ret == NULL) { VIR_ERROR0(_
ACK, with that VIR_FREEs left in. Matthias
On 04/15/2011 02:56 PM, Matthias Bolte wrote:
2011/4/14 Eric Blake <eblake@redhat.com>:
* src/phyp/phyp_driver.c (phypExecBuffer): New function. Use it throughout file for less code, and for plugging a few leaks. ---
Amazing how much copy-and-paste code this consolidated.
" -p %s -o a -s %d -d 0 -a \"adapter_type=server\"", vios_name, slot); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - goto cleanup; - } - - VIR_FREE(cmd); - VIR_FREE(ret);
Why are you killing the VIR_FREE(ret) here, in the hunk before you left it in. It's necessary here too.
Accident of rebasing. I had previously inserted VIR_FREE(ret) earlier in the function, before your patch to independently fix leaks of ret, and missed that my rebase was deleting your instances.
ACK, with that VIR_FREEs left in.
I've added those back in, and pushed 1-8. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Rather than copying and pasting lots of code, factor it into a single helper function. * src/phyp/phyp_driver.c (phypExecInt): New function. (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID) (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot) (phypGetVIOSNextSlotNumber, phypAttachDevice) (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes) (phypNumOfStoragePools, phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypNumOfInterfaces): Use it. --- src/phyp/phyp_driver.c | 316 ++++++++++-------------------------------------- 1 files changed, 67 insertions(+), 249 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index bc24b76..98d5cd6 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -228,6 +228,26 @@ phypExecBuffer(LIBSSH2_SESSION *session, virBufferPtr buf, int *exit_status, return ret; } +/* Convenience wrapper function */ +static int phypExecInt(LIBSSH2_SESSION *, virBufferPtr, virConnectPtr, int *) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +static int +phypExecInt(LIBSSH2_SESSION *session, virBufferPtr buf, virConnectPtr conn, + int *result) +{ + char *str; + int ret; + + str = phypExecBuffer(session, buf, &ret, conn, true); + if (!str || ret) { + VIR_FREE(str); + return -1; + } + ret = virStrToLong_i(str, NULL, 10, result); + VIR_FREE(str); + return ret; +} + static int phypGetSystemType(virConnectPtr conn) { @@ -255,10 +275,7 @@ phypGetVIOSPartitionID(virConnectPtr conn) phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; - char *ret = NULL; - int exit_status = 0; int id = -1; - char *char_ptr; char *managed_system = phyp_driver->managed_system; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -267,17 +284,7 @@ phypGetVIOSPartitionID(virConnectPtr conn) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferAddLit(&buf, " -r lpar -F lpar_id,lpar_env" "|sed -n '/vioserver/ {\n s/,.*$//\n p\n}'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &id) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &id); return id; } @@ -340,10 +347,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; - int exit_status = 0; int ndom = -1; - char *char_ptr; - char *ret = NULL; char *managed_system = phyp_driver->managed_system; const char *state; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", state); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &ndom); return ndom; } @@ -1298,27 +1292,14 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, { phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - int exit_status = 0; int lpar_id = -1; - char *char_ptr; - char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " --filter lpar_names=%s -F lpar_id", name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &lpar_id); return lpar_id; } @@ -1382,10 +1363,7 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *ret = NULL; - char *char_ptr; int memory = 0; - int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; if (type != 1 && type != 0) @@ -1397,17 +1375,7 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d", type ? "curr_mem" : "curr_max_mem", lpar_id); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &memory); return memory; } @@ -1419,9 +1387,6 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *ret = NULL; - char *char_ptr; - int exit_status = 0; int vcpus = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1431,17 +1396,7 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, virBufferVSprintf(&buf, " -r proc --level lpar -F %s --filter lpar_ids=%d", type ? "curr_max_procs" : "curr_procs", lpar_id); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &vcpus) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &vcpus); return vcpus; } @@ -1480,10 +1435,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, LIBSSH2_SESSION *session = connection_data->session; phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - char *ret = NULL; - char *char_ptr; int remote_slot = -1; - int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "lshwres"); @@ -1491,17 +1443,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F " "remote_slot_num --filter lpar_names=%s", lpar_name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &remote_slot) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &remote_slot); return remote_slot; } @@ -1609,9 +1551,6 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) 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 *ret = NULL; char *profile = NULL; int slot = -1; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1632,21 +1571,9 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) "virtual_serial_adapters|sed -e 's/\"//g' -e " "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" "|sort|tail -n 1", profile); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto cleanup; - - slot += 1; - -cleanup: - VIR_FREE(profile); - VIR_FREE(ret); - - return slot; + if (phypExecInt(session, &buf, conn, &slot) < 0) + return -1; + return slot + 1; } static int @@ -1779,7 +1706,6 @@ phypAttachDevice(virDomainPtr domain, const char *xml) int system_type = phyp_driver->system_type; int vios_id = phyp_driver->vios_id; int exit_status = 0; - char *char_ptr = NULL; char *ret = NULL; char *scsi_adapter = NULL; int slot = 0; @@ -1859,12 +1785,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " slot_num,backing_device|grep %s|cut -d, -f1", dev->data.disk->src); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup; /* Listing all the virtual_scsi_adapter interfaces, the new adapter must @@ -1893,10 +1814,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", domain_name, domain->id, ret, slot, vios_id, vios_name); - VIR_FREE(ret); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup; /* Finally I add the new scsi adapter to VIOS using the same slot @@ -1999,11 +1917,8 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) 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 *ret = NULL; int sp_size = -1; - char *char_ptr; virBuffer buf = VIR_BUFFER_INITIALIZER; if (system_type == HMC) @@ -2016,17 +1931,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|sed '1d; s/ //g'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &sp_size); return sp_size; } @@ -2457,7 +2362,7 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, int i; char *ret = NULL; char *volumes_list = NULL; - char *char_ptr2 = NULL; + char *char_ptr = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; if (system_type == HMC) @@ -2479,16 +2384,16 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, volumes_list = ret; while (got < nvolumes) { - char_ptr2 = strchr(volumes_list, '\n'); + char_ptr = strchr(volumes_list, '\n'); - if (char_ptr2) { - *char_ptr2 = '\0'; + if (char_ptr) { + *char_ptr = '\0'; if ((volumes[got++] = strdup(volumes_list)) == NULL) { virReportOOMError(); goto cleanup; } - char_ptr2++; - volumes_list = char_ptr2; + char_ptr++; + volumes_list = char_ptr; } else break; } @@ -2515,12 +2420,9 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) 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 = -1; - 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) @@ -2530,21 +2432,11 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) if (system_type == HMC) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|grep -c '^.*$'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) - goto cleanup; + if (phypExecInt(session, &buf, conn, &nvolumes) < 0) + return -1; /* We need to remove 2 line from the header text output */ - nvolumes -= 2; - -cleanup: - VIR_FREE(ret); - - return nvolumes; + return nvolumes - 2; } static int @@ -2632,12 +2524,9 @@ phypNumOfStoragePools(virConnectPtr conn) 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 = -1; - 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) @@ -2650,17 +2539,7 @@ phypNumOfStoragePools(virConnectPtr conn) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|grep -c '^.*$'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &nsp) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &nsp); return nsp; } @@ -2679,7 +2558,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) int i; char *ret = NULL; char *storage_pools = NULL; - char *char_ptr2 = NULL; + char *char_ptr = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; if (system_type == HMC) @@ -2699,16 +2578,16 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) storage_pools = ret; while (got < npools) { - char_ptr2 = strchr(storage_pools, '\n'); + char_ptr = strchr(storage_pools, '\n'); - if (char_ptr2) { - *char_ptr2 = '\0'; + if (char_ptr) { + *char_ptr = '\0'; if ((pools[got++] = strdup(storage_pools)) == NULL) { virReportOOMError(); goto cleanup; } - char_ptr2++; - storage_pools = char_ptr2; + char_ptr++; + storage_pools = char_ptr; } else break; } @@ -2884,7 +2763,6 @@ phypInterfaceDestroy(virInterfacePtr iface, int exit_status = 0; int slot_num = 0; int lpar_id = 0; - char *char_ptr; char *ret = NULL; int rv = -1; @@ -2898,17 +2776,10 @@ phypInterfaceDestroy(virInterfacePtr iface, " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,slot_num|" " sed -n '/%s/ s/^.*,//p'", iface->mac); - ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1) + if (phypExecInt(session, &buf, iface->conn, &slot_num) < 0) goto cleanup; /* Getting the remote slot number */ - VIR_FREE(ret); - virBufferAddLit(&buf, "lshwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -2917,17 +2788,10 @@ phypInterfaceDestroy(virInterfacePtr iface, " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,lpar_id|" " sed -n '/%s/ s/^.*,//p'", iface->mac); - ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + if (phypExecInt(session, &buf, iface->conn, &lpar_id) < 0) goto cleanup; /* excluding interface */ - VIR_FREE(ret); - virBufferAddLit(&buf, "chhwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -2960,7 +2824,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, char *managed_system = phyp_driver->managed_system; int system_type = phyp_driver->system_type; int exit_status = 0; - char *char_ptr; int slot = 0; char *ret = NULL; char name[PHYP_IFACENAME_SIZE]; @@ -2980,20 +2843,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, " -r virtualio --rsubtype slot --level slot" " -Fslot_num --filter lpar_names=%s" " |sort|tail -n 1", def->name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup; /* The next free slot itself: */ slot++; /* Now adding the new network interface */ - VIR_FREE(ret); - virBufferAddLit(&buf, "chhwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3078,7 +2934,6 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) char *managed_system = phyp_driver->managed_system; int system_type = phyp_driver->system_type; int exit_status = 0; - char *char_ptr; char *ret = NULL; int slot = 0; int lpar_id = 0; @@ -3094,17 +2949,10 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype slot --level slot " " -F drc_name,slot_num |" " sed -n '/%s/ s/^.*,//p'", name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup; /*Getting the lpar_id for the interface */ - VIR_FREE(ret); - virBufferAddLit(&buf, "lshwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3113,12 +2961,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype slot --level slot " " -F drc_name,lpar_id |" " sed -n '/%s/ s/^.*,//p'", name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + if (phypExecInt(session, &buf, conn, &lpar_id) < 0) goto cleanup; /*Getting the interface mac */ @@ -3130,7 +2973,6 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype eth --level lpar " " -F lpar_id,slot_num,mac_addr|" " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); - VIR_FREE(ret); ret = phypExecBuffer(session, &buf, &exit_status, conn, false); if (exit_status < 0 || ret == NULL) @@ -3154,10 +2996,7 @@ phypInterfaceIsActive(virInterfacePtr iface) virBuffer buf = VIR_BUFFER_INITIALIZER; char *managed_system = phyp_driver->managed_system; int system_type = phyp_driver->system_type; - int exit_status = 0; int state = -1; - char *char_ptr; - char *ret = NULL; virBufferAddLit(&buf, "lshwres "); if (system_type == HMC) @@ -3167,16 +3006,7 @@ phypInterfaceIsActive(virInterfacePtr iface) " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,state |" " sed -n '/%s/ s/^.*,//p'", iface->mac); - ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); + phypExecInt(session, &buf, iface->conn, &state); return state; } @@ -3194,7 +3024,7 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) int i; char *ret = NULL; char *networks = NULL; - char *char_ptr2 = NULL; + char *char_ptr = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; bool success = false; @@ -3214,16 +3044,16 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) networks = ret; while (got < nnames) { - char_ptr2 = strchr(networks, '\n'); + char_ptr = strchr(networks, '\n'); - if (char_ptr2) { - *char_ptr2 = '\0'; + if (char_ptr) { + *char_ptr = '\0'; if ((names[got++] = strdup(networks)) == NULL) { virReportOOMError(); goto cleanup; } - char_ptr2++; - networks = char_ptr2; + char_ptr++; + networks = char_ptr; } else { break; } @@ -3247,10 +3077,7 @@ phypNumOfInterfaces(virConnectPtr conn) 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 nnets = -1; - char *char_ptr; - char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "lshwres "); @@ -3260,16 +3087,7 @@ phypNumOfInterfaces(virConnectPtr conn) virBufferVSprintf(&buf, "-r virtualio --rsubtype eth --level lpar|" "grep -v lpar_id=%d|grep -c lpar_name", vios_id); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); + phypExecInt(session, &buf, conn, &nnets); return nnets; } @@ -3376,7 +3194,7 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) int i; char *ret = NULL; char *domains = NULL; - char *char_ptr2 = NULL; + char *char_ptr = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "lssyscfg -r lpar"); @@ -3393,16 +3211,16 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) domains = ret; while (got < nnames) { - char_ptr2 = strchr(domains, '\n'); + char_ptr = strchr(domains, '\n'); - if (char_ptr2) { - *char_ptr2 = '\0'; + if (char_ptr) { + *char_ptr = '\0'; if ((names[got++] = strdup(domains)) == NULL) { virReportOOMError(); goto cleanup; } - char_ptr2++; - domains = char_ptr2; + char_ptr++; + domains = char_ptr; } else break; } -- 1.7.4.2
2011/4/14 Eric Blake <eblake@redhat.com>:
Rather than copying and pasting lots of code, factor it into a single helper function.
* src/phyp/phyp_driver.c (phypExecInt): New function. (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID) (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot) (phypGetVIOSNextSlotNumber, phypAttachDevice) (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes) (phypNumOfStoragePools, phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypNumOfInterfaces): Use it. --- src/phyp/phyp_driver.c | 316 ++++++++++-------------------------------------- 1 files changed, 67 insertions(+), 249 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index bc24b76..98d5cd6 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -228,6 +228,26 @@ phypExecBuffer(LIBSSH2_SESSION *session, virBufferPtr buf, int *exit_status, return ret; }
+/* Convenience wrapper function */ +static int phypExecInt(LIBSSH2_SESSION *, virBufferPtr, virConnectPtr, int *) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +static int +phypExecInt(LIBSSH2_SESSION *session, virBufferPtr buf, virConnectPtr conn, + int *result) +{ + char *str; + int ret; + + str = phypExecBuffer(session, buf, &ret, conn, true); + if (!str || ret) { + VIR_FREE(str); + return -1; + } + ret = virStrToLong_i(str, NULL, 10, result);
You made the parsing stricter by passing NULL as second argument to virStrToLong_i. I don't expect it but this might be possible that this breaks the behavior of the driver.
+ VIR_FREE(str); + return ret; +} +
The rest of the patch looks fine. Matthias
On 04/15/2011 03:01 PM, Matthias Bolte wrote:
+ str = phypExecBuffer(session, buf, &ret, conn, true); + if (!str || ret) { + VIR_FREE(str); + return -1; + } + ret = virStrToLong_i(str, NULL, 10, result);
You made the parsing stricter by passing NULL as second argument to virStrToLong_i. I don't expect it but this might be possible that this breaks the behavior of the driver.
That was an intentional decision of mine (I guess I should have documented it better), since the rest of the code was getting the character after the parsed integer but doing nothing with it. In most cases, it was like the code _expected_ a newline after the integer (such as the output of a sed -c run, where that holds true), but wasn't enforcing that expectation. Should I modify the commit message and push with the newer strict behavior, or modify the code to keep the older relaxed behavior (but this time add a VIR_WARN if garbage is found after the parse)? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/4/15 Eric Blake <eblake@redhat.com>:
On 04/15/2011 03:01 PM, Matthias Bolte wrote:
+ str = phypExecBuffer(session, buf, &ret, conn, true); + if (!str || ret) { + VIR_FREE(str); + return -1; + } + ret = virStrToLong_i(str, NULL, 10, result);
You made the parsing stricter by passing NULL as second argument to virStrToLong_i. I don't expect it but this might be possible that this breaks the behavior of the driver.
That was an intentional decision of mine (I guess I should have documented it better), since the rest of the code was getting the character after the parsed integer but doing nothing with it. In most cases, it was like the code _expected_ a newline after the integer (such as the output of a sed -c run, where that holds true), but wasn't enforcing that expectation.
Should I modify the commit message and push with the newer strict behavior, or modify the code to keep the older relaxed behavior (but this time add a VIR_WARN if garbage is found after the parse)?
See my other mail in this thread for a detailed analysis. Yes, lets stick to the relaxed parsing and add a VIR_WARN. Matthias
2011/4/14 Eric Blake <eblake@redhat.com>:
Rather than copying and pasting lots of code, factor it into a single helper function.
* src/phyp/phyp_driver.c (phypExecInt): New function. (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID) (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot) (phypGetVIOSNextSlotNumber, phypAttachDevice) (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes) (phypNumOfStoragePools, phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypNumOfInterfaces): Use it. --- src/phyp/phyp_driver.c | 316 ++++++++++-------------------------------------- 1 files changed, 67 insertions(+), 249 deletions(-)
Okay lets take a look at each instance if stricter parsing is safe or not.
@@ -267,17 +284,7 @@ phypGetVIOSPartitionID(virConnectPtr conn) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferAddLit(&buf, " -r lpar -F lpar_id,lpar_env" "|sed -n '/vioserver/ {\n s/,.*$//\n p\n}'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &id) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &id); return id; }
In this case I'm not sure that your change is safe. The executed command looks like it requests a table with two columns lpar_id,lpar_env and the uses some sed construct to pick the line that contains "vioserver". Then the lpar_id is parsed from the beginning of that line. I'm not sure that this sed construct just returns the number from the beginning of the line. If that is the case that your stricter parsing it safe. But it looks like that's not that case and ignoring content after the number in the selected line is important.
@@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", state); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &ndom); return ndom; }
Here the stricter parsing will be safe as the last grep returns a count. But I wonder about the last grep. I thought it would be there to count the number of lines that start with a number, but it doesn't work: $ printf "aa\n22\n33\n" | grep -c '^[0-9]*' 3 I expected it to print 2 here. $ printf "aa\n22\n33\n\n" | grep -c '^[0-9]*' 4 So the '^[0-9]*' pattern matches every line. But $ printf "aa\n22\n33\n\n" | grep -c '^[0-9]+' 0 So the last grep here is just a wc -l.
@@ -1298,27 +1292,14 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, { phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; - int exit_status = 0; int lpar_id = -1; - char *char_ptr; - char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER;
virBufferAddLit(&buf, "lssyscfg -r lpar"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " --filter lpar_names=%s -F lpar_id", name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &lpar_id); return lpar_id; }
This one is safe too, as the requested output is just the lpar_id.
@@ -1397,17 +1375,7 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, virBufferVSprintf(&buf, " -r mem --level lpar -F %s --filter lpar_ids=%d", type ? "curr_mem" : "curr_max_mem", lpar_id); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &memory); return memory; }
This one is safe too, as the requested output is just the curr_mem or curr_max_mem.
@@ -1431,17 +1396,7 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, virBufferVSprintf(&buf, " -r proc --level lpar -F %s --filter lpar_ids=%d", type ? "curr_max_procs" : "curr_procs", lpar_id); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &vcpus) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &vcpus); return vcpus; }
Safe too.
@@ -1491,17 +1443,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F " "remote_slot_num --filter lpar_names=%s", lpar_name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &remote_slot) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &remote_slot); return remote_slot; }
Safe too.
@@ -1632,21 +1571,9 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) "virtual_serial_adapters|sed -e 's/\"//g' -e " "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'" "|sort|tail -n 1", profile); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto cleanup; - - slot += 1; - -cleanup: - VIR_FREE(profile); - VIR_FREE(ret); - - return slot; + if (phypExecInt(session, &buf, conn, &slot) < 0) + return -1; + return slot + 1; }
Chain of seds transform line of comma separated items into a list of \n separated items, then strips the trailing non-number part of each item. Finally sorts the list and takes the last number. This makes the stricter parsing safe.
@@ -1859,12 +1785,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBufferVSprintf(&buf, " slot_num,backing_device|grep %s|cut -d, -f1", dev->data.disk->src); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup;
The cut -d, -f1 makes stricter parsing safe here.
@@ -1893,10 +1814,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml) "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'", domain_name, domain->id, ret, slot, vios_id, vios_name); - VIR_FREE(ret); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup;
There is no filtering of the output before parsing it here. Therefore, I'm not sure that stricter parsing is safe here.
@@ -2016,17 +1931,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) virBufferAddChar(&buf, '\'');
virBufferVSprintf(&buf, "|sed '1d; s/ //g'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &sp_size); return sp_size; }
Here the stricter parsing might be unsafe.
@@ -2530,21 +2432,11 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) if (system_type == HMC) virBufferAddChar(&buf, '\''); virBufferVSprintf(&buf, "|grep -c '^.*$'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) - goto cleanup; + if (phypExecInt(session, &buf, conn, &nvolumes) < 0) + return -1;
Here it's safe because of grep -c, that could be replaced by wc -l, as it doesn't have a specific pattern.
@@ -2650,17 +2539,7 @@ phypNumOfStoragePools(virConnectPtr conn) virBufferAddChar(&buf, '\'');
virBufferVSprintf(&buf, "|grep -c '^.*$'"); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &nsp) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); - + phypExecInt(session, &buf, conn, &nsp); return nsp; }
Safe because of grep -c.
@@ -2898,17 +2776,10 @@ phypInterfaceDestroy(virInterfacePtr iface, " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,slot_num|" " sed -n '/%s/ s/^.*,//p'", iface->mac); - ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1) + if (phypExecInt(session, &buf, iface->conn, &slot_num) < 0) goto cleanup;
The final sed makes stricter parsing safe here.
@@ -2917,17 +2788,10 @@ phypInterfaceDestroy(virInterfacePtr iface, " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,lpar_id|" " sed -n '/%s/ s/^.*,//p'", iface->mac); - ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + if (phypExecInt(session, &buf, iface->conn, &lpar_id) < 0) goto cleanup;
Safe too.
@@ -2980,20 +2843,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, " -r virtualio --rsubtype slot --level slot" " -Fslot_num --filter lpar_names=%s" " |sort|tail -n 1", def->name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup;
Safe because of -Fslot_num.
@@ -3094,17 +2949,10 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype slot --level slot " " -F drc_name,slot_num |" " sed -n '/%s/ s/^.*,//p'", name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + if (phypExecInt(session, &buf, conn, &slot) < 0) goto cleanup;
Safe too because of the last sed.
virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3113,12 +2961,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) " -r virtualio --rsubtype slot --level slot " " -F drc_name,lpar_id |" " sed -n '/%s/ s/^.*,//p'", name); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + if (phypExecInt(session, &buf, conn, &lpar_id) < 0) goto cleanup;
Again, safe too because of the last sed.
@@ -3167,16 +3006,7 @@ phypInterfaceIsActive(virInterfacePtr iface) " -r virtualio --rsubtype eth --level lpar " " -F mac_addr,state |" " sed -n '/%s/ s/^.*,//p'", iface->mac); - ret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); + phypExecInt(session, &buf, iface->conn, &state); return state; }
Again, safe too because of the last sed.
@@ -3260,16 +3087,7 @@ phypNumOfInterfaces(virConnectPtr conn) virBufferVSprintf(&buf, "-r virtualio --rsubtype eth --level lpar|" "grep -v lpar_id=%d|grep -c lpar_name", vios_id); - ret = phypExecBuffer(session, &buf, &exit_status, conn, false); - - if (exit_status < 0 || ret == NULL) - goto cleanup; - - if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) - goto cleanup; - -cleanup: - VIR_FREE(ret); + phypExecInt(session, &buf, conn, &nnets); return nnets; }
Safe because of grep -c. In most cases we can see from the code that stricter parsing will be safe, but in some places I'm not sure. So, as long as you don't have a PHYP system at hand to really test it, I'd suggest that we stick to the relaxed parsing. Matthias
On 04/16/2011 12:18 AM, Matthias Bolte wrote:
2011/4/14 Eric Blake <eblake@redhat.com>:
Rather than copying and pasting lots of code, factor it into a single helper function.
* src/phyp/phyp_driver.c (phypExecInt): New function. (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID) (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot) (phypGetVIOSNextSlotNumber, phypAttachDevice) (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes) (phypNumOfStoragePools, phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypNumOfInterfaces): Use it. --- src/phyp/phyp_driver.c | 316 ++++++++++-------------------------------------- 1 files changed, 67 insertions(+), 249 deletions(-)
Okay lets take a look at each instance if stricter parsing is safe or not.
Thanks for doing that.
@@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", Here the stricter parsing will be safe as the last grep returns a count.
But I wonder about the last grep. I thought it would be there to count the number of lines that start with a number, but it doesn't work:
$ printf "aa\n22\n33\n" | grep -c '^[0-9]*' 3
Oops - that's a definite bug; elsewhere in the file, we consistently use "[0-9][0-9]*" when matching a non-empty string of digits ([0-9]+ is not portable to POSIX BRE).
In most cases we can see from the code that stricter parsing will be safe, but in some places I'm not sure.
So, as long as you don't have a PHYP system at hand to really test it, I'd suggest that we stick to the relaxed parsing.
Here's what I'm squashing in. Do I need to send a v3, or is this interdiff sufficient for an ack? diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c index fc4ad5c..6bb9b49 100644 --- i/src/phyp/phyp_driver.c +++ w/src/phyp/phyp_driver.c @@ -237,13 +237,16 @@ phypExecInt(LIBSSH2_SESSION *session, virBufferPtr buf, virConnectPtr conn, { char *str; int ret; + char *char_ptr; str = phypExecBuffer(session, buf, &ret, conn, true); if (!str || ret) { VIR_FREE(str); return -1; } - ret = virStrToLong_i(str, NULL, 10, result); + ret = virStrToLong_i(str, &char_ptr, 10, result); + if (ret == 0 && *char_ptr) + VIR_WARN("ignoring suffix during integer parsing of '%s'", str); VIR_FREE(str); return ret; } @@ -366,7 +369,7 @@ 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]*'", + virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9][0-9]*'", state); phypExecInt(session, &buf, conn, &ndom); return ndom; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/4/18 Eric Blake <eblake@redhat.com>:
On 04/16/2011 12:18 AM, Matthias Bolte wrote:
2011/4/14 Eric Blake <eblake@redhat.com>:
Rather than copying and pasting lots of code, factor it into a single helper function.
* src/phyp/phyp_driver.c (phypExecInt): New function. (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID) (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot) (phypGetVIOSNextSlotNumber, phypAttachDevice) (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes) (phypNumOfStoragePools, phypInterfaceDestroy) (phypInterfaceDefineXML, phypInterfaceLookupByName) (phypInterfaceIsActive, phypNumOfInterfaces): Use it. --- src/phyp/phyp_driver.c | 316 ++++++++++-------------------------------------- 1 files changed, 67 insertions(+), 249 deletions(-)
Okay lets take a look at each instance if stricter parsing is safe or not.
Thanks for doing that.
@@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) virBufferVSprintf(&buf, " -m %s", managed_system); virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'", Here the stricter parsing will be safe as the last grep returns a count.
But I wonder about the last grep. I thought it would be there to count the number of lines that start with a number, but it doesn't work:
$ printf "aa\n22\n33\n" | grep -c '^[0-9]*' 3
Oops - that's a definite bug; elsewhere in the file, we consistently use "[0-9][0-9]*" when matching a non-empty string of digits ([0-9]+ is not portable to POSIX BRE).
In most cases we can see from the code that stricter parsing will be safe, but in some places I'm not sure.
So, as long as you don't have a PHYP system at hand to really test it, I'd suggest that we stick to the relaxed parsing.
Here's what I'm squashing in. Do I need to send a v3, or is this interdiff sufficient for an ack?
diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c index fc4ad5c..6bb9b49 100644 --- i/src/phyp/phyp_driver.c +++ w/src/phyp/phyp_driver.c @@ -237,13 +237,16 @@ phypExecInt(LIBSSH2_SESSION *session, virBufferPtr buf, virConnectPtr conn, { char *str; int ret; + char *char_ptr;
str = phypExecBuffer(session, buf, &ret, conn, true); if (!str || ret) { VIR_FREE(str); return -1; } - ret = virStrToLong_i(str, NULL, 10, result); + ret = virStrToLong_i(str, &char_ptr, 10, result); + if (ret == 0 && *char_ptr) + VIR_WARN("ignoring suffix during integer parsing of '%s'", str); VIR_FREE(str); return ret; } @@ -366,7 +369,7 @@ 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]*'", + virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9][0-9]*'", state); phypExecInt(session, &buf, conn, &ndom); return ndom;
Interdiff is fine, ACK. Matthias
On 04/18/2011 10:51 AM, Matthias Bolte wrote:
2011/4/18 Eric Blake <eblake@redhat.com>:
On 04/16/2011 12:18 AM, Matthias Bolte wrote:
2011/4/14 Eric Blake <eblake@redhat.com>:
Rather than copying and pasting lots of code, factor it into a single helper function.
So, as long as you don't have a PHYP system at hand to really test it, I'd suggest that we stick to the relaxed parsing.
Here's what I'm squashing in. Do I need to send a v3, or is this interdiff sufficient for an ack?
Interdiff is fine, ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Matthias Bolte