[libvirt] [libvirt-php][PATCH 0/7] Rework libvirt_domain_new a bit

Patches for: https://bugzilla.redhat.com/show_bug.cgi?id=1513190 You can also get the patches from my github: https://github.com/zippy2/libvirt-php/commits/libvirt_domain_new Apart from usual case where I just push patches immediately, I'm gonna give others chance to review since it's not trivial change set. Michal Privoznik (7): Make installation_get_xml hurt my eyes less. Rework libvirt_domain_new a bit installation_get_xml: Don't override defaults of <on_reboot/> and friends util: Introduce VIR_FREE libvirt_domain_new: Resolve couple of memleaks installation_get_xml: Resolve couple of memleaks src: Use VIR_FREE instead of free src/libvirt-connection.c | 26 ++--- src/libvirt-domain.c | 278 +++++++++++++++++++++++++++-------------------- src/libvirt-network.c | 26 ++--- src/libvirt-node.c | 9 +- src/libvirt-nodedev.c | 52 ++++----- src/libvirt-nwfilter.c | 6 +- src/libvirt-php.c | 234 ++++++++++++++++++++------------------- src/libvirt-php.h | 2 +- src/libvirt-snapshot.c | 4 +- src/libvirt-storage.c | 20 ++-- src/util.c | 2 +- src/util.h | 6 + src/vncfunc.c | 5 +- 13 files changed, 362 insertions(+), 308 deletions(-) -- 2.13.6

This function has a lot of problems. Fix some of them: 1) long lines 2) useless argument @step 3) unclear domain XML 4) unclear difference in generated XMLs for step 1 and step 2 5) static 32KB buffer(!) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 8 +-- src/libvirt-php.c | 194 +++++++++++++++++++++++++++------------------------ src/libvirt-php.h | 2 +- 3 files changed, 107 insertions(+), 97 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 18f6888..c517dfd 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -145,8 +145,8 @@ PHP_FUNCTION(libvirt_domain_new) snprintf(tmpname, sizeof(tmpname), "%s-install", name); DPRINTF("%s: Name is '%s', memMB is %d, maxmemMB is %d\n", PHPFUNC, tmpname, (int) memMB, (int) maxmemMB); - tmp = installation_get_xml(1, - conn->conn, tmpname, memMB, maxmemMB, NULL /* arch */, NULL, vcpus, iso_image, + tmp = installation_get_xml(conn->conn, tmpname, memMB, maxmemMB, + NULL /* arch */, NULL, vcpus, iso_image, vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { @@ -194,8 +194,8 @@ PHP_FUNCTION(libvirt_domain_new) set_vnc_location(vncl TSRMLS_CC); - tmp = installation_get_xml(2, - conn->conn, name, memMB, maxmemMB, NULL /* arch */, NULL, vcpus, iso_image, + tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, + NULL /* arch */, NULL, vcpus, NULL, vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { diff --git a/src/libvirt-php.c b/src/libvirt-php.c index efbef58..51534a5 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -2248,9 +2248,11 @@ char *get_network_xml(char *mac, char *network, char *model) /* * Private function name: installation_get_xml * Since version: 0.4.5 - * Description: Function is used to generate the installation XML description to install a new domain - * Arguments: @step [int]: number of step for XML output (1 or 2) - * @conn [virConnectPtr]: libvirt connection pointer + * Description: Function is used to generate the installation XML + * description to install a new domain. If @iso_image + * is not NULL, domain XML is created so that it boots + * from it. + * Arguments: @conn [virConnectPtr]: libvirt connection pointer * @name [string]: name of the new virtual machine * @memMB [int]: memory in Megabytes * @maxmemMB [int]: maximum memory in Megabytes @@ -2265,17 +2267,20 @@ char *get_network_xml(char *mac, char *network, char *model) * @domain_flags [int]: flags for the domain installation * Returns: full XML output for installation */ -char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, int maxmemMB, char *arch, char *uuid, int vCpus, char *iso_image, - tVMDisk *disks, int numDisks, tVMNetwork *networks, int numNetworks, int domain_flags TSRMLS_DC) +char *installation_get_xml(virConnectPtr conn, char *name, int memMB, + int maxmemMB, char *arch, char *uuid, int vCpus, + char *iso_image, tVMDisk *disks, int numDisks, + tVMNetwork *networks, int numNetworks, int + domain_flags TSRMLS_DC) { int i; - char xml[32768] = { 0 }; + char *xml = NULL; char disks_xml[16384] = { 0 }; char networks_xml[16384] = { 0 }; char features[128] = { 0 }; char *tmp = NULL; char type[64] = { 0 }; - // virDomainPtr domain=NULL; + int rv; if (conn == NULL) { DPRINTF("%s: Invalid libvirt connection pointer\n", __FUNCTION__); @@ -2297,7 +2302,7 @@ char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, DPRINTF("%s: No architecture defined, got host arch of '%s'\n", __FUNCTION__, arch); } - if (access(iso_image, R_OK) != 0) { + if (iso_image && access(iso_image, R_OK) != 0) { DPRINTF("%s: Installation image %s doesn't exist\n", __FUNCTION__, iso_image); return NULL; } @@ -2324,93 +2329,98 @@ char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, free(network); } - if (step == 1) - snprintf(xml, sizeof(xml), "<domain%s>\n" - "\t<name>%s</name>\n" - "\t<currentMemory>%d</currentMemory>\n" - "\t<memory>%d</memory>\n" - "\t<uuid>%s</uuid>\n" - "\t<os>\n" - "\t\t<type arch='%s'>hvm</type>\n" - "\t\t<boot dev='cdrom'/>\n" - "\t\t<boot dev='hd'/>\n" - "\t</os>\n" - "\t<features>\n" - "\t\t%s\n" - "\t</features>\n" - "\t<clock offset=\"%s\"/>\n" - "\t<on_poweroff>destroy</on_poweroff>\n" - "\t<on_reboot>destroy</on_reboot>\n" - "\t<on_crash>destroy</on_crash>\n" - "\t<vcpu>%d</vcpu>\n" - "\t<devices>\n" - "\t\t<emulator>%s</emulator>\n" - "%s" - "\t\t<disk type='file' device='cdrom'>\n" - "\t\t\t<driver name='qemu' type='raw' />\n" - "\t\t\t<source file='%s' />\n" - "\t\t\t<target dev='hdc' bus='ide' />\n" - "\t\t\t<readonly />\n" - "\t\t</disk>\n" - "%s" - "\t\t<input type='mouse' bus='ps2' />\n" - "\t\t<graphics type='vnc' port='-1' />\n" - "\t\t<console type='pty' />\n" - "%s" - "\t\t<video>\n" - "\t\t\t<model type='cirrus' />\n" - "\t\t</video>\n" - "\t</devices>\n" - "</domain>", + if (iso_image) { + rv = asprintf(&xml, + "<domain%s>\n" + " <name>%s</name>\n" + " <currentMemory>%d</currentMemory>\n" + " <memory>%d</memory>\n" + " <uuid>%s</uuid>\n" + " <os>\n" + " <type arch='%s'>hvm</type>\n" + " <boot dev='cdrom'/>\n" + " <boot dev='hd'/>\n" + " </os>\n" + " <features>\n" + " %s\n" + " </features>\n" + " <clock offset=\"%s\"/>\n" + " <on_poweroff>destroy</on_poweroff>\n" + " <on_reboot>destroy</on_reboot>\n" + " <on_crash>destroy</on_crash>\n" + " <vcpu>%d</vcpu>\n" + " <devices>\n" + " <emulator>%s</emulator>\n" + " %s" + " <disk type='file' device='cdrom'>\n" + " <driver name='qemu' type='raw' />\n" + " <source file='%s' />\n" + " <target dev='hdc' bus='ide' />\n" + " <readonly />\n" + " </disk>\n" + " %s" + " <input type='mouse' bus='ps2' />\n" + " <graphics type='vnc' port='-1' />\n" + " <console type='pty' />\n" + " %s" + " <video>\n" + " <model type='cirrus' />\n" + " </video>\n" + " </devices>\n" + "</domain>", type, name, memMB * 1024, maxmemMB * 1024, uuid, arch, features, (domain_flags & DOMAIN_FLAG_CLOCK_LOCALTIME ? "localtime" : "utc"), - vCpus, connection_get_emulator(conn, arch TSRMLS_CC), disks_xml, iso_image, networks_xml, - (domain_flags & DOMAIN_FLAG_SOUND_AC97 ? "\t\t<sound model='ac97'/>\n" : "") - ); - else - if (step == 2) - snprintf(xml, sizeof(xml), "<domain%s>\n" - "\t<name>%s</name>\n" - "\t<currentMemory>%d</currentMemory>\n" - "\t<memory>%d</memory>\n" - "\t<uuid>%s</uuid>\n" - "\t<os>\n" - "\t\t<type arch='%s'>hvm</type>\n" - "\t\t<boot dev='hd'/>\n" - "\t</os>\n" - "\t<features>\n" - "\t\t%s\n" - "\t</features>\n" - "\t<clock offset=\"%s\"/>\n" - "\t<on_poweroff>destroy</on_poweroff>\n" - "\t<on_reboot>destroy</on_reboot>\n" - "\t<on_crash>destroy</on_crash>\n" - "\t<vcpu>%d</vcpu>\n" - "\t<devices>\n" - "\t\t<emulator>%s</emulator>\n" - "%s" - "\t\t<disk type='file' device='cdrom'>\n" - "\t\t\t<driver name='qemu' type='raw' />\n" - "\t\t\t<target dev='hdc' bus='ide' />\n" - "\t\t\t<readonly />\n" - "\t\t</disk>\n" - "%s" - "\t\t<input type='mouse' bus='ps2' />\n" - "\t\t<graphics type='vnc' port='-1' />\n" - "\t\t<console type='pty' />\n" - "%s" - "\t\t<video>\n" - "\t\t\t<model type='cirrus' />\n" - "\t\t</video>\n" - "\t</devices>\n" - "</domain>", - type, name, memMB * 1024, maxmemMB * 1024, uuid, arch, features, - (domain_flags & DOMAIN_FLAG_CLOCK_LOCALTIME ? "localtime" : "utc"), - vCpus, connection_get_emulator(conn, arch TSRMLS_CC), disks_xml, networks_xml, - (domain_flags & DOMAIN_FLAG_SOUND_AC97 ? "\t\t<sound model='ac97'/>\n" : "") - ); + vCpus, connection_get_emulator(conn, arch TSRMLS_CC), disks_xml, + iso_image, networks_xml, + (domain_flags & DOMAIN_FLAG_SOUND_AC97 ? "<sound model='ac97'/>\n" : "")); + } else { + rv = asprintf(&xml, + "<domain%s>\n" + " <name>%s</name>\n" + " <currentMemory>%d</currentMemory>\n" + " <memory>%d</memory>\n" + " <uuid>%s</uuid>\n" + " <os>\n" + " <type arch='%s'>hvm</type>\n" + " <boot dev='hd'/>\n" + " </os>\n" + " <features>\n" + " %s\n" + " </features>\n" + " <clock offset=\"%s\"/>\n" + " <on_poweroff>destroy</on_poweroff>\n" + " <on_reboot>destroy</on_reboot>\n" + " <on_crash>destroy</on_crash>\n" + " <vcpu>%d</vcpu>\n" + " <devices>\n" + " <emulator>%s</emulator>\n" + " %s" + " <disk type='file' device='cdrom'>\n" + " <driver name='qemu' type='raw' />\n" + " <target dev='hdc' bus='ide' />\n" + " <readonly />\n" + " </disk>\n" + " %s" + " <input type='mouse' bus='ps2' />\n" + " <graphics type='vnc' port='-1' />\n" + " <console type='pty' />\n" + " %s" + " <video>\n" + " <model type='cirrus' />\n" + " </video>\n" + " </devices>\n" + "</domain>", + type, name, memMB * 1024, maxmemMB * 1024, uuid, arch, features, + (domain_flags & DOMAIN_FLAG_CLOCK_LOCALTIME ? "localtime" : "utc"), + vCpus, connection_get_emulator(conn, arch TSRMLS_CC), disks_xml, + networks_xml, + (domain_flags & DOMAIN_FLAG_SOUND_AC97 ? "<sound model='ac97'/>\n" : "")); + } - return (strlen(xml) > 0) ? strdup(xml) : NULL; + if (rv < 0) + return NULL; + + return xml; } /* diff --git a/src/libvirt-php.h b/src/libvirt-php.h index aea43a2..e6c22b2 100644 --- a/src/libvirt-php.h +++ b/src/libvirt-php.h @@ -175,7 +175,7 @@ int set_logfile(char *filename, long maxsize TSRMLS_DC); char *get_string_from_xpath(char *xml, char *xpath, zval **val, int *retVal); char **get_array_from_xpath(char *xml, char *xpath, int *num); void parse_array(zval *arr, tVMDisk *disk, tVMNetwork *network); -char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, +char *installation_get_xml(virConnectPtr conn, char *name, int memMB, int maxmemMB, char *arch, char *uuid, int vCpus, char *iso_image, tVMDisk *disks, int numDisks, tVMNetwork *networks, int numNetworks, -- 2.13.6

On Thu, Dec 07, 2017 at 10:22:56AM +0100, Michal Privoznik wrote:
This function has a lot of problems. Fix some of them:
1) long lines 2) useless argument @step 3) unclear domain XML 4) unclear difference in generated XMLs for step 1 and step 2 5) static 32KB buffer(!)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 8 +-- src/libvirt-php.c | 194 +++++++++++++++++++++++++++------------------------ src/libvirt-php.h | 2 +- 3 files changed, 107 insertions(+), 97 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 18f6888..c517dfd 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -145,8 +145,8 @@ PHP_FUNCTION(libvirt_domain_new)
snprintf(tmpname, sizeof(tmpname), "%s-install", name); DPRINTF("%s: Name is '%s', memMB is %d, maxmemMB is %d\n", PHPFUNC, tmpname, (int) memMB, (int) maxmemMB); - tmp = installation_get_xml(1, - conn->conn, tmpname, memMB, maxmemMB, NULL /* arch */, NULL, vcpus, iso_image, + tmp = installation_get_xml(conn->conn, tmpname, memMB, maxmemMB, + NULL /* arch */, NULL, vcpus, iso_image,
The legacy 'arch' commentary is IMHO useless and should be dropped, since you can read the function signature and the same amount of information.
vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { @@ -194,8 +194,8 @@ PHP_FUNCTION(libvirt_domain_new)
set_vnc_location(vncl TSRMLS_CC);
- tmp = installation_get_xml(2, - conn->conn, name, memMB, maxmemMB, NULL /* arch */, NULL, vcpus, iso_image, + tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, + NULL /* arch */, NULL, vcpus, NULL,
same here...
vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { diff --git a/src/libvirt-php.c b/src/libvirt-php.c index efbef58..51534a5 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -2248,9 +2248,11 @@ char *get_network_xml(char *mac, char *network, char *model) /* * Private function name: installation_get_xml * Since version: 0.4.5 - * Description: Function is used to generate the installation XML description to install a new domain - * Arguments: @step [int]: number of step for XML output (1 or 2) - * @conn [virConnectPtr]: libvirt connection pointer + * Description: Function is used to generate the installation XML + * description to install a new domain. If @iso_image + * is not NULL, domain XML is created so that it boots + * from it. + * Arguments: @conn [virConnectPtr]: libvirt connection pointer * @name [string]: name of the new virtual machine * @memMB [int]: memory in Megabytes * @maxmemMB [int]: maximum memory in Megabytes @@ -2265,17 +2267,20 @@ char *get_network_xml(char *mac, char *network, char *model) * @domain_flags [int]: flags for the domain installation * Returns: full XML output for installation */ -char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, int maxmemMB, char *arch, char *uuid, int vCpus, char *iso_image, - tVMDisk *disks, int numDisks, tVMNetwork *networks, int numNetworks, int domain_flags TSRMLS_DC) +char *installation_get_xml(virConnectPtr conn, char *name, int memMB, + int maxmemMB, char *arch, char *uuid, int vCpus, + char *iso_image, tVMDisk *disks, int numDisks, + tVMNetwork *networks, int numNetworks, int + domain_flags TSRMLS_DC) { int i; - char xml[32768] = { 0 }; + char *xml = NULL; char disks_xml[16384] = { 0 }; char networks_xml[16384] = { 0 }; char features[128] = { 0 }; char *tmp = NULL; char type[64] = { 0 }; - // virDomainPtr domain=NULL; + int rv;
if (conn == NULL) { DPRINTF("%s: Invalid libvirt connection pointer\n", __FUNCTION__); @@ -2297,7 +2302,7 @@ char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, DPRINTF("%s: No architecture defined, got host arch of '%s'\n", __FUNCTION__, arch); }
- if (access(iso_image, R_OK) != 0) { + if (iso_image && access(iso_image, R_OK) != 0) { DPRINTF("%s: Installation image %s doesn't exist\n", __FUNCTION__, iso_image); return NULL; } @@ -2324,93 +2329,98 @@ char *installation_get_xml(int step, virConnectPtr conn, char *name, int memMB, free(network); }
- if (step == 1) - snprintf(xml, sizeof(xml), "<domain%s>\n" - "\t<name>%s</name>\n" - "\t<currentMemory>%d</currentMemory>\n" - "\t<memory>%d</memory>\n" - "\t<uuid>%s</uuid>\n" - "\t<os>\n" - "\t\t<type arch='%s'>hvm</type>\n" - "\t\t<boot dev='cdrom'/>\n" - "\t\t<boot dev='hd'/>\n" - "\t</os>\n" - "\t<features>\n" - "\t\t%s\n" - "\t</features>\n" - "\t<clock offset=\"%s\"/>\n" - "\t<on_poweroff>destroy</on_poweroff>\n" - "\t<on_reboot>destroy</on_reboot>\n" - "\t<on_crash>destroy</on_crash>\n" - "\t<vcpu>%d</vcpu>\n" - "\t<devices>\n" - "\t\t<emulator>%s</emulator>\n" - "%s" - "\t\t<disk type='file' device='cdrom'>\n" - "\t\t\t<driver name='qemu' type='raw' />\n" - "\t\t\t<source file='%s' />\n" - "\t\t\t<target dev='hdc' bus='ide' />\n" - "\t\t\t<readonly />\n" - "\t\t</disk>\n" - "%s" - "\t\t<input type='mouse' bus='ps2' />\n" - "\t\t<graphics type='vnc' port='-1' />\n" - "\t\t<console type='pty' />\n" - "%s" - "\t\t<video>\n" - "\t\t\t<model type='cirrus' />\n" - "\t\t</video>\n" - "\t</devices>\n" - "</domain>", + if (iso_image) { + rv = asprintf(&xml, + "<domain%s>\n" + " <name>%s</name>\n" + " <currentMemory>%d</currentMemory>\n" + " <memory>%d</memory>\n" + " <uuid>%s</uuid>\n" + " <os>\n" + " <type arch='%s'>hvm</type>\n" + " <boot dev='cdrom'/>\n" + " <boot dev='hd'/>\n" + " </os>\n" + " <features>\n" + " %s\n" + " </features>\n" + " <clock offset=\"%s\"/>\n" + " <on_poweroff>destroy</on_poweroff>\n" + " <on_reboot>destroy</on_reboot>\n" + " <on_crash>destroy</on_crash>\n" + " <vcpu>%d</vcpu>\n" + " <devices>\n" + " <emulator>%s</emulator>\n" + " %s" + " <disk type='file' device='cdrom'>\n" + " <driver name='qemu' type='raw' />\n" + " <source file='%s' />\n" + " <target dev='hdc' bus='ide' />\n" + " <readonly />\n" + " </disk>\n" + " %s" + " <input type='mouse' bus='ps2' />\n" + " <graphics type='vnc' port='-1' />\n" + " <console type='pty' />\n" + " %s" + " <video>\n" + " <model type='cirrus' />\n" + " </video>\n" + " </devices>\n" + "</domain>",
Good for the start, but a virBuffer equivalent is much needed to be introduced in the future...(I know, patches are welcome...) Reviewed-by: Erik Skultety <eskultet@redhat.com>

Firstly, this API is creating $domName-install for installation and at the same time it defines $domName (but never runs it). This is not very optimal - libvirt can handle two definitions for a single domain (active and inactive ones). Secondly, this function is leaking domain objects on any error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 65 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index c517dfd..3c4c683 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -63,8 +63,8 @@ PHP_FUNCTION(libvirt_domain_new) { php_libvirt_connection *conn = NULL; php_libvirt_domain *res_domain = NULL; - virDomainPtr domain2 = NULL; virDomainPtr domain = NULL; + virDomainPtr domainUpdated = NULL; zval *zconn; char *arch = NULL; strsize_t arch_len; @@ -85,10 +85,9 @@ PHP_FUNCTION(libvirt_domain_new) zval *data; HashPosition pointer; char vncl[2048] = { 0 }; - char tmpname[1024] = { 0 }; char *xml = NULL; int retval = 0; - char *uuid = NULL; + char uuid[VIR_UUID_STRING_BUFLEN] = { 0 }; zend_long flags = 0; int fd = -1; @@ -143,9 +142,7 @@ PHP_FUNCTION(libvirt_domain_new) } VIRT_FOREACH_END(); numNets = i; - snprintf(tmpname, sizeof(tmpname), "%s-install", name); - DPRINTF("%s: Name is '%s', memMB is %d, maxmemMB is %d\n", PHPFUNC, tmpname, (int) memMB, (int) maxmemMB); - tmp = installation_get_xml(conn->conn, tmpname, memMB, maxmemMB, + tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, NULL /* arch */, NULL, vcpus, iso_image, vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); @@ -155,25 +152,37 @@ PHP_FUNCTION(libvirt_domain_new) RETURN_FALSE; } - domain = virDomainCreateXML(conn->conn, tmp, 0); + domain = virDomainDefineXML(conn->conn, tmp); if (domain == NULL) { - set_error_if_unset("Cannot create installation domain from the XML description" TSRMLS_CC); - DPRINTF("%s: Cannot create installation domain from the XML description (%s): %s\n", PHPFUNC, LIBVIRT_G(last_error), tmp); + set_error_if_unset("Cannot define domain from the XML description" TSRMLS_CC); + DPRINTF("%s: Cannot define domain from the XML description (%s): %s\n", PHPFUNC, LIBVIRT_G(last_error), tmp); RETURN_FALSE; } + if (virDomainCreate(domain) < 0) { + DPRINTF("%s: Cannot create domain: %s\n", PHPFUNC, LIBVIRT_G(last_error)); + set_error_if_unset("Cannot create domain" TSRMLS_CC); + goto error; + } + xml = virDomainGetXMLDesc(domain, 0); if (!xml) { - DPRINTF("%s: Cannot get the XML description\n", PHPFUNC); + DPRINTF("%s: Cannot get the XML description: %s\n", PHPFUNC, LIBVIRT_G(last_error)); set_error_if_unset("Cannot get the XML description" TSRMLS_CC); - RETURN_FALSE; + goto error; + } + + if (virDomainGetUUIDString(domain, uuid) < 0) { + DPRINTF("%s: Cannot get domain UUID: %s\n", PHPFUNC, LIBVIRT_G(last_error)); + set_error_if_unset("Cannot get domain UUID" TSRMLS_CC); + goto error; } tmp = get_string_from_xpath(xml, "//domain/devices/graphics[@type='vnc']/@port", NULL, &retval); if (retval < 0) { DPRINTF("%s: Cannot get port from XML description\n", PHPFUNC); set_error_if_unset("Cannot get port from XML description" TSRMLS_CC); - RETURN_FALSE; + goto error; } snprintf(vncl, sizeof(vncl), "%s:%s", virConnectGetHostname(conn->conn), tmp); @@ -195,23 +204,25 @@ PHP_FUNCTION(libvirt_domain_new) set_vnc_location(vncl TSRMLS_CC); tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, - NULL /* arch */, NULL, vcpus, NULL, + NULL /* arch */, uuid, vcpus, NULL, vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { DPRINTF("%s: Cannot get installation XML, step 2\n", PHPFUNC); set_error("Cannot get installation XML, step 2" TSRMLS_CC); - virDomainFree(domain); - RETURN_FALSE; + goto error; } - domain2 = virDomainDefineXML(conn->conn, tmp); - if (domain2 == NULL) { - set_error_if_unset("Cannot define domain from the XML description" TSRMLS_CC); - DPRINTF("%s: Cannot define domain from the XML description (name = '%s', uuid = '%s', error = '%s')\n", PHPFUNC, name, uuid, LIBVIRT_G(last_error)); - RETURN_FALSE; + domainUpdated = virDomainDefineXML(conn->conn, tmp); + if (domainUpdated == NULL) { + set_error_if_unset("Cannot update domain definition" TSRMLS_CC); + DPRINTF("%s: Cannot update domain definition " + "(name = '%s', uuid = '%s', error = '%s')\n", + PHPFUNC, name, uuid, LIBVIRT_G(last_error)); + goto error; } - virDomainFree(domain2); + virDomainFree(domainUpdated); + domainUpdated = NULL; res_domain = (php_libvirt_domain *)emalloc(sizeof(php_libvirt_domain)); res_domain->domain = domain; @@ -221,6 +232,18 @@ PHP_FUNCTION(libvirt_domain_new) resource_change_counter(INT_RESOURCE_DOMAIN, conn->conn, res_domain->domain, 1 TSRMLS_CC); VIRT_REGISTER_RESOURCE(res_domain, le_libvirt_domain); + return; + + error: + if (domain) { + if (virDomainIsActive(domain) > 0) + virDomainDestroy(domain); + virDomainUndefine(domain); + virDomainFree(domain); + } + if (domainUpdated) + virDomainFree(domainUpdated); + RETURN_FALSE; } /* -- 2.13.6

On Thu, Dec 07, 2017 at 10:22:57AM +0100, Michal Privoznik wrote:
Firstly, this API is creating $domName-install for installation and at the same time it defines $domName (but never runs it). This is not very optimal - libvirt can handle two definitions for a single domain (active and inactive ones).
Secondly, this function is leaking domain objects on any error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
...
src/libvirt-domain.c | 65 +++++++++++++++++++++++++++++++++++-----------------
snprintf(vncl, sizeof(vncl), "%s:%s", virConnectGetHostname(conn->conn), tmp); @@ -195,23 +204,25 @@ PHP_FUNCTION(libvirt_domain_new) set_vnc_location(vncl TSRMLS_CC);
tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, - NULL /* arch */, NULL, vcpus, NULL, + NULL /* arch */, uuid, vcpus, NULL, vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { DPRINTF("%s: Cannot get installation XML, step 2\n", PHPFUNC); set_error("Cannot get installation XML, step 2" TSRMLS_CC);
Since you dropped the step1/2 notation from installation_get_xml in patch 1, this error message should be rephrased as well, I'd maybe prefer to have that change in patch 1, but I don't really care that much. Reviewed-by: Erik Skultety <eskultet@redhat.com>

We don't really need to override the defaults because libvirt chooses sane ones. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-php.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 51534a5..307412e 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -2345,9 +2345,6 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, " %s\n" " </features>\n" " <clock offset=\"%s\"/>\n" - " <on_poweroff>destroy</on_poweroff>\n" - " <on_reboot>destroy</on_reboot>\n" - " <on_crash>destroy</on_crash>\n" " <vcpu>%d</vcpu>\n" " <devices>\n" " <emulator>%s</emulator>\n" @@ -2388,9 +2385,6 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, " %s\n" " </features>\n" " <clock offset=\"%s\"/>\n" - " <on_poweroff>destroy</on_poweroff>\n" - " <on_reboot>destroy</on_reboot>\n" - " <on_crash>destroy</on_crash>\n" " <vcpu>%d</vcpu>\n" " <devices>\n" " <emulator>%s</emulator>\n" -- 2.13.6

On Thu, Dec 07, 2017 at 10:22:58AM +0100, Michal Privoznik wrote:
We don't really need to override the defaults because libvirt chooses sane ones.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-php.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 51534a5..307412e 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -2345,9 +2345,6 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, " %s\n" " </features>\n" " <clock offset=\"%s\"/>\n" - " <on_poweroff>destroy</on_poweroff>\n" - " <on_reboot>destroy</on_reboot>\n"
You should preserve the <on_reboot> element here, since the default action in libvirt for that is restart, but we're installing a new guest so you want to destroy it in order to alter the post-install configuration.
- " <on_crash>destroy</on_crash>\n" " <vcpu>%d</vcpu>\n" " <devices>\n" " <emulator>%s</emulator>\n" @@ -2388,9 +2385,6 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, " %s\n" " </features>\n" " <clock offset=\"%s\"/>\n" - " <on_poweroff>destroy</on_poweroff>\n" - " <on_reboot>destroy</on_reboot>\n" - " <on_crash>destroy</on_crash>\n"
This chunk is fine though, since the configuration has been modified and it's the final modification before the first post-install bootup. Reviewed-by: Erik Skultety <eskultet@redhat.com>

Just like libvirt has it. After freeing pointer set it to NULL to avoid double frees. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util.h b/src/util.h index fcd4075..3af77d4 100644 --- a/src/util.h +++ b/src/util.h @@ -36,6 +36,12 @@ # define DPRINTF(fmt, ...) \ debugPrint(debugSource, fmt, __VA_ARGS__) +# define VIR_FREE(ptr) \ + do { \ + free(ptr); \ + ptr = NULL; \ + } while (0) + # define ARRAY_CARDINALITY(array) (sizeof(array) / sizeof(array[0])) # define IS_BIGENDIAN (*(uint16_t *)"\0\xff" < 0x100) -- 2.13.6

On Thu, Dec 07, 2017 at 10:22:59AM +0100, Michal Privoznik wrote:
Just like libvirt has it. After freeing pointer set it to NULL to avoid double frees.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>

There are still some, but it's definitely better now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3c4c683..6ae6c0a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -86,6 +86,7 @@ PHP_FUNCTION(libvirt_domain_new) HashPosition pointer; char vncl[2048] = { 0 }; char *xml = NULL; + char *hostname = NULL; int retval = 0; char uuid[VIR_UUID_STRING_BUFLEN] = { 0 }; zend_long flags = 0; @@ -149,14 +150,14 @@ PHP_FUNCTION(libvirt_domain_new) if (tmp == NULL) { DPRINTF("%s: Cannot get installation XML\n", PHPFUNC); set_error("Cannot get installation XML" TSRMLS_CC); - RETURN_FALSE; + goto error; } domain = virDomainDefineXML(conn->conn, tmp); if (domain == NULL) { set_error_if_unset("Cannot define domain from the XML description" TSRMLS_CC); DPRINTF("%s: Cannot define domain from the XML description (%s): %s\n", PHPFUNC, LIBVIRT_G(last_error), tmp); - RETURN_FALSE; + goto error; } if (virDomainCreate(domain) < 0) { @@ -178,22 +179,31 @@ PHP_FUNCTION(libvirt_domain_new) goto error; } + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//domain/devices/graphics[@type='vnc']/@port", NULL, &retval); if (retval < 0) { DPRINTF("%s: Cannot get port from XML description\n", PHPFUNC); set_error_if_unset("Cannot get port from XML description" TSRMLS_CC); goto error; } + VIR_FREE(xml); - snprintf(vncl, sizeof(vncl), "%s:%s", virConnectGetHostname(conn->conn), tmp); + hostname = virConnectGetHostname(conn->conn); + if (!hostname) { + DPRINTF("%s: Cannot get hostname\n", PHPFUNC); + set_error_if_unset("Cannot get hostname" TSRMLS_CC); + goto error; + } + + snprintf(vncl, sizeof(vncl), "%s:%s", hostname, tmp); DPRINTF("%s: Trying to connect to '%s'\n", PHPFUNC, vncl); #ifndef EXTWIN - if ((fd = connect_socket(virConnectGetHostname(conn->conn), tmp, 0, 0, flags & DOMAIN_FLAG_TEST_LOCAL_VNC)) < 0) { + if ((fd = connect_socket(hostname, tmp, 0, 0, flags & DOMAIN_FLAG_TEST_LOCAL_VNC)) < 0) { DPRINTF("%s: Cannot connect to '%s'\n", PHPFUNC, vncl); snprintf(vncl, sizeof(vncl), "Connection failed, port %s is most likely forbidden on firewall (iptables) on the host (%s)" " or the emulator VNC server is bound to localhost address only.", - tmp, virConnectGetHostname(conn->conn)); + tmp, hostname); } else { close(fd); DPRINTF("%s: Connection to '%s' successfull (%s local connection)\n", PHPFUNC, vncl, @@ -203,6 +213,7 @@ PHP_FUNCTION(libvirt_domain_new) set_vnc_location(vncl TSRMLS_CC); + VIR_FREE(tmp); tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, NULL /* arch */, uuid, vcpus, NULL, vmDisks, numDisks, vmNetworks, numNets, @@ -232,6 +243,9 @@ PHP_FUNCTION(libvirt_domain_new) resource_change_counter(INT_RESOURCE_DOMAIN, conn->conn, res_domain->domain, 1 TSRMLS_CC); VIRT_REGISTER_RESOURCE(res_domain, le_libvirt_domain); + VIR_FREE(vmDisks); + VIR_FREE(vmNetworks); + VIR_FREE(tmp); return; error: @@ -243,6 +257,9 @@ PHP_FUNCTION(libvirt_domain_new) } if (domainUpdated) virDomainFree(domainUpdated); + VIR_FREE(vmDisks); + VIR_FREE(vmNetworks); + VIR_FREE(tmp); RETURN_FALSE; } -- 2.13.6

On Thu, Dec 07, 2017 at 10:23:00AM +0100, Michal Privoznik wrote:
There are still some, but it's definitely better now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-domain.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
...
set_vnc_location(vncl TSRMLS_CC);
+ VIR_FREE(tmp); tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, NULL /* arch */, uuid, vcpus, NULL, vmDisks, numDisks, vmNetworks, numNets, @@ -232,6 +243,9 @@ PHP_FUNCTION(libvirt_domain_new) resource_change_counter(INT_RESOURCE_DOMAIN, conn->conn, res_domain->domain, 1 TSRMLS_CC);
VIRT_REGISTER_RESOURCE(res_domain, le_libvirt_domain); + VIR_FREE(vmDisks); + VIR_FREE(vmNetworks); + VIR_FREE(tmp); return;
You still leak @hostname. Also there are multiple occurrences of @hostname within the module which don't appear to be freed either, but since I have no idea what zend_parse_parameters (one of the APIs where engineering definitely wasn't done right...) does, I can't be sure unless I ask valgrind... Reviewed-by: Erik Skultety <eskultet@redhat.com>
error: @@ -243,6 +257,9 @@ PHP_FUNCTION(libvirt_domain_new) } if (domainUpdated) virDomainFree(domainUpdated); + VIR_FREE(vmDisks); + VIR_FREE(vmNetworks); + VIR_FREE(tmp); RETURN_FALSE; }
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

There are still some, but it's definitely better now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-php.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 307412e..1525834 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -2411,6 +2411,8 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, (domain_flags & DOMAIN_FLAG_SOUND_AC97 ? "<sound model='ac97'/>\n" : "")); } + VIR_FREE(tmp); + VIR_FREE(arch); if (rv < 0) return NULL; -- 2.13.6

On Thu, Dec 07, 2017 at 10:23:01AM +0100, Michal Privoznik wrote:
There are still some, but it's definitely better now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
This should be squashed into the previous patch. Reviewed-by: Erik Skultety <eskultet@redhat.com>

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-connection.c | 26 +++---- src/libvirt-domain.c | 182 ++++++++++++++++++++++++----------------------- src/libvirt-network.c | 26 +++---- src/libvirt-node.c | 9 +-- src/libvirt-nodedev.c | 52 +++++++------- src/libvirt-nwfilter.c | 6 +- src/libvirt-php.c | 44 ++++++------ src/libvirt-snapshot.c | 4 +- src/libvirt-storage.c | 20 +++--- src/util.c | 2 +- src/vncfunc.c | 5 +- 11 files changed, 189 insertions(+), 187 deletions(-) diff --git a/src/libvirt-connection.c b/src/libvirt-connection.c index 2d59d82..086cd57 100644 --- a/src/libvirt-connection.c +++ b/src/libvirt-connection.c @@ -231,7 +231,7 @@ PHP_FUNCTION(libvirt_connect_get_uri) RETURN_FALSE; VIRT_RETVAL_STRING(uri); - free(uri); + VIR_FREE(uri); } /* @@ -255,7 +255,7 @@ PHP_FUNCTION(libvirt_connect_get_hostname) RETURN_FALSE; VIRT_RETVAL_STRING(hostname); - free(hostname); + VIR_FREE(hostname); } /* @@ -326,8 +326,8 @@ PHP_FUNCTION(libvirt_connect_get_capabilities) VIRT_RETVAL_STRING(tmp); } - free(caps); - free(tmp); + VIR_FREE(caps); + VIR_FREE(tmp); } /* @@ -358,7 +358,7 @@ PHP_FUNCTION(libvirt_connect_get_emulator) } VIRT_RETVAL_STRING(tmp); - free(tmp); + VIR_FREE(tmp); } /* @@ -571,7 +571,7 @@ PHP_FUNCTION(libvirt_connect_get_sysinfo) RETURN_FALSE; VIRT_RETVAL_STRING(sysinfo); - free(sysinfo); + VIR_FREE(sysinfo); } /* @@ -631,10 +631,10 @@ PHP_FUNCTION(libvirt_connect_get_information) DPRINTF("%s: Got connection URI of %s...\n", PHPFUNC, tmp); array_init(return_value); VIRT_ADD_ASSOC_STRING(return_value, "uri", tmp ? tmp : "unknown"); - free(tmp); + VIR_FREE(tmp); tmp = virConnectGetHostname(conn->conn); VIRT_ADD_ASSOC_STRING(return_value, "hostname", tmp ? tmp : "unknown"); - free(tmp); + VIR_FREE(tmp); if ((virConnectGetVersion(conn->conn, &hvVer) == 0) && (type = virConnectGetType(conn->conn))) { VIRT_ADD_ASSOC_STRING(return_value, "hypervisor", (char *)type); @@ -759,10 +759,10 @@ PHP_FUNCTION(libvirt_connect_get_machine_types) VIRT_ADD_ASSOC_STRING(arr4, "maxCpus", numTmp); add_assoc_zval_ex(arr2, key, strlen(key) + 1, arr4); - free(numTmp); + VIR_FREE(numTmp); } - free(ret3[k]); + VIR_FREE(ret3[k]); } } @@ -793,10 +793,10 @@ PHP_FUNCTION(libvirt_connect_get_machine_types) VIRT_ADD_ASSOC_STRING(arr4, "maxCpus", numTmp); add_assoc_zval_ex(arr3, key, strlen(key) + 1, arr4); - free(numTmp); + VIR_FREE(numTmp); } - free(ret3[k]); + VIR_FREE(ret3[k]); } add_assoc_zval_ex(arr2, ret2[j], strlen(ret2[j]) + 1, arr3); @@ -806,7 +806,7 @@ PHP_FUNCTION(libvirt_connect_get_machine_types) add_assoc_zval_ex(return_value, ret[i], strlen(ret[i]) + 1, arr2); } - free(ret[i]); + VIR_FREE(ret[i]); } } } diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ae6c0a..4c36d85 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -399,8 +399,8 @@ PHP_FUNCTION(libvirt_domain_get_xml_desc) VIRT_RETVAL_STRING(tmp); } - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); } /* @@ -415,6 +415,7 @@ PHP_FUNCTION(libvirt_domain_get_disk_devices) php_libvirt_domain *domain = NULL; zval *zdomain; char *xml; + char *tmp; int retval = -1; GET_DOMAIN_FROM_ARGS("r", &zdomain); @@ -429,8 +430,9 @@ PHP_FUNCTION(libvirt_domain_get_disk_devices) array_init(return_value); - free(get_string_from_xpath(xml, "//domain/devices/disk/target/@dev", &return_value, &retval)); - free(xml); + tmp = get_string_from_xpath(xml, "//domain/devices/disk/target/@dev", &return_value, &retval); + VIR_FREE(tmp); + VIR_FREE(xml); if (retval < 0) add_assoc_long(return_value, "error_code", (long)retval); @@ -450,6 +452,7 @@ PHP_FUNCTION(libvirt_domain_get_interface_devices) php_libvirt_domain *domain = NULL; zval *zdomain; char *xml; + char *tmp; int retval = -1; GET_DOMAIN_FROM_ARGS("r", &zdomain); @@ -464,8 +467,9 @@ PHP_FUNCTION(libvirt_domain_get_interface_devices) array_init(return_value); - free(get_string_from_xpath(xml, "//domain/devices/interface/target/@dev", &return_value, &retval)); - free(xml); + tmp = get_string_from_xpath(xml, "//domain/devices/interface/target/@dev", &return_value, &retval); + VIR_FREE(tmp); + VIR_FREE(xml); if (retval < 0) add_assoc_long(return_value, "error_code", (long)retval); @@ -566,11 +570,11 @@ PHP_FUNCTION(libvirt_domain_change_memory) dom = virDomainDefineXML(conn->conn, new_xml); if (dom == NULL) { - free(xml); + VIR_FREE(xml); efree(new_xml); RETURN_FALSE; } - free(xml); + VIR_FREE(xml); efree(new_xml); res_domain = (php_libvirt_domain *)emalloc(sizeof(php_libvirt_domain)); @@ -648,11 +652,11 @@ PHP_FUNCTION(libvirt_domain_change_boot_devices) dom = virDomainDefineXML(conn->conn, new_xml); if (dom == NULL) { DPRINTF("%s: Function failed, restoring original XML\n", PHPFUNC); - free(xml); + VIR_FREE(xml); efree(newXml); RETURN_FALSE; } - free(xml); + VIR_FREE(xml); efree(newXml); res_domain = (php_libvirt_domain *)emalloc(sizeof(php_libvirt_domain)); @@ -713,7 +717,7 @@ PHP_FUNCTION(libvirt_domain_disk_add) } tmp = get_string_from_xpath(xml, xpath, NULL, &retval); if (tmp != NULL) { - free(tmp); + VIR_FREE(tmp); if (asprintf(&tmp, "Domain already has image <i>%s</i> connected", img) < 0) set_error("Out of memory" TSRMLS_CC); else @@ -721,14 +725,14 @@ PHP_FUNCTION(libvirt_domain_disk_add) goto error; } - free(xpath); + VIR_FREE(xpath); if (asprintf(&xpath, "//domain/devices/disk/target[@dev='%s']/./@dev", dev) < 0) { set_error("Out of memory" TSRMLS_CC); goto error; } tmp = get_string_from_xpath(xml, newXml, NULL, &retval); if (tmp != NULL) { - free(tmp); + VIR_FREE(tmp); if (asprintf(&tmp, "Domain already has device <i>%s</i> connected", dev) < 0) set_error("Out of memory" TSRMLS_CC); else @@ -752,15 +756,15 @@ PHP_FUNCTION(libvirt_domain_disk_add) goto error; } - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_TRUE; error: - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_FALSE; } @@ -823,15 +827,15 @@ PHP_FUNCTION(libvirt_domain_disk_remove) goto error; } - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_TRUE; error: - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_FALSE; } @@ -883,7 +887,7 @@ PHP_FUNCTION(libvirt_domain_nic_add) } tmp = get_string_from_xpath(xml, xpath, NULL, &retval); if (tmp) { - free(tmp); + VIR_FREE(tmp); if (asprintf(&tmp, "Domain already has NIC device with MAC address <i>%s</i> connected", mac) < 0) set_error("Out of memory" TSRMLS_CC); else @@ -918,15 +922,15 @@ PHP_FUNCTION(libvirt_domain_nic_add) goto error; } - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_TRUE; error: - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_FALSE; } @@ -967,7 +971,7 @@ PHP_FUNCTION(libvirt_domain_nic_remove) } tmp = get_string_from_xpath(xml, xpath, NULL, &retval); if (!tmp) { - free(tmp); + VIR_FREE(tmp); if (asprintf(&tmp, "Domain has no such interface with mac %s", mac) < 0) set_error("Out of memory" TSRMLS_CC); else @@ -989,15 +993,15 @@ PHP_FUNCTION(libvirt_domain_nic_remove) goto error; } - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_TRUE; error: - free(tmp); - free(xpath); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xpath); + VIR_FREE(xml); RETURN_FALSE; } @@ -1583,7 +1587,7 @@ PHP_FUNCTION(libvirt_domain_xml_from_native) } VIRT_RETVAL_STRING(xml); - free(xml); + VIR_FREE(xml); } /* @@ -1616,7 +1620,7 @@ PHP_FUNCTION(libvirt_domain_xml_to_native) } VIRT_RETVAL_STRING(config_data); - free(config_data); + VIR_FREE(config_data); } /* @@ -2218,14 +2222,14 @@ PHP_FUNCTION(libvirt_domain_xml_xpath) array_init(return_value); - free(get_string_from_xpath(xml, (char *)zpath, &return_value, &rc)); + tmp = get_string_from_xpath(xml, (char *)zpath, &return_value, &rc); if (return_value < 0) { - free(xml); + VIR_FREE(xml); RETURN_FALSE; } - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); if (rc == 0) RETURN_FALSE; @@ -2279,12 +2283,12 @@ PHP_FUNCTION(libvirt_domain_get_block_info) } if (retval == 0) { - free(xpath); + VIR_FREE(xpath); if (asprintf(&xpath, "//domain/devices/disk/target[@dev='%s']/../source/@file", dev) < 0) { set_error("Out of memory" TSRMLS_CC); goto error; } - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, xpath, NULL, &retval); if (retval < 0) { set_error("Cannot get XPath expression result for file storage" TSRMLS_CC); @@ -2313,12 +2317,12 @@ PHP_FUNCTION(libvirt_domain_get_block_info) else VIRT_ADD_ASSOC_STRING(return_value, "partition", tmp); - free(xpath); + VIR_FREE(xpath); if (asprintf(&xpath, "//domain/devices/disk/target[@dev='%s']/../driver/@type", dev) < 0) { set_error("Out of memory" TSRMLS_CC); goto error; } - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, xpath, NULL, &retval); if (tmp != NULL) VIRT_ADD_ASSOC_STRING(return_value, "type", tmp); @@ -2327,15 +2331,15 @@ PHP_FUNCTION(libvirt_domain_get_block_info) LONGLONG_ASSOC(return_value, "allocation", info.allocation); LONGLONG_ASSOC(return_value, "physical", info.physical); - free(xpath); - free(tmp); - free(xml); + VIR_FREE(xpath); + VIR_FREE(tmp); + VIR_FREE(xml); return; error: - free(xpath); - free(tmp); - free(xml); + VIR_FREE(xpath); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_FALSE; } @@ -2387,8 +2391,8 @@ PHP_FUNCTION(libvirt_domain_get_network_info) VIRT_ADD_ASSOC_STRING(return_value, "mac", mac); VIRT_ADD_ASSOC_STRING(return_value, "network", tmp); - free(tmp); - free(xpath); + VIR_FREE(tmp); + VIR_FREE(xpath); if (asprintf(&xpath, "//domain/devices/interface[@type='network']/mac[@address='%s']/../model/@type", mac) < 0) { set_error("Out of memory" TSRMLS_CC); @@ -2400,15 +2404,15 @@ PHP_FUNCTION(libvirt_domain_get_network_info) else VIRT_ADD_ASSOC_STRING(return_value, "nic_type", "default"); - free(xml); - free(xpath); - free(tmp); + VIR_FREE(xml); + VIR_FREE(xpath); + VIR_FREE(tmp); return; error: - free(xml); - free(xpath); - free(tmp); + VIR_FREE(xml); + VIR_FREE(xpath); + VIR_FREE(tmp); RETURN_FALSE; } @@ -2487,7 +2491,7 @@ PHP_FUNCTION(libvirt_domain_get_metadata) RETURN_NULL(); } else { VIRT_RETVAL_STRING(ret); - free(ret); + VIR_FREE(ret); } } @@ -2696,16 +2700,16 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) VIRT_ZVAL_STRINGL(return_value, buf, fsize); efree(buf); - free(tmp); - free(xml); - free(pathDup); + VIR_FREE(tmp); + VIR_FREE(xml); + VIR_FREE(pathDup); return; error: efree(buf); - free(tmp); - free(xml); - free(pathDup); + VIR_FREE(tmp); + VIR_FREE(xml); + VIR_FREE(pathDup); RETURN_FALSE; #else set_error("Function is not supported on Windows systems" TSRMLS_CC); @@ -2794,11 +2798,11 @@ PHP_FUNCTION(libvirt_domain_get_screenshot_api) VIRT_ADD_ASSOC_STRING(return_value, "mime", mime); } - free(mime); + VIR_FREE(mime); return; error: - free(mime); + VIR_FREE(mime); if (fd != -1) { unlink(file); close(fd); @@ -2846,7 +2850,7 @@ PHP_FUNCTION(libvirt_domain_get_screen_dimensions) DPRINTF("%s: hostname = %s, port = %s\n", PHPFUNC, hostname, tmp); ret = vnc_get_dimensions(hostname, tmp, &width, &height); - free(tmp); + VIR_FREE(tmp); if (ret != 0) { char error[1024] = { 0 }; if (ret == -9) @@ -2862,13 +2866,13 @@ PHP_FUNCTION(libvirt_domain_get_screen_dimensions) add_assoc_long(return_value, "width", (long)width); add_assoc_long(return_value, "height", (long)height); - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); return; error: - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_FALSE; #else set_error("Function is not supported on Windows systems" TSRMLS_CC); @@ -2927,13 +2931,13 @@ PHP_FUNCTION(libvirt_domain_send_keys) goto error; } - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_TRUE; error: - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_FALSE; #else set_error("Function is not supported on Windows systems" TSRMLS_CC); @@ -3045,13 +3049,13 @@ PHP_FUNCTION(libvirt_domain_send_pointer_event) goto error; } - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_TRUE; error: - free(tmp); - free(xml); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_FALSE; #else set_error("Function is not supported on Windows systems" TSRMLS_CC); @@ -3114,7 +3118,7 @@ PHP_FUNCTION(libvirt_domain_qemu_agent_command) RETURN_FALSE; VIRT_RETVAL_STRING(ret); - free(ret); + VIR_FREE(ret); } /* @@ -3189,7 +3193,7 @@ PHP_FUNCTION(libvirt_list_domains) for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); DPRINTF("%s: Found inactive domain %s\n", PHPFUNC, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); } @@ -3260,7 +3264,7 @@ PHP_FUNCTION(libvirt_list_domain_resources) VIRT_REGISTER_LIST_RESOURCE(domain); resource_change_counter(INT_RESOURCE_DOMAIN, conn->conn, res_domain->domain, 1 TSRMLS_CC); } - free(names[i]); + VIR_FREE(names[i]); } efree(names); } @@ -3377,7 +3381,7 @@ PHP_FUNCTION(libvirt_list_inactive_domains) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); } diff --git a/src/libvirt-network.c b/src/libvirt-network.c index 464b060..98ae539 100644 --- a/src/libvirt-network.c +++ b/src/libvirt-network.c @@ -108,8 +108,8 @@ PHP_FUNCTION(libvirt_network_get_xml_desc) VIRT_RETVAL_STRING(tmp); } - free(xml); - free(tmp); + VIR_FREE(xml); + VIR_FREE(tmp); } /* @@ -189,7 +189,7 @@ PHP_FUNCTION(libvirt_network_get_bridge) } VIRT_RETVAL_STRING(name); - free(name); + VIR_FREE(name); } /* @@ -344,14 +344,14 @@ PHP_FUNCTION(libvirt_network_get_information) VIRT_ADD_ASSOC_STRING(return_value, "dhcp_end", dhcp_end); } - free(dhcp_end); - free(dhcp_start); - free(dev); - free(mode); - free(netmask); - free(ipaddr); - free(name); - free(xml); + VIR_FREE(dhcp_end); + VIR_FREE(dhcp_start); + VIR_FREE(dev); + VIR_FREE(mode); + VIR_FREE(netmask); + VIR_FREE(ipaddr); + VIR_FREE(name); + VIR_FREE(xml); } /* @@ -556,7 +556,7 @@ PHP_FUNCTION(libvirt_list_networks) for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); @@ -575,7 +575,7 @@ PHP_FUNCTION(libvirt_list_networks) for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); diff --git a/src/libvirt-node.c b/src/libvirt-node.c index 783f2fb..1074e09 100644 --- a/src/libvirt-node.c +++ b/src/libvirt-node.c @@ -133,8 +133,7 @@ PHP_FUNCTION(libvirt_node_get_cpu_stats) VIRT_ADD_ASSOC_STRING(return_value, "cpu", "unknown"); } - free(params); - params = NULL; + VIR_FREE(params); } /* @@ -227,8 +226,7 @@ PHP_FUNCTION(libvirt_node_get_cpu_stats_for_each_cpu) add_assoc_zval(return_value, "times", time_array); - free(params); - params = NULL; + VIR_FREE(params); } /* @@ -275,8 +273,7 @@ PHP_FUNCTION(libvirt_node_get_mem_stats) add_assoc_long(return_value, "time", time(NULL)); - free(params); - params = NULL; + VIR_FREE(params); } /* diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 03f7257..5cfe428 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -100,7 +100,7 @@ PHP_FUNCTION(libvirt_nodedev_capabilities) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); @@ -140,8 +140,8 @@ PHP_FUNCTION(libvirt_nodedev_get_xml_desc) else VIRT_RETVAL_STRING(tmp); - free(xml); - free(tmp); + VIR_FREE(xml); + VIR_FREE(tmp); } /* @@ -185,7 +185,7 @@ PHP_FUNCTION(libvirt_nodedev_get_information) VIRT_ADD_ASSOC_STRING(return_value, "name", tmp); /* Get parent name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/parent", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "parent", tmp); @@ -198,105 +198,105 @@ PHP_FUNCTION(libvirt_nodedev_get_information) /* System capability is having hardware and firmware sub-blocks */ if (strcmp(cap, "system") == 0) { /* Get hardware vendor */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/hardware/vendor", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "hardware_vendor", tmp); /* Get hardware version */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/hardware/version", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "hardware_version", tmp); /* Get hardware serial */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/hardware/serial", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "hardware_serial", tmp); /* Get hardware UUID */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/hardware/uuid", NULL, &retval); if (tmp != NULL) VIRT_ADD_ASSOC_STRING(return_value, "hardware_uuid", tmp); /* Get firmware vendor */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/firmware/vendor", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "firmware_vendor", tmp); /* Get firmware version */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/firmware/version", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "firmware_version", tmp); /* Get firmware release date */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/firmware/release_date", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "firmware_release_date", tmp); } /* Get product_id */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/product/@id", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "product_id", tmp); /* Get product_name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/product", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "product_name", tmp); /* Get vendor_id */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/vendor/@id", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "vendor_id", tmp); /* Get vendor_name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/vendor", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "vendor_name", tmp); /* Get driver name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/driver/name", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "driver_name", tmp); /* Get driver name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/interface", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "interface_name", tmp); /* Get driver name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/address", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "address", tmp); /* Get driver name */ - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(xml, "//device/capability/capability/@type", NULL, &retval); if ((tmp != NULL) && (retval > 0)) VIRT_ADD_ASSOC_STRING(return_value, "capabilities", tmp); - free(cap); - free(tmp); - free(xml); + VIR_FREE(cap); + VIR_FREE(tmp); + VIR_FREE(xml); return; error: - free(cap); - free(tmp); - free(xml); + VIR_FREE(cap); + VIR_FREE(tmp); + VIR_FREE(xml); RETURN_FALSE; } @@ -333,7 +333,7 @@ PHP_FUNCTION(libvirt_list_nodedevs) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); diff --git a/src/libvirt-nwfilter.c b/src/libvirt-nwfilter.c index 87dbb0b..cd082a5 100644 --- a/src/libvirt-nwfilter.c +++ b/src/libvirt-nwfilter.c @@ -130,8 +130,8 @@ PHP_FUNCTION(libvirt_nwfilter_get_xml_desc) else VIRT_RETVAL_STRING(tmp); - free(xml); - free(tmp); + VIR_FREE(xml); + VIR_FREE(tmp); } /* @@ -403,7 +403,7 @@ PHP_FUNCTION(libvirt_list_nwfilters) for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 1525834..21cec77 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -683,7 +683,7 @@ void free_tokens(tTokenizer t) int i; for (i = 0; i < t.numTokens; i++) - free(t.tokens[i]); + VIR_FREE(t.tokens[i]); } /* @@ -1178,9 +1178,9 @@ int is_local_connection(virConnectPtr conn) freeaddrinfo(info); if (lv_hostname) - free(lv_hostname); + VIR_FREE(lv_hostname); if (result) - free(result); + VIR_FREE(result); return ret; #else @@ -1634,10 +1634,10 @@ PHP_FUNCTION(libvirt_image_remove) if (strcmp(name, hostname) != 0) { snprintf(msg, sizeof(msg), "%s works only on local systems!", PHPFUNC); set_error(msg TSRMLS_CC); - free(hostname); + VIR_FREE(hostname); RETURN_FALSE; } - free(hostname); + VIR_FREE(hostname); if (unlink(image) != 0) { snprintf(msg, sizeof(msg), "An error occured while unlinking %s: %d (%s)", image, errno, strerror(errno)); @@ -1711,8 +1711,7 @@ char *get_string_from_xpath(char *xml, char *xpath, zval **val, int *retVal) if ((value = (char *) xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1))) { snprintf(key, sizeof(key), "%d", i); VIRT_ADD_ASSOC_STRING(*val, key, value); - free(value); - value = NULL; + VIR_FREE(value); ret++; } } @@ -1892,7 +1891,7 @@ int get_subnet_bits(char *ip) if (binary[i] != '1') break; } - free(binary); + VIR_FREE(binary); return i - skip; } @@ -1922,11 +1921,14 @@ long get_next_free_numeric_value(virDomainPtr domain, char *xpath) // int array_count; zval *data; long max_slot = -1; + char *tmp; xml = virDomainGetXMLDesc(domain, VIR_DOMAIN_XML_INACTIVE); output = (zval *)emalloc(sizeof(zval)); array_init(output); - free(get_string_from_xpath(xml, xpath, &output, &retval)); + + tmp = get_string_from_xpath(xml, xpath, &output, &retval); + VIR_FREE(tmp); arr_hash = Z_ARRVAL_P(output); // array_count = zend_hash_num_elements(arr_hash); @@ -1946,7 +1948,7 @@ long get_next_free_numeric_value(virDomainPtr domain, char *xpath) } VIRT_FOREACH_END(); efree(output); - free(xml); + VIR_FREE(xml); return max_slot + 1; } @@ -1993,9 +1995,9 @@ char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC) tmp = NULL; DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, ret); cleanup: - free(tmpArch); - free(caps); - free(tmp); + VIR_FREE(tmpArch); + VIR_FREE(caps); + VIR_FREE(tmp); return ret; } @@ -2041,7 +2043,7 @@ char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC) DPRINTF("%s: No emulator found. Trying next location ...\n", __FUNCTION__); snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/emulator", arch); DPRINTF("%s: Applying xPath '%s' to capabilities XML output\n", __FUNCTION__, xpath); - free(tmp); + VIR_FREE(tmp); tmp = get_string_from_xpath(caps, xpath, NULL, &retval); if (!tmp || retval < 0) { DPRINTF("%s: None emulator found\n", __FUNCTION__); @@ -2053,9 +2055,9 @@ char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC) tmp = NULL; DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, ret); cleanup: - free(tmpArch); - free(caps); - free(tmp); + VIR_FREE(tmpArch); + VIR_FREE(caps); + VIR_FREE(tmp); return ret; } @@ -2088,8 +2090,8 @@ char *connection_get_arch(virConnectPtr conn TSRMLS_DC) DPRINTF("%s: Host CPU architecture is '%s'\n", __FUNCTION__, ret); cleanup: - free(caps); - free(tmp); + VIR_FREE(caps); + VIR_FREE(tmp); return ret; } @@ -2317,7 +2319,7 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, if (disk != NULL) strcat(disks_xml, disk); - free(disk); + VIR_FREE(disk); } for (i = 0; i < numNetworks; i++) { @@ -2326,7 +2328,7 @@ char *installation_get_xml(virConnectPtr conn, char *name, int memMB, if (network != NULL) strcat(networks_xml, network); - free(network); + VIR_FREE(network); } if (iso_image) { diff --git a/src/libvirt-snapshot.c b/src/libvirt-snapshot.c index eb2e6cd..1388d88 100644 --- a/src/libvirt-snapshot.c +++ b/src/libvirt-snapshot.c @@ -152,7 +152,7 @@ PHP_FUNCTION(libvirt_domain_snapshot_get_xml) RETURN_FALSE; VIRT_RETVAL_STRING(xml); - free(xml); + VIR_FREE(xml); } /* @@ -237,7 +237,7 @@ PHP_FUNCTION(libvirt_list_domain_snapshots) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } } efree(names); diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index 191f60a..8245227 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -165,7 +165,7 @@ PHP_FUNCTION(libvirt_storagepool_list_volumes) RETURN_FALSE; for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); @@ -326,8 +326,8 @@ PHP_FUNCTION(libvirt_storagepool_get_xml_desc) VIRT_RETVAL_STRING(tmp); } - free(xml); - free(tmp); + VIR_FREE(xml); + VIR_FREE(tmp); } /* @@ -704,7 +704,7 @@ PHP_FUNCTION(libvirt_storagevolume_get_path) RETURN_FALSE; VIRT_RETVAL_STRING(retval); - free(retval); + VIR_FREE(retval); } /* @@ -774,8 +774,8 @@ PHP_FUNCTION(libvirt_storagevolume_get_xml_desc) VIRT_RETVAL_STRING(tmp); } - free(xml); - free(tmp); + VIR_FREE(xml); + VIR_FREE(tmp); } /* @@ -1036,7 +1036,7 @@ PHP_FUNCTION(libvirt_list_storagepools) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); @@ -1052,7 +1052,7 @@ PHP_FUNCTION(libvirt_list_storagepools) for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); } @@ -1088,7 +1088,7 @@ PHP_FUNCTION(libvirt_list_active_storagepools) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); } @@ -1124,7 +1124,7 @@ PHP_FUNCTION(libvirt_list_inactive_storagepools) array_init(return_value); for (i = 0; i < count; i++) { VIRT_ADD_NEXT_INDEX_STRING(return_value, names[i]); - free(names[i]); + VIR_FREE(names[i]); } efree(names); } diff --git a/src/util.c b/src/util.c index cb8ccbe..d183aee 100644 --- a/src/util.c +++ b/src/util.c @@ -53,7 +53,7 @@ void debugPrint(const char *source, datetime = get_datetime(); fprintf(stderr, "[%s libvirt-php/%s ]: ", datetime, source); - free(datetime); + VIR_FREE(datetime); if (fmt) { va_start(args, fmt); diff --git a/src/vncfunc.c b/src/vncfunc.c index c1430d1..f50cef8 100644 --- a/src/vncfunc.c +++ b/src/vncfunc.c @@ -566,8 +566,7 @@ vnc_read_server_init(int sfd) params = vnc_parse_fb_params(buf, len + 24); cleanup: - free(buf); - buf = NULL; + VIR_FREE(buf); return params; } @@ -709,7 +708,7 @@ vnc_raw_to_bmp(char *infile, char *outfile, int width, int height) } } - free(pixels); + VIR_FREE(pixels); close(fd2); close(fd); return 0; -- 2.13.6

On Thu, Dec 07, 2017 at 10:23:02AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-connection.c | 26 +++---- src/libvirt-domain.c | 182 ++++++++++++++++++++++++----------------------- src/libvirt-network.c | 26 +++---- src/libvirt-node.c | 9 +-- src/libvirt-nodedev.c | 52 +++++++------- src/libvirt-nwfilter.c | 6 +- src/libvirt-php.c | 44 ++++++------ src/libvirt-snapshot.c | 4 +- src/libvirt-storage.c | 20 +++--- src/util.c | 2 +- src/vncfunc.c | 5 +- 11 files changed, 189 insertions(+), 187 deletions(-)
Reviewed-by: Erik Skultety <eskultet@redhat.com>

On Thu, Dec 07, 2017 at 10:22:55AM +0100, Michal Privoznik wrote:
Patches for: https://bugzilla.redhat.com/show_bug.cgi?id=1513190
You can also get the patches from my github:
https://github.com/zippy2/libvirt-php/commits/libvirt_domain_new
Apart from usual case where I just push patches immediately, I'm gonna give others chance to review since it's not trivial change set.
Since you're doing some basic cleanup, I'd suggest adding a patch to remove single line commentaries, especially some old/testing assignments, expressions, etc. Erik
participants (2)
-
Erik Skultety
-
Michal Privoznik