[Libvir] PATCH: Support network interface model in Xen and QEMU driver

This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax src/qemu_conf.c | 57 ++++++++++++++++++-- src/qemu_conf.h | 2 src/xend_internal.c | 7 ++ src/xm_internal.c | 22 +++++++ src/xml.c | 8 ++ tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args | 1 tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml | 26 +++++++++ tests/qemuxml2argvtest.c | 1 tests/qemuxml2xmltest.c | 1 tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr | 2 tests/sexpr2xmldata/sexpr2xml-net-e1000.xml | 32 +++++++++++ tests/sexpr2xmltest.c | 1 tests/testutils.c | 31 ++++++---- tests/xmconfigdata/test-paravirt-net-e1000.cfg | 12 ++++ tests/xmconfigdata/test-paravirt-net-e1000.xml | 28 +++++++++ tests/xmconfigtest.c | 1 tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr | 1 tests/xml2sexprdata/xml2sexpr-net-e1000.xml | 30 ++++++++++ tests/xml2sexprtest.c | 1 19 files changed, 247 insertions(+), 17 deletions(-) Dan. Index: src/qemu_conf.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_conf.c,v retrieving revision 1.50 diff -u -p -r1.50 qemu_conf.c --- src/qemu_conf.c 28 Apr 2008 15:14:59 -0000 1.50 +++ src/qemu_conf.c 29 Apr 2008 17:15:31 -0000 @@ -718,6 +718,7 @@ static int qemudParseInterfaceXML(virCon xmlChar *script = NULL; xmlChar *address = NULL; xmlChar *port = NULL; + xmlChar *model = NULL; net->type = QEMUD_NET_USER; @@ -779,6 +780,8 @@ static int qemudParseInterfaceXML(virCon (net->type == QEMUD_NET_ETHERNET) && xmlStrEqual(cur->name, BAD_CAST "script")) { script = xmlGetProp(cur, BAD_CAST "path"); + } else if (xmlStrEqual (cur->name, BAD_CAST "model")) { + model = xmlGetProp (cur, BAD_CAST "type"); } } cur = cur->next; @@ -938,6 +941,39 @@ static int qemudParseInterfaceXML(virCon xmlFree(address); } + /* NIC model (see -net nic,model=?). We only check that it looks + * reasonable, not that it is a supported NIC type. FWIW kvm + * supports these types as of April 2008: + * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio + */ + if (model != NULL) { + int i, len, char_ok; + + len = xmlStrlen (model); + if (len >= QEMUD_MODEL_MAX_LEN) { + qemudReportError (conn, NULL, NULL, VIR_ERR_INVALID_ARG, + _("Model name '%s' is too long"), model); + goto error; + } + for (i = 0; i < len; ++i) { + char_ok = + (model[i] >= '0' && model[i] <= '9') || + (model[i] >= 'a' && model[i] <= 'z') || + (model[i] >= 'A' && model[i] <= 'Z') || model[i] == '_'; + if (!char_ok) { + qemudReportError (conn, NULL, NULL, VIR_ERR_INVALID_ARG, + _("Model name contains invalid characters")); + goto error; + } + } + strncpy (net->model, (const char*) model, len); + net->model[len] = '\0'; + + xmlFree (model); + model = NULL; + } else + net->model[0] = '\0'; + return 0; error: @@ -953,6 +989,8 @@ static int qemudParseInterfaceXML(virCon xmlFree(script); if (bridge) xmlFree(bridge); + if (model) + xmlFree(model); return -1; } @@ -2283,13 +2321,22 @@ int qemudBuildCommandLine(virConnectPtr } else { int vlan = 0; while (net) { + char model[100]; char nic[100]; - if (snprintf(nic, sizeof(nic), "nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d", + if (net->model[0] != '\0') { + if (snprintf (model, sizeof (model), ",model=%s", net->model) + >= sizeof (model)) + goto error; + } else + model[0] = '\0'; + + if (snprintf(nic, sizeof(nic), + "nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s", net->mac[0], net->mac[1], net->mac[2], net->mac[3], net->mac[4], net->mac[5], - vlan) >= sizeof(nic)) + vlan, model) >= sizeof(nic)) goto error; if (!((*argv)[++n] = strdup("-net"))) @@ -3411,7 +3458,6 @@ static int qemudGenerateXMLChar(virBuffe virBufferVSprintf(buf, " <source mode='connect' service='%s'/>\n", dev->srcData.udp.connectService); } - break; case QEMUD_CHR_SRC_TYPE_TCP: @@ -3625,6 +3671,11 @@ char *qemudGenerateXML(virConnectPtr con net->dst.socket.port); } + if (net->model && net->model[0] != '\0') { + virBufferVSprintf(&buf, " <model type='%s'/>\n", + net->model); + } + virBufferAddLit(&buf, " </interface>\n"); net = net->next; Index: src/qemu_conf.h =================================================================== RCS file: /data/cvs/libvirt/src/qemu_conf.h,v retrieving revision 1.23 diff -u -p -r1.23 qemu_conf.h --- src/qemu_conf.h 25 Apr 2008 20:46:13 -0000 1.23 +++ src/qemu_conf.h 29 Apr 2008 17:15:32 -0000 @@ -68,6 +68,7 @@ struct qemud_vm_disk_def { }; #define QEMUD_MAC_ADDRESS_LEN 6 +#define QEMUD_MODEL_MAX_LEN 10 #define QEMUD_OS_TYPE_MAX_LEN 10 #define QEMUD_OS_ARCH_MAX_LEN 10 #define QEMUD_OS_MACHINE_MAX_LEN 10 @@ -97,6 +98,7 @@ enum qemud_vm_net_forward_type { struct qemud_vm_net_def { int type; unsigned char mac[QEMUD_MAC_ADDRESS_LEN]; + char model[QEMUD_MODEL_MAX_LEN]; union { struct { char ifname[BR_IFNAME_MAXLEN]; Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.182 diff -u -p -r1.182 xend_internal.c --- src/xend_internal.c 28 Apr 2008 15:14:59 -0000 1.182 +++ src/xend_internal.c 29 Apr 2008 17:15:34 -0000 @@ -1893,9 +1893,10 @@ xend_parse_sexp_desc(virConnectPtr conn, free(drvName); free(drvType); } else if (sexpr_lookup(node, "device/vif")) { - const char *tmp2; + const char *tmp2, *model; tmp2 = sexpr_node(node, "device/vif/script"); tmp = sexpr_node(node, "device/vif/bridge"); + model = sexpr_node(node, "device/vif/model"); if ((tmp2 && strstr(tmp2, "bridge")) || tmp) { virBufferAddLit(&buf, " <interface type='bridge'>\n"); if (tmp != NULL) @@ -1924,6 +1925,10 @@ xend_parse_sexp_desc(virConnectPtr conn, virBufferVSprintf(&buf, " <script path='%s'/>\n", tmp2); + if (model) + virBufferVSprintf(&buf, " <model type='%s'/>\n", + model); + virBufferAddLit(&buf, " </interface>\n"); vif_index++; } else if (sexpr_lookup(node, "device/vfb")) { Index: src/xm_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xm_internal.c,v retrieving revision 1.72 diff -u -p -r1.72 xm_internal.c --- src/xm_internal.c 28 Apr 2008 15:14:59 -0000 1.72 +++ src/xm_internal.c 29 Apr 2008 17:15:37 -0000 @@ -845,6 +845,7 @@ char *xenXMDomainFormatXML(virConnectPtr while (list) { int type = -1; char script[PATH_MAX]; + char model[10]; char ip[16]; char mac[18]; char bridge[50]; @@ -854,6 +855,7 @@ char *xenXMDomainFormatXML(virConnectPtr mac[0] = '\0'; script[0] = '\0'; ip[0] = '\0'; + model[0] = '\0'; if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) goto skipnic; @@ -886,6 +888,12 @@ char *xenXMDomainFormatXML(virConnectPtr len = PATH_MAX-1; strncpy(script, data, len); script[len] = '\0'; + } else if (!strncmp(key, "model=", 6)) { + int len = nextkey ? (nextkey - data) : sizeof(model)-1; + if (len > (sizeof(model)-1)) + len = sizeof(model)-1; + strncpy(model, data, len); + model[len] = '\0'; } else if (!strncmp(key, "ip=", 3)) { int len = nextkey ? (nextkey - data) : 15; if (len > 15) @@ -915,6 +923,8 @@ char *xenXMDomainFormatXML(virConnectPtr virBufferVSprintf(&buf, " <script path='%s'/>\n", script); if (ip[0]) virBufferVSprintf(&buf, " <ip address='%s'/>\n", ip); + if (model[0]) + virBufferVSprintf(&buf, " <model type='%s'/>\n", model); virBufferAddLit(&buf, " </interface>\n"); skipnic: @@ -1777,6 +1787,7 @@ static char *xenXMParseXMLVif(virConnect xmlChar *source = NULL; xmlChar *mac = NULL; xmlChar *script = NULL; + xmlChar *model = NULL; xmlChar *ip = NULL; int typ = 0; char *buf = NULL; @@ -1808,6 +1819,9 @@ static char *xenXMParseXMLVif(virConnect } else if ((mac == NULL) && (xmlStrEqual(cur->name, BAD_CAST "mac"))) { mac = xmlGetProp(cur, BAD_CAST "address"); + } else if ((model == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "model"))) { + model = xmlGetProp(cur, BAD_CAST "type"); } else if ((ip == NULL) && (xmlStrEqual(cur->name, BAD_CAST "ip"))) { ip = xmlGetProp(cur, BAD_CAST "address"); @@ -1843,6 +1857,8 @@ static char *xenXMParseXMLVif(virConnect buflen += 11; if (script) buflen += 8 + strlen((const char*)script); + if (model) + buflen += 7 + strlen((const char*)model); if (ip) buflen += 4 + strlen((const char*)ip); @@ -1870,6 +1886,10 @@ static char *xenXMParseXMLVif(virConnect strcat(buf, ",script="); strcat(buf, (const char*)script); } + if (model) { + strcat(buf, ",model="); + strcat(buf, (const char*)model); + } if (ip) { strcat(buf, ",ip="); strcat(buf, (const char*)ip); @@ -1883,6 +1903,8 @@ static char *xenXMParseXMLVif(virConnect xmlFree(source); if (script != NULL) xmlFree(script); + if (model != NULL) + xmlFree(model); if (ip != NULL) xmlFree(ip); Index: src/xml.c =================================================================== RCS file: /data/cvs/libvirt/src/xml.c,v retrieving revision 1.119 diff -u -p -r1.119 xml.c --- src/xml.c 28 Apr 2008 15:14:59 -0000 1.119 +++ src/xml.c 29 Apr 2008 17:15:38 -0000 @@ -1378,6 +1378,7 @@ virDomainParseXMLIfDesc(virConnectPtr co xmlChar *source = NULL; xmlChar *mac = NULL; xmlChar *script = NULL; + xmlChar *model = NULL; xmlChar *ip = NULL; int typ = 0; int ret = -1; @@ -1409,6 +1410,9 @@ virDomainParseXMLIfDesc(virConnectPtr co } else if ((script == NULL) && (xmlStrEqual(cur->name, BAD_CAST "script"))) { script = xmlGetProp(cur, BAD_CAST "path"); + } else if ((model == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "model"))) { + model = xmlGetProp(cur, BAD_CAST "type"); } else if ((ip == NULL) && (xmlStrEqual(cur->name, BAD_CAST "ip"))) { /* XXX in future expect to need to have > 1 ip @@ -1454,6 +1458,8 @@ virDomainParseXMLIfDesc(virConnectPtr co } if (script != NULL) virBufferVSprintf(buf, "(script '%s')", script); + if (model != NULL) + virBufferVSprintf(buf, "(model '%s')", model); if (ip != NULL) virBufferVSprintf(buf, "(ip '%s')", ip); /* @@ -1474,6 +1480,8 @@ virDomainParseXMLIfDesc(virConnectPtr co xmlFree(script); if (ip != NULL) xmlFree(ip); + if (model != NULL) + xmlFree(model); return (ret); } Index: tests/qemuxml2argvtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/qemuxml2argvtest.c,v retrieving revision 1.15 diff -u -p -r1.15 qemuxml2argvtest.c --- tests/qemuxml2argvtest.c 25 Apr 2008 20:46:13 -0000 1.15 +++ tests/qemuxml2argvtest.c 29 Apr 2008 17:15:38 -0000 @@ -146,6 +146,7 @@ main(int argc, char **argv) DO_TEST("misc-acpi"); DO_TEST("misc-no-reboot"); DO_TEST("net-user"); + DO_TEST("net-virtio"); DO_TEST("serial-vc"); DO_TEST("serial-pty"); Index: tests/qemuxml2xmltest.c =================================================================== RCS file: /data/cvs/libvirt/tests/qemuxml2xmltest.c,v retrieving revision 1.13 diff -u -p -r1.13 qemuxml2xmltest.c --- tests/qemuxml2xmltest.c 25 Apr 2008 20:46:13 -0000 1.13 +++ tests/qemuxml2xmltest.c 29 Apr 2008 17:15:38 -0000 @@ -109,6 +109,7 @@ main(int argc, char **argv) DO_TEST("misc-acpi"); DO_TEST("misc-no-reboot"); DO_TEST("net-user"); + DO_TEST("net-virtio"); DO_TEST("serial-vc"); DO_TEST("serial-pty"); Index: tests/sexpr2xmltest.c =================================================================== RCS file: /data/cvs/libvirt/tests/sexpr2xmltest.c,v retrieving revision 1.26 diff -u -p -r1.26 sexpr2xmltest.c --- tests/sexpr2xmltest.c 26 Apr 2008 14:22:02 -0000 1.26 +++ tests/sexpr2xmltest.c 29 Apr 2008 17:15:38 -0000 @@ -116,6 +116,7 @@ main(int argc, char **argv) DO_TEST("curmem", "curmem", 1); DO_TEST("net-routed", "net-routed", 2); DO_TEST("net-bridged", "net-bridged", 2); + DO_TEST("net-e1000", "net-e1000", 2); DO_TEST("no-source-cdrom", "no-source-cdrom", 1); DO_TEST("fv-utc", "fv-utc", 1); Index: tests/testutils.c =================================================================== RCS file: /data/cvs/libvirt/tests/testutils.c,v retrieving revision 1.12 diff -u -p -r1.12 testutils.c --- tests/testutils.c 18 Apr 2008 15:05:29 -0000 1.12 +++ tests/testutils.c 29 Apr 2008 17:15:38 -0000 @@ -23,6 +23,7 @@ #include <fcntl.h> #include <limits.h> #include "testutils.h" +#include "internal.h" #ifdef HAVE_PATHS_H #include <paths.h> @@ -231,23 +232,27 @@ int virtTestDifference(FILE *stream, const char *expectEnd = expect + (strlen(expect)-1); const char *actualStart = actual; const char *actualEnd = actual + (strlen(actual)-1); + const char *debug; - if (getenv("DEBUG_TESTS") == NULL) + if ((debug = getenv("DEBUG_TESTS")) == NULL) return 0; - /* Skip to first character where they differ */ - while (*expectStart && *actualStart && - *actualStart == *expectStart) { - actualStart++; - expectStart++; - } + if (STREQ(debug, "") || + STREQ(debug, "1")) { + /* Skip to first character where they differ */ + while (*expectStart && *actualStart && + *actualStart == *expectStart) { + actualStart++; + expectStart++; + } - /* Work backwards to last character where they differ */ - while (actualEnd > actualStart && - expectEnd > expectStart && - *actualEnd == *expectEnd) { - actualEnd--; - expectEnd--; + /* Work backwards to last character where they differ */ + while (actualEnd > actualStart && + expectEnd > expectStart && + *actualEnd == *expectEnd) { + actualEnd--; + expectEnd--; + } } /* Show the trimmed differences */ Index: tests/xmconfigtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/xmconfigtest.c,v retrieving revision 1.15 diff -u -p -r1.15 xmconfigtest.c --- tests/xmconfigtest.c 26 Apr 2008 14:22:02 -0000 1.15 +++ tests/xmconfigtest.c 29 Apr 2008 17:15:39 -0000 @@ -202,6 +202,7 @@ main(int argc, char **argv) DO_TEST("paravirt-old-pvfb", 2); DO_TEST("paravirt-new-pvfb", 3); + DO_TEST("paravirt-net-e1000", 3); DO_TEST("fullvirt-old-cdrom", 1); DO_TEST("fullvirt-new-cdrom", 2); DO_TEST("fullvirt-utc", 2); Index: tests/xml2sexprtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/xml2sexprtest.c,v retrieving revision 1.25 diff -u -p -r1.25 xml2sexprtest.c --- tests/xml2sexprtest.c 26 Apr 2008 14:22:02 -0000 1.25 +++ tests/xml2sexprtest.c 29 Apr 2008 17:15:39 -0000 @@ -123,6 +123,7 @@ main(int argc, char **argv) DO_TEST("curmem", "curmem", "rhel5", 2); DO_TEST("net-routed", "net-routed", "pvtest", 2); DO_TEST("net-bridged", "net-bridged", "pvtest", 2); + DO_TEST("net-e1000", "net-e1000", "pvtest", 2); DO_TEST("no-source-cdrom", "no-source-cdrom", "test", 2); DO_TEST("fv-utc", "fv-utc", "fvtest", 1); Index: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args =================================================================== RCS file: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args diff -N tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1 @@ +/usr/bin/qemu -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=virtio -net user,vlan=0 -serial none -parallel none -usb \ No newline at end of file Index: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml =================================================================== RCS file: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml diff -N tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda'/> + </disk> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + </interface> + </devices> +</domain> Index: tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr =================================================================== RCS file: tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr diff -N tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1,2 @@ +(domain (domid 6)(name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d2171f48fb2e068e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... ')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))(device (vif (mac '00:11:22:33:44:55')(bridge 'xenbr2')(script 'vif-bridge')(model 'e1000')))) + Index: tests/sexpr2xmldata/sexpr2xml-net-e1000.xml =================================================================== RCS file: tests/sexpr2xmldata/sexpr2xml-net-e1000.xml diff -N tests/sexpr2xmldata/sexpr2xml-net-e1000.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/sexpr2xmldata/sexpr2xml-net-e1000.xml 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1,32 @@ +<domain type='xen' id='6'> + <name>pvtest</name> + <uuid>596a5d21-71f4-8fb2-e068-e2386a5c413e</uuid> + <os> + <type>linux</type> + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> + <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <driver name='file'/> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <interface type='bridge'> + <source bridge='xenbr2'/> + <target dev='vif6.0'/> + <mac address='00:11:22:33:44:55'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <console type='pty'> + <target port='0'/> + </console> + </devices> +</domain> Index: tests/xmconfigdata/test-paravirt-net-e1000.cfg =================================================================== RCS file: tests/xmconfigdata/test-paravirt-net-e1000.cfg diff -N tests/xmconfigdata/test-paravirt-net-e1000.cfg --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xmconfigdata/test-paravirt-net-e1000.cfg 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1,12 @@ +name = "XenGuest1" +uuid = "c7a5fdb0-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +bootloader = "/usr/bin/pygrub" +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +vfb = [ "type=vnc,vncunused=1,vnclisten=127.0.0.1,vncpasswd=123poi" ] +disk = [ "phy:/dev/HostVG/XenGuest1,xvda,w" ] +vif = [ "mac=00:16:3E:66:94:9C,model=e1000,ip=192.168.0.9" ] Index: tests/xmconfigdata/test-paravirt-net-e1000.xml =================================================================== RCS file: tests/xmconfigdata/test-paravirt-net-e1000.xml diff -N tests/xmconfigdata/test-paravirt-net-e1000.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xmconfigdata/test-paravirt-net-e1000.xml 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1,28 @@ +<domain type='xen'> + <name>XenGuest1</name> + <uuid>c7a5fdb0-cdaf-9455-926a-d65c16db1809</uuid> + <bootloader>/usr/bin/pygrub</bootloader> + <currentMemory>403456</currentMemory> + <memory>592896</memory> + <vcpu>1</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest1'/> + <target dev='xvda'/> + </disk> + <interface type='bridge'> + <mac address='00:16:3E:66:94:9C'/> + <ip address='192.168.0.9'/> + <model type='e1000'/> + </interface> + <input type='mouse' bus='xen'/> + <graphics type='vnc' port='-1' listen='127.0.0.1' passwd='123poi'/> + <console type='pty'> + <target port='0'/> + </console> + </devices> +</domain> Index: tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr =================================================================== RCS file: tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr diff -N tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1 @@ +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d2171f48fb2e068e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... ')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))(device (vif (mac '00:11:22:33:44:55')(bridge 'xenbr2')(script 'vif-bridge')(model 'e1000')))) \ No newline at end of file Index: tests/xml2sexprdata/xml2sexpr-net-e1000.xml =================================================================== RCS file: tests/xml2sexprdata/xml2sexpr-net-e1000.xml diff -N tests/xml2sexprdata/xml2sexpr-net-e1000.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xml2sexprdata/xml2sexpr-net-e1000.xml 29 Apr 2008 17:15:39 -0000 @@ -0,0 +1,30 @@ +<domain type='xen' id='15'> + <name>pvtest</name> + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> + <os> + <type>linux</type> + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> + <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <interface type="bridge"> + <mac address="00:11:22:33:44:55"/> + <source bridge="xenbr2"/> + <script path="vif-bridge"/> + <model type='e1000'/> + <target dev="vif4.0"/> + </interface> + <console tty='/dev/pts/4'/> + </devices> +</domain> + -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Tue, Apr 29, 2008 at 06:20:01PM +0100, Daniel P. Berrange wrote:
This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax
Here is a re-diff following Jim's xmlFree cleanups. Dan. Index: tests/qemuxml2argvtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/qemuxml2argvtest.c,v retrieving revision 1.15 diff -u -p -r1.15 qemuxml2argvtest.c --- tests/qemuxml2argvtest.c 25 Apr 2008 20:46:13 -0000 1.15 +++ tests/qemuxml2argvtest.c 29 Apr 2008 20:46:07 -0000 @@ -146,6 +146,7 @@ main(int argc, char **argv) DO_TEST("misc-acpi"); DO_TEST("misc-no-reboot"); DO_TEST("net-user"); + DO_TEST("net-virtio"); DO_TEST("serial-vc"); DO_TEST("serial-pty"); Index: tests/qemuxml2xmltest.c =================================================================== RCS file: /data/cvs/libvirt/tests/qemuxml2xmltest.c,v retrieving revision 1.13 diff -u -p -r1.13 qemuxml2xmltest.c --- tests/qemuxml2xmltest.c 25 Apr 2008 20:46:13 -0000 1.13 +++ tests/qemuxml2xmltest.c 29 Apr 2008 20:46:07 -0000 @@ -109,6 +109,7 @@ main(int argc, char **argv) DO_TEST("misc-acpi"); DO_TEST("misc-no-reboot"); DO_TEST("net-user"); + DO_TEST("net-virtio"); DO_TEST("serial-vc"); DO_TEST("serial-pty"); Index: tests/sexpr2xmltest.c =================================================================== RCS file: /data/cvs/libvirt/tests/sexpr2xmltest.c,v retrieving revision 1.26 diff -u -p -r1.26 sexpr2xmltest.c --- tests/sexpr2xmltest.c 26 Apr 2008 14:22:02 -0000 1.26 +++ tests/sexpr2xmltest.c 29 Apr 2008 20:46:07 -0000 @@ -116,6 +116,7 @@ main(int argc, char **argv) DO_TEST("curmem", "curmem", 1); DO_TEST("net-routed", "net-routed", 2); DO_TEST("net-bridged", "net-bridged", 2); + DO_TEST("net-e1000", "net-e1000", 2); DO_TEST("no-source-cdrom", "no-source-cdrom", 1); DO_TEST("fv-utc", "fv-utc", 1); Index: tests/testutils.c =================================================================== RCS file: /data/cvs/libvirt/tests/testutils.c,v retrieving revision 1.12 diff -u -p -r1.12 testutils.c --- tests/testutils.c 18 Apr 2008 15:05:29 -0000 1.12 +++ tests/testutils.c 29 Apr 2008 20:46:07 -0000 @@ -23,6 +23,7 @@ #include <fcntl.h> #include <limits.h> #include "testutils.h" +#include "internal.h" #ifdef HAVE_PATHS_H #include <paths.h> @@ -231,23 +232,27 @@ int virtTestDifference(FILE *stream, const char *expectEnd = expect + (strlen(expect)-1); const char *actualStart = actual; const char *actualEnd = actual + (strlen(actual)-1); + const char *debug; - if (getenv("DEBUG_TESTS") == NULL) + if ((debug = getenv("DEBUG_TESTS")) == NULL) return 0; - /* Skip to first character where they differ */ - while (*expectStart && *actualStart && - *actualStart == *expectStart) { - actualStart++; - expectStart++; - } + if (STREQ(debug, "") || + STREQ(debug, "1")) { + /* Skip to first character where they differ */ + while (*expectStart && *actualStart && + *actualStart == *expectStart) { + actualStart++; + expectStart++; + } - /* Work backwards to last character where they differ */ - while (actualEnd > actualStart && - expectEnd > expectStart && - *actualEnd == *expectEnd) { - actualEnd--; - expectEnd--; + /* Work backwards to last character where they differ */ + while (actualEnd > actualStart && + expectEnd > expectStart && + *actualEnd == *expectEnd) { + actualEnd--; + expectEnd--; + } } /* Show the trimmed differences */ Index: tests/xmconfigtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/xmconfigtest.c,v retrieving revision 1.15 diff -u -p -r1.15 xmconfigtest.c --- tests/xmconfigtest.c 26 Apr 2008 14:22:02 -0000 1.15 +++ tests/xmconfigtest.c 29 Apr 2008 20:46:07 -0000 @@ -202,6 +202,7 @@ main(int argc, char **argv) DO_TEST("paravirt-old-pvfb", 2); DO_TEST("paravirt-new-pvfb", 3); + DO_TEST("paravirt-net-e1000", 3); DO_TEST("fullvirt-old-cdrom", 1); DO_TEST("fullvirt-new-cdrom", 2); DO_TEST("fullvirt-utc", 2); Index: tests/xml2sexprtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/xml2sexprtest.c,v retrieving revision 1.25 diff -u -p -r1.25 xml2sexprtest.c --- tests/xml2sexprtest.c 26 Apr 2008 14:22:02 -0000 1.25 +++ tests/xml2sexprtest.c 29 Apr 2008 20:46:07 -0000 @@ -123,6 +123,7 @@ main(int argc, char **argv) DO_TEST("curmem", "curmem", "rhel5", 2); DO_TEST("net-routed", "net-routed", "pvtest", 2); DO_TEST("net-bridged", "net-bridged", "pvtest", 2); + DO_TEST("net-e1000", "net-e1000", "pvtest", 2); DO_TEST("no-source-cdrom", "no-source-cdrom", "test", 2); DO_TEST("fv-utc", "fv-utc", "fvtest", 1); Index: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args =================================================================== RCS file: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args diff -N tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1 @@ +/usr/bin/qemu -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=virtio -net user,vlan=0 -serial none -parallel none -usb \ No newline at end of file Index: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml =================================================================== RCS file: tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml diff -N tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda'/> + </disk> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + </interface> + </devices> +</domain> Index: tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr =================================================================== RCS file: tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr diff -N tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1,2 @@ +(domain (domid 6)(name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d2171f48fb2e068e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... ')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))(device (vif (mac '00:11:22:33:44:55')(bridge 'xenbr2')(script 'vif-bridge')(model 'e1000')))) + Index: tests/sexpr2xmldata/sexpr2xml-net-e1000.xml =================================================================== RCS file: tests/sexpr2xmldata/sexpr2xml-net-e1000.xml diff -N tests/sexpr2xmldata/sexpr2xml-net-e1000.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/sexpr2xmldata/sexpr2xml-net-e1000.xml 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1,32 @@ +<domain type='xen' id='6'> + <name>pvtest</name> + <uuid>596a5d21-71f4-8fb2-e068-e2386a5c413e</uuid> + <os> + <type>linux</type> + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> + <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <driver name='file'/> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <interface type='bridge'> + <source bridge='xenbr2'/> + <target dev='vif6.0'/> + <mac address='00:11:22:33:44:55'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <console type='pty'> + <target port='0'/> + </console> + </devices> +</domain> Index: tests/xmconfigdata/test-paravirt-net-e1000.cfg =================================================================== RCS file: tests/xmconfigdata/test-paravirt-net-e1000.cfg diff -N tests/xmconfigdata/test-paravirt-net-e1000.cfg --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xmconfigdata/test-paravirt-net-e1000.cfg 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1,12 @@ +name = "XenGuest1" +uuid = "c7a5fdb0-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +bootloader = "/usr/bin/pygrub" +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +vfb = [ "type=vnc,vncunused=1,vnclisten=127.0.0.1,vncpasswd=123poi" ] +disk = [ "phy:/dev/HostVG/XenGuest1,xvda,w" ] +vif = [ "mac=00:16:3E:66:94:9C,model=e1000,ip=192.168.0.9" ] Index: tests/xmconfigdata/test-paravirt-net-e1000.xml =================================================================== RCS file: tests/xmconfigdata/test-paravirt-net-e1000.xml diff -N tests/xmconfigdata/test-paravirt-net-e1000.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xmconfigdata/test-paravirt-net-e1000.xml 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1,28 @@ +<domain type='xen'> + <name>XenGuest1</name> + <uuid>c7a5fdb0-cdaf-9455-926a-d65c16db1809</uuid> + <bootloader>/usr/bin/pygrub</bootloader> + <currentMemory>403456</currentMemory> + <memory>592896</memory> + <vcpu>1</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest1'/> + <target dev='xvda'/> + </disk> + <interface type='bridge'> + <mac address='00:16:3E:66:94:9C'/> + <ip address='192.168.0.9'/> + <model type='e1000'/> + </interface> + <input type='mouse' bus='xen'/> + <graphics type='vnc' port='-1' listen='127.0.0.1' passwd='123poi'/> + <console type='pty'> + <target port='0'/> + </console> + </devices> +</domain> Index: tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr =================================================================== RCS file: tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr diff -N tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1 @@ +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d2171f48fb2e068e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... ')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))(device (vif (mac '00:11:22:33:44:55')(bridge 'xenbr2')(script 'vif-bridge')(model 'e1000')))) \ No newline at end of file Index: tests/xml2sexprdata/xml2sexpr-net-e1000.xml =================================================================== RCS file: tests/xml2sexprdata/xml2sexpr-net-e1000.xml diff -N tests/xml2sexprdata/xml2sexpr-net-e1000.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xml2sexprdata/xml2sexpr-net-e1000.xml 29 Apr 2008 20:46:07 -0000 @@ -0,0 +1,30 @@ +<domain type='xen' id='15'> + <name>pvtest</name> + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> + <os> + <type>linux</type> + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> + <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <interface type="bridge"> + <mac address="00:11:22:33:44:55"/> + <source bridge="xenbr2"/> + <script path="vif-bridge"/> + <model type='e1000'/> + <target dev="vif4.0"/> + </interface> + <console tty='/dev/pts/4'/> + </devices> +</domain> + -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Apr 29, 2008 at 06:20:01PM +0100, Daniel P. Berrange wrote:
This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax
Here is a re-diff following Jim's xmlFree cleanups.
Looks fine to me.
+/usr/bin/qemu -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=virtio -net user,vlan=0 -serial none -parallel none -usb
It'd be nice to change the line above to e.g., qemu -M pc -m 214 -smp 1 -nographic -monitor pty \ -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 \ -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=virtio \ -net user,vlan=0 -serial none -parallel none -usb When possible, I prefer not to hard-code program names like /usr/bin/qemu in tests, because that lets the test work even when the tool in question is installed somewhere else. What if someone is using qemu they built themselves...

Daniel P. Berrange wrote:
This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax
I still think our consensus from when I posted this patch last year (<nic model=...>) makes more sense ... but getting any form of this patch upstream sounds good to me.
- if (snprintf(nic, sizeof(nic), "nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d", + if (net->model[0] != '\0') { + if (snprintf (model, sizeof (model), ",model=%s", net->model) + >= sizeof (model)) + goto error; + } else + model[0] = '\0'; + + if (snprintf(nic, sizeof(nic), + "nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s", net->mac[0], net->mac[1], net->mac[2], net->mac[3], net->mac[4], net->mac[5], - vlan) >= sizeof(nic)) + vlan, model) >= sizeof(nic))
You could simplify this and not require the temporary buffer if you do it this way: http://www.mail-archive.com/libvir-list@redhat.com/msg03557.html -jim

On Wed, Apr 30, 2008 at 12:29:18AM -0400, Jim Paris wrote:
Daniel P. Berrange wrote:
This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax
I still think our consensus from when I posted this patch last year (<nic model=...>) makes more sense ... but getting any form of this patch upstream sounds good to me.
I'm fine with the patch. Concerning this detail of the syntax, we already know at that point that it's about a nic since we are in an <interface> description, so it's a bit redundant. Also it's the first time we introduce XML to describe a specific model of hardware (so far we managed to avoid this, from a virtualization POV it's more of a problem than a feature in my opinion), the advantage of <model type='....'/> to me is that we could reuse exactly the same construct each time we want to specify the hardware model of an emulated device, the device type being already defined by the englobing element (disk/input/graphic/serial/...) Since we are introducing new syntax, making it as generic as possible sounds better to me, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Wed, Apr 30, 2008 at 03:22:26AM -0400, Daniel Veillard wrote:
On Wed, Apr 30, 2008 at 12:29:18AM -0400, Jim Paris wrote:
Daniel P. Berrange wrote:
This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax
I still think our consensus from when I posted this patch last year (<nic model=...>) makes more sense ... but getting any form of this patch upstream sounds good to me.
I'm fine with the patch.
Concerning this detail of the syntax, we already know at that point that it's about a nic since we are in an <interface> description, so it's a bit redundant. Also it's the first time we introduce XML to describe a specific model of hardware (so far we managed to avoid this, from a virtualization POV it's more of a problem than a feature in my opinion), the advantage of <model type='....'/> to me is that we could reuse exactly the same construct each time we want to specify the hardware model of an emulated device, the device type being already defined by the englobing element (disk/input/graphic/serial/...) Since we are introducing new syntax, making it as generic as possible sounds better to me,
And Ubuntu have already shipped a product with a patch using this syntax applied, so we can't reasonably change it. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Apr 30, 2008 at 12:43:06PM +0100, Daniel P. Berrange wrote:
And Ubuntu have already shipped a product with a patch using this syntax applied, so we can't reasonably change it.
I refuse to be bound by such arguments. We just can't accept this, if someone ships a mofified version before it got agreed upon upstream, they perfectly well know that they are putting their users at risk and will have to sort the mess later. That's the rule of development in our whole ecosystem. A patch has to be reviewed by its technical merits. If someone ships a patched version and upstream uses something different in the end they will have to keep another patch to preserve the compatibility. People should push things here quickly, if we are not quick enough, feel free to complain publicly, but please don't play with the end users. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Wed, Apr 30, 2008 at 12:43:06PM +0100, Daniel P. Berrange wrote:
And Ubuntu have already shipped a product with a patch using this syntax applied, so we can't reasonably change it.
Ironically, I'm with Daniel Veillard on this one. Sure, it'd be nice if it was factored into the decision to some extent, but I'd be sad if I have somehow short-circuited the development process by forcing this decision onto the rest of you guys. I clearly read too much into the fact that Richard had posted a patch that used this syntax and noone objected. My bad entirely, and I'll deal with the mess it causes. Somehow. :) -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/

On Wed, Apr 30, 2008 at 07:27:41PM +0200, Soren Hansen wrote:
On Wed, Apr 30, 2008 at 12:43:06PM +0100, Daniel P. Berrange wrote:
And Ubuntu have already shipped a product with a patch using this syntax applied, so we can't reasonably change it.
Ironically, I'm with Daniel Veillard on this one. Sure, it'd be nice if it was factored into the decision to some extent, but I'd be sad if I have somehow short-circuited the development process by forcing this decision onto the rest of you guys. I clearly read too much into the fact that Richard had posted a patch that used this syntax and noone objected. My bad entirely, and I'll deal with the mess it causes.
WRT to the network interface type attribute, I advised Soren at the virt summit in Austin, that since Rich Jones had already posted the patch and we'd all basically agreed on syntax it was reasonably to include the patch in Ubuntu. It was only a matter of time before we merged it - as I have done today. Now, the disk model syntax supporting virtio is where I agree with Daniel that it should have been posted upstream before inclusion in a product Even if the code was just a quick hack, not in a state fit for merging - it is always beneficial to post as early as possible just for the sake of visibility & comment. This said I believe the proposed 'bus' atribute for disks is the optimal way to handle virtio for disks. Just for future enhancements please post ideas to this list asap. I myself have posted ideas more than 1 year before actually getting around to implementing them, so there's no requirement to follow through with code immediately :-) Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Apr 30, 2008 at 11:36:33PM +0100, Daniel P. Berrange wrote:
WRT to the network interface type attribute, I advised Soren at the virt summit in Austin, that since Rich Jones had already posted the patch and we'd all basically agreed on syntax it was reasonably to include the patch in Ubuntu. It was only a matter of time before we merged it - as I have done today.
Thanks very much. That strikes that bit off of my "Stuff I might need to worry about" list. :)
Now, the disk model syntax supporting virtio is where I agree with Daniel that it should have been posted upstream before inclusion in a product Even if the code was just a quick hack, not in a state fit for merging - it is always beneficial to post as early as possible just for the sake of visibility & comment.
This is good advice. Thanks.
This said I believe the proposed 'bus' atribute for disks is the optimal way to handle virtio for disks.
I agree. A <model type='foo' /> element in the disk definition could still be used to specify which particular SCSI controller you'd like.
Just for future enhancements please post ideas to this list asap.
I'll keep that in mind. I'm truly sorry for the stir I've caused and I have every intention of making sure it won't happen again.
I myself have posted ideas more than 1 year before actually getting around to implementing them, so there's no requirement to follow through with code immediately :-)
:) -- Soren Hansen | Virtualisation specialist | Ubuntu Server Team Canonical Ltd. | http://www.ubuntu.com/

On Wed, Apr 30, 2008 at 12:29:18AM -0400, Jim Paris wrote:
Daniel P. Berrange wrote:
This patch finishes off the work from Rich / Soren to support network interface model in both Xen and QEMU drivers, and adds test cases for the new syntax
I still think our consensus from when I posted this patch last year (<nic model=...>) makes more sense ... but getting any form of this patch upstream sounds good to me.
- if (snprintf(nic, sizeof(nic), "nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d", + if (net->model[0] != '\0') { + if (snprintf (model, sizeof (model), ",model=%s", net->model) + >= sizeof (model)) + goto error; + } else + model[0] = '\0'; + + if (snprintf(nic, sizeof(nic), + "nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s", net->mac[0], net->mac[1], net->mac[2], net->mac[3], net->mac[4], net->mac[5], - vlan) >= sizeof(nic)) + vlan, model) >= sizeof(nic))
You could simplify this and not require the temporary buffer if you do it this way: http://www.mail-archive.com/libvir-list@redhat.com/msg03557.html
I committed the patch with this simplification included Regards, Dan -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (5)
-
Daniel P. Berrange
-
Daniel Veillard
-
Jim Meyering
-
Jim Paris
-
Soren Hansen