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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.