When we hot-plug a disk device into a qemu guest, we need to retain its PCI address so that it can be removed again later. Currently, we do retain the slot number, but not across libvirtd restarts. Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the domain and bus number, but the format allows us to do that in future. * src/domain_conf.h: replace slotnum with pci_addr * src/domain_conf.c: handle formatting and parsing the address * src/qemu_driver.c: store the parsed slot number as a full PCI address, and use this address with the pci_del monitor command * src/vbox/vbox_tmpl.c: we're debug printing slotnum here even though it can never be set, just delete it --- src/domain_conf.c | 22 +++++++++++++++++++--- src/domain_conf.h | 2 +- src/qemu_driver.c | 42 +++++++++++++++++++++++++++++------------- src/vbox/vbox_tmpl.c | 1 - 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 10e6ac6..16f7d73 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -287,6 +287,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->dst); VIR_FREE(def->driverName); VIR_FREE(def->driverType); + VIR_FREE(def->pci_addr); VIR_FREE(def); } @@ -643,7 +644,7 @@ int virDomainDiskCompare(virDomainDiskDefPtr a, static virDomainDiskDefPtr virDomainDiskDefParseXML(virConnectPtr conn, xmlNodePtr node, - int flags ATTRIBUTE_UNUSED) { + int flags) { virDomainDiskDefPtr def; xmlNodePtr cur; char *type = NULL; @@ -654,6 +655,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *target = NULL; char *bus = NULL; char *cachetag = NULL; + char *pci_addr = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -708,6 +710,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { def->shared = 1; + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && + xmlStrEqual(cur->name, BAD_CAST "state")) { + pci_addr = virXMLPropString(cur, "devaddr"); } } cur = cur->next; @@ -815,6 +820,8 @@ virDomainDiskDefParseXML(virConnectPtr conn, driverName = NULL; def->driverType = driverType; driverType = NULL; + def->pci_addr = pci_addr; + pci_addr = NULL; cleanup: VIR_FREE(bus); @@ -825,6 +832,7 @@ cleanup: VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(cachetag); + VIR_FREE(pci_addr); return def; @@ -3388,7 +3396,8 @@ virDomainLifecycleDefFormat(virConnectPtr conn, static int virDomainDiskDefFormat(virConnectPtr conn, virBufferPtr buf, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + int flags) { const char *type = virDomainDiskTypeToString(def->type); const char *device = virDomainDiskDeviceTypeToString(def->device); @@ -3446,6 +3455,13 @@ virDomainDiskDefFormat(virConnectPtr conn, if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && def->pci_addr) { + virBufferAddLit(buf, " <state"); + if (def->pci_addr) + virBufferEscapeString(buf, " devaddr='%s'", def->pci_addr); + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " </disk>\n"); return 0; @@ -4048,7 +4064,7 @@ char *virDomainDefFormat(virConnectPtr conn, def->emulator); for (n = 0 ; n < def->ndisks ; n++) - if (virDomainDiskDefFormat(conn, &buf, def->disks[n]) < 0) + if (virDomainDiskDefFormat(conn, &buf, def->disks[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nfss ; n++) diff --git a/src/domain_conf.h b/src/domain_conf.h index 6e111fa..1766b61 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -106,7 +106,7 @@ struct _virDomainDiskDef { int cachemode; unsigned int readonly : 1; unsigned int shared : 1; - int slotnum; /* pci slot number for unattach */ + char *pci_addr; /* for detach */ }; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 00dc6e5..3dec630 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4390,6 +4390,8 @@ try_command: return -1; } + VIR_FREE(cmd); + DEBUG ("%s: pci_add reply: %s", vm->def->name, reply); /* If the command succeeds qemu prints: * OK bus 0, slot XXX... @@ -4399,22 +4401,28 @@ try_command: if ((s = strstr(reply, "OK ")) && (s = strstr(s, "slot "))) { char *dummy = s; + int domain, bus, slot; + s += strlen("slot "); - if (virStrToLong_i ((const char*)s, &dummy, 10, &dev->data.disk->slotnum) == -1) - VIR_WARN("%s", _("Unable to parse slot number\n")); /* XXX not neccessarily always going to end up in domain 0 / bus 0 :-( */ - /* XXX this slotnum is not persistant across restarts :-( */ + domain = bus = 0; + if (virStrToLong_i ((const char*)s, &dummy, 10, &slot) == -1) + VIR_WARN("%s", _("Unable to parse slot number\n")); + else if (virAsprintf(&dev->data.disk->pci_addr, "%.4x:%.2x:%.2x", + domain, bus, slot) < 0) { + virReportOOMError(conn); + VIR_FREE(reply); + return -1; + } } else if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { VIR_FREE(reply); - VIR_FREE(cmd); tryOldSyntax = 1; goto try_command; } else { qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("adding %s disk failed: %s"), type, reply); VIR_FREE(reply); - VIR_FREE(cmd); return -1; } @@ -4423,7 +4431,7 @@ try_command: virDomainDiskQSort); VIR_FREE(reply); - VIR_FREE(cmd); + return 0; } @@ -4660,21 +4668,29 @@ static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, goto cleanup; } - if (detach->slotnum < 1) { + if (!detach->pci_addr) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - invalid slot number %d"), - detach->dst, detach->slotnum); + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); goto cleanup; } try_command: if (tryOldSyntax) { - if (virAsprintf(&cmd, "pci_del 0 %d", detach->slotnum) < 0) { + const char *slot = strrchr(detach->pci_addr, ':'); + if (!slot) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("Invalid PCI address for device '%s' : %s"), + detach->dst, detach->pci_addr); + goto cleanup; + } + ++slot; + if (virAsprintf(&cmd, "pci_del 0 %s", slot) < 0) { virReportOOMError(conn); goto cleanup; } } else { - if (virAsprintf(&cmd, "pci_del pci_addr=0:0:%d", detach->slotnum) < 0) { + if (virAsprintf(&cmd, "pci_del pci_addr=%s", detach->pci_addr) < 0) { virReportOOMError(conn); goto cleanup; } @@ -4698,8 +4714,8 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to detach disk %s: invalid slot %d: %s"), - detach->dst, detach->slotnum, reply); + _("failed to detach disk %s: invalid PCI address %s: %s"), + detach->dst, detach->pci_addr, reply); goto cleanup; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 3208f03..a2b958a 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2712,7 +2712,6 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { DEBUG("disk(%d) cachemode: %d", i, def->disks[i]->cachemode); DEBUG("disk(%d) readonly: %s", i, def->disks[i]->readonly ? "True" : "False"); DEBUG("disk(%d) shared: %s", i, def->disks[i]->shared ? "True" : "False"); - DEBUG("disk(%d) slotnum: %d", i, def->disks[i]->slotnum); if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { -- 1.6.2.5

Re-factor this code so that it can be used for NIC hotplug too. The awkward prefix and type_sep arguments are needed to allow us to do "host_net_add tap vlan=..." * src/qemu_conf.c: factor the net backend string formatting code into its own function --- src/qemu_conf.c | 233 +++++++++++++++++++++++++++++-------------------------- 1 files changed, 122 insertions(+), 111 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 679dac3..ba99652 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -740,40 +740,34 @@ int qemudExtractVersion(virConnectPtr conn, } -static char * +static int qemudNetworkIfaceConnect(virConnectPtr conn, struct qemud_driver *driver, - int **tapfds, - int *ntapfds, virDomainNetDefPtr net, - int vlan, int vnet_hdr) { char *brname; - char tapfdstr[4+3+32+7]; - char *retval = NULL; int err; int tapfd = -1; if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetworkPtr network = virNetworkLookupByName(conn, net->data.network.name); - if (!network) { - goto error; - } + if (!network) + return -1; + brname = virNetworkGetBridgeName(network); virNetworkFree(network); - if (brname == NULL) { - goto error; - } + if (brname == NULL) + return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { brname = net->data.bridge.brname; } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); - goto error; + return -1; } if (!net->ifname || @@ -782,7 +776,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net->ifname); if (!(net->ifname = strdup("vnet%d"))) { virReportOOMError(conn); - goto error; + return -1; } } @@ -791,7 +785,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("cannot initialize bridge support: %s"), virStrerror(err, ebuf, sizeof ebuf)); - goto error; + return -1; } if ((err = brAddTap(driver->brctl, brname, @@ -807,30 +801,10 @@ qemudNetworkIfaceConnect(virConnectPtr conn, "to bridge '%s' : %s"), net->ifname, brname, virStrerror(err, ebuf, sizeof ebuf)); } - goto error; + return -1; } - snprintf(tapfdstr, sizeof(tapfdstr), - "tap,fd=%d,vlan=%d", - tapfd, vlan); - - if (!(retval = strdup(tapfdstr))) - goto no_memory; - - if (VIR_REALLOC_N(*tapfds, (*ntapfds)+1) < 0) - goto no_memory; - - (*tapfds)[(*ntapfds)++] = tapfd; - - return retval; - - no_memory: - virReportOOMError(conn); - error: - VIR_FREE(retval); - if (tapfd != -1) - close(tapfd); - return NULL; + return tapfd; } static int @@ -858,6 +832,96 @@ qemuBuildNicStr(virConnectPtr conn, return 0; } +static int +qemuBuildHostNetStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *prefix, + char type_sep, + int vlan, + int tapfd, + char **str) +{ + switch (net->type) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + if (virAsprintf(str, "%stap%cfd=%d,vlan=%d", + prefix ? prefix : "", + type_sep, tapfd, vlan) < 0) { + virReportOOMError(conn); + return -1; + } + break; + + case VIR_DOMAIN_NET_TYPE_ETHERNET: + { + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (prefix) + virBufferAdd(&buf, prefix, strlen(prefix)); + virBufferAddLit(&buf, "tap"); + if (net->ifname) { + virBufferVSprintf(&buf, "%cifname=%s", type_sep, net->ifname); + type_sep = ','; + } + if (net->data.ethernet.script) { + virBufferVSprintf(&buf, "%cscript=%s", type_sep, + net->data.ethernet.script); + type_sep = ','; + } + virBufferVSprintf(&buf, "%cvlan=%d", type_sep, vlan); + if (virBufferError(&buf)) { + virReportOOMError(conn); + return -1; + } + + *str = virBufferContentAndReset(&buf); + } + break; + + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_MCAST: + { + const char *mode = NULL; + + switch (net->type) { + case VIR_DOMAIN_NET_TYPE_CLIENT: + mode = "connect"; + break; + case VIR_DOMAIN_NET_TYPE_SERVER: + mode = "listen"; + break; + case VIR_DOMAIN_NET_TYPE_MCAST: + mode = "mcast"; + break; + } + + if (virAsprintf(str, "%ssocket%c%s=%s:%d,vlan=%d", + prefix ? prefix : "", + type_sep, mode, + net->data.socket.address, + net->data.socket.port, + vlan) < 0) { + virReportOOMError(conn); + return -1; + } + } + break; + + case VIR_DOMAIN_NET_TYPE_USER: + default: + if (virAsprintf(str, "%suser%cvlan=%d", + prefix ? prefix : "", + type_sep, vlan) < 0) { + virReportOOMError(conn); + return -1; + } + break; + } + + return 0; +} + static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) @@ -1388,95 +1452,42 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-net"); ADD_ARG_LIT("none"); } else { - int vlan = 0; for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; - char *nic; + char *nic, *host; + int tapfd = -1; - if (qemuBuildNicStr(conn, net, NULL, ',', vlan, &nic) < 0) + if (qemuBuildNicStr(conn, net, NULL, ',', i, &nic) < 0) goto error; ADD_ARG_LIT("-net"); ADD_ARG(nic); - ADD_ARG_LIT("-net"); - switch (net->type) { - case VIR_DOMAIN_NET_TYPE_NETWORK: - case VIR_DOMAIN_NET_TYPE_BRIDGE: - { - char *tap; - int vnet_hdr = 0; - - if (qemuCmdFlags & QEMUD_CMD_FLAG_VNET_HDR && - net->model && STREQ(net->model, "virtio")) - vnet_hdr = 1; - - tap = qemudNetworkIfaceConnect(conn, driver, - tapfds, ntapfds, - net, vlan, vnet_hdr); - if (tap == NULL) - goto error; - ADD_ARG(tap); - break; - } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || + net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + int vnet_hdr = 0; - case VIR_DOMAIN_NET_TYPE_ETHERNET: - { - virBuffer buf = VIR_BUFFER_INITIALIZER; + if (qemuCmdFlags & QEMUD_CMD_FLAG_VNET_HDR && + net->model && STREQ(net->model, "virtio")) + vnet_hdr = 1; - virBufferAddLit(&buf, "tap"); - if (net->ifname) - virBufferVSprintf(&buf, ",ifname=%s", net->ifname); - if (net->data.ethernet.script) - virBufferVSprintf(&buf, ",script=%s", net->data.ethernet.script); - virBufferVSprintf(&buf, ",vlan=%d", vlan); - if (virBufferError(&buf)) - goto error; - - ADD_ARG(virBufferContentAndReset(&buf)); - } - break; + tapfd = qemudNetworkIfaceConnect(conn, driver, net, vnet_hdr); + if (tapfd < 0) + goto error; - case VIR_DOMAIN_NET_TYPE_CLIENT: - case VIR_DOMAIN_NET_TYPE_SERVER: - case VIR_DOMAIN_NET_TYPE_MCAST: - { - char arg[PATH_MAX]; - const char *mode = NULL; - switch (net->type) { - case VIR_DOMAIN_NET_TYPE_CLIENT: - mode = "connect"; - break; - case VIR_DOMAIN_NET_TYPE_SERVER: - mode = "listen"; - break; - case VIR_DOMAIN_NET_TYPE_MCAST: - mode = "mcast"; - break; - } - if (snprintf(arg, PATH_MAX-1, "socket,%s=%s:%d,vlan=%d", - mode, - net->data.socket.address, - net->data.socket.port, - vlan) >= (PATH_MAX-1)) - goto error; - - ADD_ARG_LIT(arg); + if (VIR_REALLOC_N(*tapfds, (*ntapfds)+1) < 0) { + close(tapfd); + goto no_memory; } - break; - case VIR_DOMAIN_NET_TYPE_USER: - default: - { - char arg[PATH_MAX]; - if (snprintf(arg, PATH_MAX-1, "user,vlan=%d", vlan) >= (PATH_MAX-1)) - goto error; - - ADD_ARG_LIT(arg); - } + (*tapfds)[(*ntapfds)++] = tapfd; } - vlan++; + if (qemuBuildHostNetStr(conn, net, NULL, ',', i, tapfd, &host) < 0) + goto error; + + ADD_ARG_LIT("-net"); + ADD_ARG(host); } } -- 1.6.2.5

We need these so that we can remove the devices via the monitor. * src/domain_conf.h: add nic_name and hostnet_name to virDomainNetDef * src/domain_conf.c: free nic_name and hostnet_name * src/qemu_conf.c: add qemuAssignNetNames(), use it if qemu has support for the param and pass the names on the command line * tests/qemuxml2argv*: add a test for this --- src/domain_conf.c | 2 + src/domain_conf.h | 2 + src/qemu_conf.c | 95 ++++++++++++++++++-- .../qemuxml2argv-net-eth-names.args | 1 + .../qemuxml2argv-net-eth-names.xml | 31 +++++++ tests/qemuxml2argvtest.c | 1 + 6 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.xml diff --git a/src/domain_conf.c b/src/domain_conf.c index 16f7d73..a5e4697 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -339,6 +339,8 @@ void virDomainNetDefFree(virDomainNetDefPtr def) } VIR_FREE(def->ifname); + VIR_FREE(def->nic_name); + VIR_FREE(def->hostnet_name); VIR_FREE(def); } diff --git a/src/domain_conf.h b/src/domain_conf.h index 1766b61..1e30107 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -175,6 +175,8 @@ struct _virDomainNetDef { } internal; } data; char *ifname; + char *nic_name; + char *hostnet_name; }; enum virDomainChrSrcType { diff --git a/src/qemu_conf.c b/src/qemu_conf.c index a9e5e4e..0362d76 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -810,6 +810,68 @@ qemudNetworkIfaceConnect(virConnectPtr conn, return tapfd; } +static const char * +qemuNetTypeToHostNet(int type) +{ + switch (type) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + return "tap"; + + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_MCAST: + return "socket"; + + case VIR_DOMAIN_NET_TYPE_USER: + default: + return "user"; + } +} + +static int +qemuAssignNetNames(virDomainDefPtr def, + virDomainNetDefPtr net) +{ + char *nic_name, *hostnet_name; + int i, nic_index = 0, hostnet_index = 0; + + for (i = 0; i < def->nnets; i++) { + if (def->nets[i] == net) + continue; + + if (!def->nets[i]->nic_name || !def->nets[i]->hostnet_name) + continue; + + if ((def->nets[i]->model == NULL && net->model == NULL) || + (def->nets[i]->model != NULL && net->model != NULL && + STREQ(def->nets[i]->model, net->model))) + ++nic_index; + + if (STREQ(qemuNetTypeToHostNet(def->nets[i]->type), + qemuNetTypeToHostNet(net->type))) + ++hostnet_index; + } + + if (virAsprintf(&nic_name, "%s.%d", + net->model ? net->model : "nic", + nic_index) < 0) + return -1; + + if (virAsprintf(&hostnet_name, "%s.%d", + qemuNetTypeToHostNet(net->type), + hostnet_index) < 0) { + VIR_FREE(nic_name); + return -1; + } + + net->nic_name = nic_name; + net->hostnet_name = hostnet_name; + + return 0; +} + static int qemuBuildNicStr(virConnectPtr conn, virDomainNetDefPtr net, @@ -819,7 +881,7 @@ qemuBuildNicStr(virConnectPtr conn, char **str) { if (virAsprintf(str, - "%snic%cmacaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s%s", + "%snic%cmacaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s%s%s%s", prefix ? prefix : "", type_sep, net->mac[0], net->mac[1], @@ -827,7 +889,9 @@ qemuBuildNicStr(virConnectPtr conn, net->mac[4], net->mac[5], vlan, (net->model ? ",model=" : ""), - (net->model ? net->model : "")) < 0) { + (net->model ? net->model : ""), + (net->nic_name ? ",name=" : ""), + (net->nic_name ? net->nic_name : "")) < 0) { virReportOOMError(conn); return -1; } @@ -847,9 +911,11 @@ qemuBuildHostNetStr(virConnectPtr conn, switch (net->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: - if (virAsprintf(str, "%stap%cfd=%d,vlan=%d", + if (virAsprintf(str, "%stap%cfd=%d,vlan=%d%s%s", prefix ? prefix : "", - type_sep, tapfd, vlan) < 0) { + type_sep, tapfd, vlan, + (net->hostnet_name ? ",name=" : ""), + (net->hostnet_name ? net->hostnet_name : "")) < 0) { virReportOOMError(conn); return -1; } @@ -872,6 +938,11 @@ qemuBuildHostNetStr(virConnectPtr conn, type_sep = ','; } virBufferVSprintf(&buf, "%cvlan=%d", type_sep, vlan); + if (net->hostnet_name) { + virBufferVSprintf(&buf, "%cname=%s", type_sep, + net->hostnet_name); + type_sep = ','; + } if (virBufferError(&buf)) { virReportOOMError(conn); return -1; @@ -899,12 +970,14 @@ qemuBuildHostNetStr(virConnectPtr conn, break; } - if (virAsprintf(str, "%ssocket%c%s=%s:%d,vlan=%d", + if (virAsprintf(str, "%ssocket%c%s=%s:%d,vlan=%d%s%s", prefix ? prefix : "", type_sep, mode, net->data.socket.address, net->data.socket.port, - vlan) < 0) { + vlan, + (net->hostnet_name ? ",name=" : ""), + (net->hostnet_name ? net->hostnet_name : "")) < 0) { virReportOOMError(conn); return -1; } @@ -913,9 +986,11 @@ qemuBuildHostNetStr(virConnectPtr conn, case VIR_DOMAIN_NET_TYPE_USER: default: - if (virAsprintf(str, "%suser%cvlan=%d", + if (virAsprintf(str, "%suser%cvlan=%d%s%s", prefix ? prefix : "", - type_sep, vlan) < 0) { + type_sep, vlan, + (net->hostnet_name ? ",name=" : ""), + (net->hostnet_name ? net->hostnet_name : "")) < 0) { virReportOOMError(conn); return -1; } @@ -1460,6 +1535,10 @@ int qemudBuildCommandLine(virConnectPtr conn, char *nic, *host; int tapfd = -1; + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && + qemuAssignNetNames(def, net) < 0) + goto no_memory; + if (qemuBuildNicStr(conn, net, NULL, ',', i, &nic) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args new file mode 100644 index 0000000..70a10cd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,name=nic.0 -net tap,script=/etc/qemu-ifup,vlan=0,name=tap.0 -net nic,macaddr=00:11:22:33:44:56,vlan=1,model=e1000,name=e1000.0 -net tap,script=/etc/qemu-ifup,vlan=1,name=tap.1 -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.xml new file mode 100644 index 0000000..8aae269 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.xml @@ -0,0 +1,31 @@ +<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' bus='ide'/> + </disk> + <interface type='ethernet'> + <mac address='00:11:22:33:44:55'/> + <script path='/etc/qemu-ifup'/> + </interface> + <interface type='ethernet'> + <mac address='00:11:22:33:44:56'/> + <script path='/etc/qemu-ifup'/> + <model type='e1000'/> + </interface> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b6e258a..73a6709 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -245,6 +245,7 @@ mymain(int argc, char **argv) DO_TEST("net-virtio", 0); DO_TEST("net-eth", 0); DO_TEST("net-eth-ifname", 0); + DO_TEST("net-eth-names", QEMUD_CMD_FLAG_NET_NAME); DO_TEST("serial-vc", 0); DO_TEST("serial-pty", 0); -- 1.6.2.5

Currently, an interface's vlan number corresponds to its index in the table of network interfaces. That is no longer true when we allow devices to be removed. To fix this, we store the vlan number in the domain's state XML so that it survives libvirtd restarts. * src/domain_conf.h: add vlan number to virDomainNetDef * src/domain_conf.c: store it in XML as <state vlan='N'/>, defaulting to -1 if this is state saved by a previous version of libvirt * src/qemu_conf.c: assign vlan numbers before starting qemu --- src/domain_conf.c | 12 ++++++++++++ src/domain_conf.h | 1 + src/qemu_conf.c | 7 +++++-- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 2b4fe90..cce863c 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -954,6 +954,7 @@ virDomainNetDefParseXML(virConnectPtr conn, char *internal = NULL; char *nic_name = NULL; char *hostnet_name = NULL; + char *vlan = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -1023,6 +1024,7 @@ virDomainNetDefParseXML(virConnectPtr conn, xmlStrEqual(cur->name, BAD_CAST "state")) { nic_name = virXMLPropString(cur, "nic"); hostnet_name = virXMLPropString(cur, "hostnet"); + vlan = virXMLPropString(cur, "vlan"); } } cur = cur->next; @@ -1038,6 +1040,13 @@ virDomainNetDefParseXML(virConnectPtr conn, def->hostnet_name = hostnet_name; nic_name = hostnet_name = NULL; + def->vlan = -1; + if (vlan && virStrToLong_i(vlan, NULL, 10, &def->vlan) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <state> 'vlan' attribute")); + goto error; + } + switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: if (network == NULL) { @@ -1159,6 +1168,7 @@ cleanup: VIR_FREE(internal); VIR_FREE(nic_name); VIR_FREE(hostnet_name); + VIR_FREE(vlan); return def; @@ -3614,6 +3624,8 @@ virDomainNetDefFormat(virConnectPtr conn, virBufferEscapeString(buf, " nic='%s'", def->nic_name); if (def->hostnet_name) virBufferEscapeString(buf, " hostnet='%s'", def->hostnet_name); + if (def->vlan > 0) + virBufferVSprintf(buf, " vlan='%d'", def->vlan); virBufferAddLit(buf, "/>\n"); } diff --git a/src/domain_conf.h b/src/domain_conf.h index 1e30107..8fa384a 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -177,6 +177,7 @@ struct _virDomainNetDef { char *ifname; char *nic_name; char *hostnet_name; + int vlan; }; enum virDomainChrSrcType { diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 0362d76..6d876cb 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1535,11 +1535,13 @@ int qemudBuildCommandLine(virConnectPtr conn, char *nic, *host; int tapfd = -1; + net->vlan = i; + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && qemuAssignNetNames(def, net) < 0) goto no_memory; - if (qemuBuildNicStr(conn, net, NULL, ',', i, &nic) < 0) + if (qemuBuildNicStr(conn, net, NULL, ',', net->vlan, &nic) < 0) goto error; ADD_ARG_LIT("-net"); @@ -1565,7 +1567,8 @@ int qemudBuildCommandLine(virConnectPtr conn, (*tapfds)[(*ntapfds)++] = tapfd; } - if (qemuBuildHostNetStr(conn, net, NULL, ',', i, tapfd, &host) < 0) + if (qemuBuildHostNetStr(conn, net, NULL, ',', + net->vlan, tapfd, &host) < 0) goto error; ADD_ARG_LIT("-net"); -- 1.6.2.5

Implement basic NIC hotplug support using the 'host_net_add' and 'pci_add' qemu monitor commands. For now, we don't support 'bridge' or 'network' types. Also, if pci_add fails, we currently fail to remove the backend which we added. Finally, NIC hot-unplug support is missing. * src/qemu_driver.c: add qemudDomainAttachNetDevice() * src/qemu_conf.[ch]: export qemuBuildNicStr(), qemuBuildHostNetStr() and qemuAssignNames() * src/libvirt_private.syms: export virDomainNetTypeToString() --- src/libvirt_private.syms | 1 + src/qemu_conf.c | 6 ++-- src/qemu_conf.h | 18 ++++++++++ src/qemu_driver.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 59c78d5..0caa590 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -91,6 +91,7 @@ virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; virDomainLoadAllConfigs; virDomainNetDefFree; +virDomainNetTypeToString; virDomainObjFree; virDomainObjListFree; virDomainRemoveInactive; diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 6d876cb..f483a51 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -830,7 +830,7 @@ qemuNetTypeToHostNet(int type) } } -static int +int qemuAssignNetNames(virDomainDefPtr def, virDomainNetDefPtr net) { @@ -872,7 +872,7 @@ qemuAssignNetNames(virDomainDefPtr def, return 0; } -static int +int qemuBuildNicStr(virConnectPtr conn, virDomainNetDefPtr net, const char *prefix, @@ -899,7 +899,7 @@ qemuBuildNicStr(virConnectPtr conn, return 0; } -static int +int qemuBuildHostNetStr(virConnectPtr conn, virDomainNetDefPtr net, const char *prefix, diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 1b2d061..50d7c0a 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -145,6 +145,24 @@ int qemudBuildCommandLine (virConnectPtr conn, int *ntapfds, const char *migrateFrom); +int qemuBuildHostNetStr (virConnectPtr conn, + virDomainNetDefPtr net, + const char *prefix, + char type_sep, + int vlan, + int tapfd, + char **str); + +int qemuBuildNicStr (virConnectPtr conn, + virDomainNetDefPtr net, + const char *prefix, + char type_sep, + int vlan, + char **str); + +int qemuAssignNetNames (virDomainDefPtr def, + virDomainNetDefPtr net); + virDomainDefPtr qemuParseCommandLine(virConnectPtr conn, virCapsPtr caps, const char **progenv, diff --git a/src/qemu_driver.c b/src/qemu_driver.c index cbc185c..cde789e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4492,6 +4492,84 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, return 0; } +static int qemudDomainAttachNetDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + unsigned int qemuCmdFlags) +{ + virDomainNetDefPtr net = dev->data.net; + char *cmd, *reply; + int i; + + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_HOST_NET_ADD)) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("%s"), + "installed qemu version does not support host_net_add"); + return -1; + } + + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("network device type '%s' cannot be attached"), + virDomainNetTypeToString(net->type)); + return -1; + } + + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) { + virReportOOMError(conn); + return -1; + } + + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && + qemuAssignNetNames(vm->def, net) < 0) { + virReportOOMError(conn); + return -1; + } + + /* Choose a vlan value greater than all other values since + * older versions did not store the value in the state file. + */ + net->vlan = vm->def->nnets; + for (i = 0; i < vm->def->nnets; i++) + if (vm->def->nets[i]->vlan >= net->vlan) + net->vlan = vm->def->nets[i]->vlan; + + if (qemuBuildHostNetStr(conn, net, + "host_net_add ", ' ', net->vlan, -1, &cmd) < 0) + return -1; + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to add network backend with '%s'"), cmd); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + if (qemuBuildNicStr(conn, net, + "pci_add pci_addr=auto ", ' ', net->vlan, &cmd) < 0) { + /* FIXME: try and remove the backend again */ + return -1; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + /* FIXME: try and remove the backend again */ + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to add NIC with '%s'"), cmd); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + vm->def->nets[vm->def->nnets++] = net; + + return 0; +} + static int qemudDomainAttachHostDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) @@ -4617,6 +4695,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom, virDomainDiskDeviceTypeToString(dev->data.disk->device)); goto cleanup; } + } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { + ret = qemudDomainAttachNetDevice(dom->conn, vm, dev, qemuCmdFlags); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { -- 1.6.2.5

The current code for parsing pci_add replies ignores the the domain and bus numbers. Re-write the code to rectify that. Also, since pci_add is used for NIC hotplug as well ask disk hotplug, re-factor the code into a separate function. * src/qemu_driver.c: add qemudParsePciAddReply() function which can handle parsing domain and bus numbers --- src/qemu_driver.c | 111 +++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 82 insertions(+), 29 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 4cc78f5..27bda12 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4340,15 +4340,83 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, return 0; } +static int +qemudParsePciAddReply(virDomainObjPtr vm, + const char *reply, + int *domain, + int *bus, + int *slot) +{ + char *s, *e; + + DEBUG("%s: pci_add reply: %s", vm->def->name, reply); + + /* If the command succeeds qemu prints: + * OK bus 0, slot XXX... + * or + * OK domain 0, bus 0, slot XXX + */ + if (!(s = strstr(reply, "OK "))) + return -1; + + s += 3; + + if (STRPREFIX(s, "domain ")) { + s += strlen("domain "); + + if (virStrToLong_i(s, &e, 10, domain) == -1) { + VIR_WARN(_("Unable to parse domain number '%s'\n"), s); + return -1; + } + + if (!STRPREFIX(e, ", ")) { + VIR_WARN(_("Expected ', ' parsing pci_add reply '%s'\n"), s); + return -1; + } + s = e + 2; + } + + if (!STRPREFIX(s, "bus ")) { + VIR_WARN(_("Expected 'bus ' parsing pci_add reply '%s'\n"), s); + return -1; + } + s += strlen("bus "); + + if (virStrToLong_i(s, &e, 10, bus) == -1) { + VIR_WARN(_("Unable to parse bus number '%s'\n"), s); + return -1; + } + + if (!STRPREFIX(e, ", ")) { + VIR_WARN(_("Expected ', ' parsing pci_add reply '%s'\n"), s); + return -1; + } + s = e + 2; + + if (!STRPREFIX(s, "slot ")) { + VIR_WARN(_("Expected 'slot ' parsing pci_add reply '%s'\n"), s); + return -1; + } + s += strlen("slot "); + + if (virStrToLong_i(s, &e, 10, slot) == -1) { + VIR_WARN(_("Unable to parse slot number '%s'\n"), s); + return -1; + } + + return 0; +} + static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int ret, i; - char *cmd, *reply, *s; + char *cmd, *reply; char *safe_path; const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus); int tryOldSyntax = 0; + int domain, bus, slot; for (i = 0 ; i < vm->def->ndisks ; i++) { if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { @@ -4387,46 +4455,31 @@ try_command: VIR_FREE(cmd); - DEBUG ("%s: pci_add reply: %s", vm->def->name, reply); - /* If the command succeeds qemu prints: - * OK bus 0, slot XXX... - * or - * OK domain 0, bus 0, slot XXX - */ - if ((s = strstr(reply, "OK ")) && - (s = strstr(s, "slot "))) { - char *dummy = s; - int domain, bus, slot; - - s += strlen("slot "); - - /* XXX not neccessarily always going to end up in domain 0 / bus 0 :-( */ - domain = bus = 0; - if (virStrToLong_i ((const char*)s, &dummy, 10, &slot) == -1) - VIR_WARN("%s", _("Unable to parse slot number\n")); - else if (virAsprintf(&dev->data.disk->pci_addr, "%.4x:%.2x:%.2x", - domain, bus, slot) < 0) { - virReportOOMError(conn); + if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { VIR_FREE(reply); - return -1; + tryOldSyntax = 1; + goto try_command; } - } else if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; - } else { + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("adding %s disk failed: %s"), type, reply); VIR_FREE(reply); return -1; } + VIR_FREE(reply); + + if (virAsprintf(&dev->data.disk->pci_addr, "%.4x:%.2x:%.2x", + domain, bus, slot) < 0) { + virReportOOMError(conn); + return -1; + } + vm->def->disks[vm->def->ndisks++] = dev->data.disk; qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks), virDomainDiskQSort); - VIR_FREE(reply); - return 0; } -- 1.6.2.5

qemu network devices are hot-unplugged in two stages - first the PCI NIC is removed using 'pci_del <pci_addr>' and then the backend is removed using 'host_net_remove <vlan> <name>'. In order to perform these operations we need to have retained the PCI address, backend name and vlan number. * src/qemu_driver.c: add qemudDomainDetachNetDevice() --- src/qemu_driver.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 93 insertions(+), 2 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a3bb650..4fa946c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4906,6 +4906,96 @@ cleanup: return ret; } +static int +qemudDomainDetachNetDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int i, ret = -1; + char *cmd = NULL; + char *reply = NULL; + virDomainNetDefPtr detach = NULL; + + for (i = 0 ; i < vm->def->nnets ; i++) { + virDomainNetDefPtr net = vm->def->nets[i]; + + if (!memcmp(net->mac, dev->data.net->mac, sizeof(net->mac))) { + detach = net; + break; + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("network device %02x:%02x:%02x:%02x:%02x:%02x not found"), + detach->mac[0], detach->mac[1], detach->mac[2], + detach->mac[3], detach->mac[4], detach->mac[5]); + goto cleanup; + } + + if (!detach->pci_addr || !detach->vlan || !detach->hostnet_name) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("network device cannot be detached - device state missing")); + goto cleanup; + } + + if (virAsprintf(&cmd, "pci_del pci_addr=%s", detach->pci_addr) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("network device dettach command '%s' failed"), cmd); + goto cleanup; + } + + DEBUG("%s: pci_del reply: %s", vm->def->name, reply); + + /* If the command fails due to a wrong PCI address qemu prints + * 'invalid pci address'; nothing is printed on success */ + if (strstr(reply, "Invalid pci address")) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach network device: invalid PCI address %s: %s"), + detach->pci_addr, reply); + goto cleanup; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + if (virAsprintf(&cmd, "host_net_remove %d %s", + detach->vlan, detach->hostnet_name) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("network device dettach command '%s' failed"), cmd); + goto cleanup; + } + + DEBUG("%s: host_net_remove reply: %s", vm->def->name, reply); + + if (vm->def->nnets > 1) { + vm->def->nets[i] = vm->def->nets[--vm->def->nnets]; + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } else { + VIR_FREE(vm->def->nets[0]); + vm->def->nnets = 0; + } + ret = 0; + +cleanup: + VIR_FREE(reply); + VIR_FREE(cmd); + return ret; +} + static int qemudDomainDetachDevice(virDomainPtr dom, const char *xml) { struct qemud_driver *driver = dom->conn->privateData; @@ -4944,8 +5034,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom, driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) VIR_WARN0("Fail to restore disk device ownership"); - } - else + } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { + ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); + } else qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", _("only SCSI or virtio disk device can be detached dynamically")); -- 1.6.2.5

On Mon, Jul 20, 2009 at 12:51:24PM +0100, Mark McLoughlin wrote:

qemu network devices are hot-unplugged in two stages - first the PCI NIC is removed using 'pci_del <pci_addr>' and then the backend is removed using 'host_net_remove <vlan> <name>'.

In order to perform these operations we need to have retained the PCI address, backend name and vlan number.

* src/qemu_driver.c: add qemudDomainDetachNetDevice() --- src/qemu_driver.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a3bb650..4fa946c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4906,6 +4906,96 @@ cleanup: return ret; }

+static int +qemudDomainDetachNetDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int i, ret = -1; + char *cmd = NULL; + char *reply = NULL; + virDomainNetDefPtr detach = NULL; + + for (i = 0 ; i < vm->def->nnets ; i++) { + virDomainNetDefPtr net = vm->def->nets[i]; + + if (!memcmp(net->mac, dev->data.net->mac, sizeof(net->mac))) { + detach = net; + break; + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("network device %02x:%02x:%02x:%02x:%02x:%02x not found"), + detach->mac[0], detach->mac[1], detach->mac[2], + detach->mac[3], detach->mac[4], detach->mac[5]); + goto cleanup; + } + + if (!detach->pci_addr || !detach->vlan || !detach->hostnet_name) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("network device cannot be detached - device state missing")); + goto cleanup; + } + + if (virAsprintf(&cmd, "pci_del pci_addr=%s", detach->pci_addr) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("network device dettach command '%s' failed"), cmd); + goto cleanup; + } + + DEBUG("%s: pci_del reply: %s", vm->def->name, reply); + + /* If the command fails due to a wrong PCI address qemu prints + * 'invalid pci address'; nothing is printed on success */ + if (strstr(reply, "Invalid pci address")) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach network device: invalid PCI address %s: %s"), + detach->pci_addr, reply); + goto cleanup; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + if (virAsprintf(&cmd, "host_net_remove %d %s", + detach->vlan, detach->hostnet_name) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("network device dettach command '%s' failed"), cmd); + goto cleanup; + } + + DEBUG("%s: host_net_remove reply: %s", vm->def->name, reply); + + if (vm->def->nnets > 1) { + vm->def->nets[i] = vm->def->nets[--vm->def->nnets]; + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } else { + VIR_FREE(vm->def->nets[0]); + vm->def->nnets = 0; + } + ret = 0; + +cleanup: + VIR_FREE(reply); + VIR_FREE(cmd); + return ret; +} + static int qemudDomainDetachDevice(virDomainPtr dom, const char *xml) { struct qemud_driver *driver = dom->conn->privateData; @@ -4944,8 +5034,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom, driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) VIR_WARN0("Fail to restore disk device ownership"); - } - else + } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { + ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); + } else qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", _("only SCSI or virtio disk device can be detached dynamically"));

--

ACK Daniel -- |: Red Hat, Engineering, London -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 Mon, Jul 20, 2009 at 12:51:20PM +0100, Mark McLoughlin wrote:

Implement basic NIC hotplug support using the 'host_net_add' and 'pci_add' qemu monitor commands.

For now, we don't support 'bridge' or 'network' types.

Also, if pci_add fails, we currently fail to remove the backend which we added.

Urgh, that rather sucks. I guess that is a current QEMU limitation ?

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index cbc185c..cde789e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4492,6 +4492,84 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, return 0; }

+static int qemudDomainAttachNetDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + unsigned int qemuCmdFlags) +{ + virDomainNetDefPtr net = dev->data.net; + char *cmd, *reply; + int i; + + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_HOST_NET_ADD)) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("%s"), + "installed qemu version does not support host_net_add"); + return -1; + } + + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("network device type '%s' cannot be attached"), + virDomainNetTypeToString(net->type)); + return -1; + } + + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) { + virReportOOMError(conn); + return -1; + } + + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && + qemuAssignNetNames(vm->def, net) < 0) { + virReportOOMError(conn); + return -1; + } + + /* Choose a vlan value greater than all other values since + * older versions did not store the value in the state file. + */ + net->vlan = vm->def->nnets; + for (i = 0; i < vm->def->nnets; i++) + if (vm->def->nets[i]->vlan >= net->vlan) + net->vlan = vm->def->nets[i]->vlan; + + if (qemuBuildHostNetStr(conn, net, + "host_net_add ", ' ', net->vlan, -1, &cmd) < 0) + return -1; + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to add network backend with '%s'"), cmd); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + if (qemuBuildNicStr(conn, net, + "pci_add pci_addr=auto ", ' ', net->vlan, &cmd) < 0) { + /* FIXME: try and remove the backend again */ + return -1; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + /* FIXME: try and remove the backend again */ + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to add NIC with '%s'"), cmd); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + vm->def->nets[vm->def->nnets++] = net; + + return 0; +}

I'd recommend adding a nice VIR_DEBUG() line in after each command, including the reply string, so if something goes wrong we can debug it more easily. Regards, Daniel -- |: Red Hat, Engineering, London -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 Mon, Jul 20, 2009 at 12:51:18PM +0100, Mark McLoughlin wrote:

Currently, an interface's vlan number corresponds to its index in the table of network interfaces. That is no longer true when we allow devices to be removed.

To fix this, we store the vlan number in the domain's state XML so that it survives libvirtd restarts.

* src/domain_conf.h: add vlan number to virDomainNetDef

* src/domain_conf.c: store it in XML as <state vlan='N'/>, defaulting to -1 if this is state saved by a previous version of libvirt

* src/qemu_conf.c: assign vlan numbers before starting qemu [...] @@ -3614,6 +3624,8 @@ virDomainNetDefFormat(virConnectPtr conn, virBufferEscapeString(buf, " nic='%s'", def->nic_name); if (def->hostnet_name) virBufferEscapeString(buf, " hostnet='%s'", def->hostnet_name); + if (def->vlan > 0) + virBufferVSprintf(buf, " vlan='%d'", def->vlan); virBufferAddLit(buf, "/>\n"); }

shouldn't we do that only when doing 'internal' dump, otherwise there is a risk to expose this at the public dump level, where adding it is a completely different issue. Otherwise looks fine. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Mon, Jul 20, 2009 at 12:51:18PM +0100, Mark McLoughlin wrote:

Currently, an interface's vlan number corresponds to its index in the table of network interfaces. That is no longer true when we allow devices to be removed.

To fix this, we store the vlan number in the domain's state XML so that it survives libvirtd restarts.

* src/domain_conf.h: add vlan number to virDomainNetDef

* src/domain_conf.c: store it in XML as <state vlan='N'/>, defaulting to -1 if this is state saved by a previous version of libvirt

* src/qemu_conf.c: assign vlan numbers before starting qemu

Am I right in thinking we only need to preserve this info so that we can figure out the next free VLAN number for hot-plug ?

--- src/domain_conf.c | 12 ++++++++++++ src/domain_conf.h | 1 + src/qemu_conf.c | 7 +++++-- 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/domain_conf.c b/src/domain_conf.c index 2b4fe90..cce863c 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -954,6 +954,7 @@ virDomainNetDefParseXML(virConnectPtr conn, char *internal = NULL; char *nic_name = NULL; char *hostnet_name = NULL; + char *vlan = NULL;

if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -1023,6 +1024,7 @@ virDomainNetDefParseXML(virConnectPtr conn, xmlStrEqual(cur->name, BAD_CAST "state")) { nic_name = virXMLPropString(cur, "nic"); hostnet_name = virXMLPropString(cur, "hostnet"); + vlan = virXMLPropString(cur, "vlan"); } } cur = cur->next; @@ -1038,6 +1040,13 @@ virDomainNetDefParseXML(virConnectPtr conn, def->hostnet_name = hostnet_name; nic_name = hostnet_name = NULL;

+ def->vlan = -1; + if (vlan && virStrToLong_i(vlan, NULL, 10, &def->vlan) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <state> 'vlan' attribute")); + goto error; + }

I'm wondering if we shouldn't just add a virXMLPropInt() method which would mean we wouldn't have to keep doing the virStrToLong stuff throughout the XML parsing code. Daniel -- |: Red Hat, Engineering, London -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 Mon, Jul 20, 2009 at 12:51:15PM +0100, Mark McLoughlin wrote:

Add QEMUD_CMD_FLAG_NET_NAME to indicate that '-net ...,name=foo' is supported and QEMUD_CMD_FLAG_HOST_NET_ADD to indicate that the 'host_net_add' monitor command is available.

Set both these flags if the qemu version is greater than 0.10.0. Checking via the '-help' output would not work for the monitor command and even for the command line arg, it would be quite fragile.

* src/qemu_conf.h: add new flags as aliases of QEMUD_CMD_FLAG_0_10

* src/qemu_conf.c: set QEMUD_CMD_FLAG_0_10 for versions >= 0.10.0

* tests/qemuhelptest.c: set QEMUD_CMD_FLAG_0_10 for the appropriate qemu versions [...] --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -58,6 +58,11 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_KVM = (1 << 13), /* Whether KVM is compiled in */ QEMUD_CMD_FLAG_DRIVE_FORMAT = (1 << 14), /* Is -drive format= avail */ QEMUD_CMD_FLAG_VGA = (1 << 15), /* Is -vga avail */ + + /* features added in qemu-0.10.0 */ + QEMUD_CMD_FLAG_0_10 = (1 << 16), + QEMUD_CMD_FLAG_NET_NAME = QEMUD_CMD_FLAG_0_10, /* -net ...,name=str */ + QEMUD_CMD_FLAG_HOST_NET_ADD = QEMUD_CMD_FLAG_0_10, /* host_net_add monitor command */ };

Hum, defining multiple time the same value in an enum, maybe that's fine but that looks weird to me, especially as each entry so far was about separated capabilities, independantly of the potential version. Not a big deal but what do others think ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Mon, Jul 20, 2009 at 12:51:15PM +0100, Mark McLoughlin wrote:

Add QEMUD_CMD_FLAG_NET_NAME to indicate that '-net ...,name=foo' is supported and QEMUD_CMD_FLAG_HOST_NET_ADD to indicate that the 'host_net_add' monitor command is available.

Set both these flags if the qemu version is greater than 0.10.0. Checking via the '-help' output would not work for the monitor command and even for the command line arg, it would be quite fragile.

* src/qemu_conf.h: add new flags as aliases of QEMUD_CMD_FLAG_0_10

* src/qemu_conf.c: set QEMUD_CMD_FLAG_0_10 for versions >= 0.10.0

* tests/qemuhelptest.c: set QEMUD_CMD_FLAG_0_10 for the appropriate qemu versions --- src/qemu_conf.c | 3 +++ src/qemu_conf.h | 5 +++++ tests/qemuhelptest.c | 9 ++++++--- 3 files changed, 14 insertions(+), 3 deletions(-)

ACK

diff --git a/src/qemu_conf.c b/src/qemu_conf.c index ba99652..a9e5e4e 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -527,6 +527,9 @@ static unsigned int qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_MIGRATE_KVM_STDIO; }

+ if (version >= 10000) + flags |= QEMUD_CMD_FLAG_0_10; + return flags; }

diff --git a/src/qemu_conf.h b/src/qemu_conf.h index fbf2ab9..1b2d061 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -58,6 +58,11 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_KVM = (1 << 13), /* Whether KVM is compiled in */ QEMUD_CMD_FLAG_DRIVE_FORMAT = (1 << 14), /* Is -drive format= avail */ QEMUD_CMD_FLAG_VGA = (1 << 15), /* Is -vga avail */ + + /* features added in qemu-0.10.0 */ + QEMUD_CMD_FLAG_0_10 = (1 << 16), + QEMUD_CMD_FLAG_NET_NAME = QEMUD_CMD_FLAG_0_10, /* -net ...,name=str */ + QEMUD_CMD_FLAG_HOST_NET_ADD = QEMUD_CMD_FLAG_0_10, /* host_net_add monitor command */ };

This looks a bit wierd, but then again we're very soon going to rnu out of bits if we keep using up flags, so re-using the flag is OK I reckon. Besides which, this is internal code, so we can change it it we decide we don't like it later. Daniel -- |: Red Hat, Engineering, London -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 Mon, 2009-07-20 at 14:42 +0100, Daniel P. Berrange wrote:

On Mon, Jul 20, 2009 at 12:51:12PM +0100, Mark McLoughlin wrote:

...

When we hot-plug a disk device into a qemu guest, we need to retain its PCI address so that it can be removed again later. Currently, we do retain the slot number, but not across libvirtd restarts.

Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the domain and bus number, but the format allows us to do that in future.

* src/domain_conf.h: replace slotnum with pci_addr

...

diff --git a/src/domain_conf.h b/src/domain_conf.h index 6e111fa..1766b61 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -106,7 +106,7 @@ struct _virDomainDiskDef { int cachemode; unsigned int readonly : 1; unsigned int shared : 1; - int slotnum; /* pci slot number for unattach */ + char *pci_addr; /* for detach */ };

I think it'd be nicer to store the parsed address here as a nested struct with domain, bus, slot.

It is not really saving us trouble by using a string, since most of the places using this field end up asprintf'ing it into another string, or even having to extract pieces out of it again.

It's saving us trouble because you don't have to code the equivalent of virDomainHostdevSubsysPciDefParseXML() and have e.g. <state> <address domain="0" bus="0" slot="5"/> </state> I just now started to do it and then realized how much extra hassle the XML parsing was going to be. All for some internal data that we use in textual format anyway. Are you sure? :-) Cheers, Mark.

On Mon, 2009-07-20 at 16:38 +0200, Daniel Veillard wrote:

On Mon, Jul 20, 2009 at 03:05:31PM +0100, Mark McLoughlin wrote:

...

On Mon, 2009-07-20 at 14:42 +0100, Daniel P. Berrange wrote:

...

On Mon, Jul 20, 2009 at 12:51:12PM +0100, Mark McLoughlin wrote:

...

When we hot-plug a disk device into a qemu guest, we need to retain its PCI address so that it can be removed again later. Currently, we do retain the slot number, but not across libvirtd restarts.

Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the domain and bus number, but the format allows us to do that in future.

* src/domain_conf.h: replace slotnum with pci_addr

...

diff --git a/src/domain_conf.h b/src/domain_conf.h index 6e111fa..1766b61 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -106,7 +106,7 @@ struct _virDomainDiskDef { int cachemode; unsigned int readonly : 1; unsigned int shared : 1; - int slotnum; /* pci slot number for unattach */ + char *pci_addr; /* for detach */ };

I think it'd be nicer to store the parsed address here as a nested struct with domain, bus, slot.

I understand dan 'here' as in the C struct not in the XML

...

...

It is not really saving us trouble by using a string, since most of the places using this field end up asprintf'ing it into another string, or even having to extract pieces out of it again.

It's saving us trouble because you don't have to code the equivalent of virDomainHostdevSubsysPciDefParseXML() and have e.g.

<state> <address domain="0" bus="0" slot="5"/> </state>

I just now started to do it and then realized how much extra hassle the XML parsing was going to be. All for some internal data that we use in textual format anyway. Are you sure? :-)

Well a single string in the XML is fine, but in the parsed Def let's keep the bits as fully parsed, i.e. the set of ints we extract in patch 12/14 Agreed with Dan, Agreed that separating them in the XML will make the code way more complex especially for error handling.

Okay, here's how that looks. Personally, I think even this much adds complexity (parsing the devaddr param from the XML, re-formatting it in several places, not being able to just check pci_addr != NULL) without much benefit. Cheers, Mark. From: Mark McLoughlin <markmc@redhat.com> Subject: [PATCH 02/14] Retain disk PCI address across libvirtd restarts When we hot-plug a disk device into a qemu guest, we need to retain its PCI address so that it can be removed again later. Currently, we do retain the slot number, but not across libvirtd restarts. Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the domain and bus number, but the format allows us to do that in future. * src/domain_conf.h: replace slotnum with pci_addr struct, add helper for testing whether the address is valid * src/domain_conf.c: handle formatting and parsing the address * src/qemu_driver.c: store the parsed slot number as a full PCI address, and use this address with the pci_del monitor command * src/vbox/vbox_tmpl.c: we're debug printing slotnum here even though it can never be set, just delete it --- src/domain_conf.c | 33 ++++++++++++++++++++++++++++++--- src/domain_conf.h | 12 +++++++++++- src/qemu_driver.c | 36 ++++++++++++++++++++++++------------ src/vbox/vbox_tmpl.c | 1 - 4 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 10e6ac6..91a6c6e 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -643,7 +643,7 @@ int virDomainDiskCompare(virDomainDiskDefPtr a, static virDomainDiskDefPtr virDomainDiskDefParseXML(virConnectPtr conn, xmlNodePtr node, - int flags ATTRIBUTE_UNUSED) { + int flags) { virDomainDiskDefPtr def; xmlNodePtr cur; char *type = NULL; @@ -654,6 +654,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *target = NULL; char *bus = NULL; char *cachetag = NULL; + char *devaddr = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -708,6 +709,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { def->shared = 1; + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && + xmlStrEqual(cur->name, BAD_CAST "state")) { + devaddr = virXMLPropString(cur, "devaddr"); } } cur = cur->next; @@ -807,6 +811,17 @@ virDomainDiskDefParseXML(virConnectPtr conn, goto error; } + if (devaddr && + sscanf(devaddr, "%x:%x:%x", + &def->pci_addr.domain, + &def->pci_addr.bus, + &def->pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse devaddr parameter '%s'"), + devaddr); + goto error; + } + def->src = source; source = NULL; def->dst = target; @@ -825,6 +840,7 @@ cleanup: VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(cachetag); + VIR_FREE(devaddr); return def; @@ -3388,7 +3404,8 @@ virDomainLifecycleDefFormat(virConnectPtr conn, static int virDomainDiskDefFormat(virConnectPtr conn, virBufferPtr buf, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + int flags) { const char *type = virDomainDiskTypeToString(def->type); const char *device = virDomainDiskDeviceTypeToString(def->device); @@ -3446,6 +3463,16 @@ virDomainDiskDefFormat(virConnectPtr conn, if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { + virBufferAddLit(buf, " <state"); + if (virDiskHasValidPciAddr(def)) + virBufferVSprintf(buf, " devaddr='%.4x:%.2x:%.2x'", + def->pci_addr.domain, + def->pci_addr.bus, + def->pci_addr.slot); + virBufferAddLit(buf, "/>\n"); + } + virBufferAddLit(buf, " </disk>\n"); return 0; @@ -4048,7 +4075,7 @@ char *virDomainDefFormat(virConnectPtr conn, def->emulator); for (n = 0 ; n < def->ndisks ; n++) - if (virDomainDiskDefFormat(conn, &buf, def->disks[n]) < 0) + if (virDomainDiskDefFormat(conn, &buf, def->disks[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nfss ; n++) diff --git a/src/domain_conf.h b/src/domain_conf.h index 69b665f..c0fdd65 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -111,9 +111,19 @@ struct _virDomainDiskDef { int cachemode; unsigned int readonly : 1; unsigned int shared : 1; - int slotnum; /* pci slot number for unattach */ + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } pci_addr; }; +static inline int +virDiskHasValidPciAddr(virDomainDiskDefPtr def) +{ + return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; +} + /* Two types of disk backends */ enum virDomainFSType { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 00dc6e5..a87ef70 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4390,6 +4390,8 @@ try_command: return -1; } + VIR_FREE(cmd); + DEBUG ("%s: pci_add reply: %s", vm->def->name, reply); /* If the command succeeds qemu prints: * OK bus 0, slot XXX... @@ -4399,22 +4401,25 @@ try_command: if ((s = strstr(reply, "OK ")) && (s = strstr(s, "slot "))) { char *dummy = s; + unsigned slot; + s += strlen("slot "); - if (virStrToLong_i ((const char*)s, &dummy, 10, &dev->data.disk->slotnum) == -1) + if (virStrToLong_ui((const char*)s, &dummy, 10, &slot) == -1) VIR_WARN("%s", _("Unable to parse slot number\n")); + /* XXX not neccessarily always going to end up in domain 0 / bus 0 :-( */ - /* XXX this slotnum is not persistant across restarts :-( */ + dev->data.disk->pci_addr.domain = 0; + dev->data.disk->pci_addr.bus = 0; + dev->data.disk->pci_addr.slot = slot; } else if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { VIR_FREE(reply); - VIR_FREE(cmd); tryOldSyntax = 1; goto try_command; } else { qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("adding %s disk failed: %s"), type, reply); VIR_FREE(reply); - VIR_FREE(cmd); return -1; } @@ -4423,7 +4428,7 @@ try_command: virDomainDiskQSort); VIR_FREE(reply); - VIR_FREE(cmd); + return 0; } @@ -4660,21 +4665,24 @@ static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, goto cleanup; } - if (detach->slotnum < 1) { + if (!virDiskHasValidPciAddr(detach)) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - invalid slot number %d"), - detach->dst, detach->slotnum); + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); goto cleanup; } try_command: if (tryOldSyntax) { - if (virAsprintf(&cmd, "pci_del 0 %d", detach->slotnum) < 0) { + if (virAsprintf(&cmd, "pci_del 0 %.2x", detach->pci_addr.slot) < 0) { virReportOOMError(conn); goto cleanup; } } else { - if (virAsprintf(&cmd, "pci_del pci_addr=0:0:%d", detach->slotnum) < 0) { + if (virAsprintf(&cmd, "pci_del pci_addr=%.4x:%.2x:%.2x", + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot) < 0) { virReportOOMError(conn); goto cleanup; } @@ -4698,8 +4706,12 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to detach disk %s: invalid slot %d: %s"), - detach->dst, detach->slotnum, reply); + _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->dst, + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, + reply); goto cleanup; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 3208f03..a2b958a 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2712,7 +2712,6 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { DEBUG("disk(%d) cachemode: %d", i, def->disks[i]->cachemode); DEBUG("disk(%d) readonly: %s", i, def->disks[i]->readonly ? "True" : "False"); DEBUG("disk(%d) shared: %s", i, def->disks[i]->shared ? "True" : "False"); - DEBUG("disk(%d) slotnum: %d", i, def->disks[i]->slotnum); if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { -- 1.6.2.5

On Mon, Jul 20, 2009 at 12:51:11PM +0100, Mark McLoughlin wrote:

We need to store things like device names and PCI slot numbers in the qemu domain state file so that we don't lose that information on libvirtd restart. Add a flag to indicate that this information should be parsed or formatted.

* include/libvirt/libvirt.h: add VIR_DOMAIN_XML_INTERNAL_STATUS

* src/domain_conf.c: pass the flag from virDomainObjParseXML() and virDomainSaveStatus

* src/libvirt.c: reject the flag in virDomainGetXMLDesc() --- include/libvirt/libvirt.h | 3 ++- include/libvirt/libvirt.h.in | 3 ++- src/domain_conf.c | 8 ++++---- src/libvirt.c | 6 ++++++ 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h index 90007a1..07495fc 100644 --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -585,7 +585,8 @@ int virDomainGetSecurityLabel (virDomainPtr domain,

typedef enum { VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ + VIR_DOMAIN_XML_INACTIVE = 2, /* dump inactive domain information */ + VIR_DOMAIN_XML_INTERNAL_STATUS = 4, /* dump internal domain status information */ } virDomainXMLFlags;

char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index ba2b6f0..6794c61 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -585,7 +585,8 @@ int virDomainGetSecurityLabel (virDomainPtr domain,

typedef enum { VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ + VIR_DOMAIN_XML_INACTIVE = 2, /* dump inactive domain information */ + VIR_DOMAIN_XML_INTERNAL_STATUS = 4, /* dump internal domain status information */ } virDomainXMLFlags;

char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/domain_conf.c b/src/domain_conf.c index f3e4c6c..10e6ac6 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2896,7 +2896,8 @@ static virDomainObjPtr virDomainObjParseXML(virConnectPtr conn,

oldnode = ctxt->node; ctxt->node = config; - obj->def = virDomainDefParseXML(conn, caps, ctxt, 0); + obj->def = virDomainDefParseXML(conn, caps, ctxt, + VIR_DOMAIN_XML_INTERNAL_STATUS); ctxt->node = oldnode; if (!obj->def) goto error; @@ -4277,12 +4278,11 @@ int virDomainSaveStatus(virConnectPtr conn, const char *statusDir, virDomainObjPtr obj) { + int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS; int ret = -1; char *xml;

- if (!(xml = virDomainObjFormat(conn, - obj, - VIR_DOMAIN_XML_SECURE))) + if (!(xml = virDomainObjFormat(conn, obj, flags))) goto cleanup;

if (virDomainSaveXML(conn, statusDir, obj->def, xml)) diff --git a/src/libvirt.c b/src/libvirt.c index f4a7fa7..c8926b3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2730,6 +2730,12 @@ virDomainGetXMLDesc(virDomainPtr domain, int flags) goto error; }

+ if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { + virLibConnError(conn, VIR_ERR_OPERATION_DENIED, + _("virDomainGetXMLDesc with internal status flag")); + goto error; + } + if (conn->driver->domainDumpXML) { char *ret; ret = conn->driver->domainDumpXML (domain, flags);

Hum, that's very confusing. Why expose that flag at the API level but forbid it's use from the API ? Seems to me adding an extra parameter to the internal function virDomainDefParseXML() is a far cleaner way to do things by looking at this patch. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Mon, Jul 20, 2009 at 02:18:38PM +0200, Daniel Veillard wrote:

On Mon, Jul 20, 2009 at 12:51:11PM +0100, Mark McLoughlin wrote:

...

We need to store things like device names and PCI slot numbers in the qemu domain state file so that we don't lose that information on libvirtd restart. Add a flag to indicate that this information should be parsed or formatted.

* include/libvirt/libvirt.h: add VIR_DOMAIN_XML_INTERNAL_STATUS

* src/domain_conf.c: pass the flag from virDomainObjParseXML() and virDomainSaveStatus

* src/libvirt.c: reject the flag in virDomainGetXMLDesc() --- include/libvirt/libvirt.h | 3 ++- include/libvirt/libvirt.h.in | 3 ++- src/domain_conf.c | 8 ++++---- src/libvirt.c | 6 ++++++ 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h index 90007a1..07495fc 100644 --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -585,7 +585,8 @@ int virDomainGetSecurityLabel (virDomainPtr domain,

typedef enum { VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ + VIR_DOMAIN_XML_INACTIVE = 2, /* dump inactive domain information */ + VIR_DOMAIN_XML_INTERNAL_STATUS = 4, /* dump internal domain status information */ } virDomainXMLFlags;

char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index ba2b6f0..6794c61 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -585,7 +585,8 @@ int virDomainGetSecurityLabel (virDomainPtr domain,

typedef enum { VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ + VIR_DOMAIN_XML_INACTIVE = 2, /* dump inactive domain information */ + VIR_DOMAIN_XML_INTERNAL_STATUS = 4, /* dump internal domain status information */ } virDomainXMLFlags;

char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/domain_conf.c b/src/domain_conf.c index f3e4c6c..10e6ac6 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2896,7 +2896,8 @@ static virDomainObjPtr virDomainObjParseXML(virConnectPtr conn,

oldnode = ctxt->node; ctxt->node = config; - obj->def = virDomainDefParseXML(conn, caps, ctxt, 0); + obj->def = virDomainDefParseXML(conn, caps, ctxt, + VIR_DOMAIN_XML_INTERNAL_STATUS); ctxt->node = oldnode; if (!obj->def) goto error; @@ -4277,12 +4278,11 @@ int virDomainSaveStatus(virConnectPtr conn, const char *statusDir, virDomainObjPtr obj) { + int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS; int ret = -1; char *xml;

- if (!(xml = virDomainObjFormat(conn, - obj, - VIR_DOMAIN_XML_SECURE))) + if (!(xml = virDomainObjFormat(conn, obj, flags))) goto cleanup;

if (virDomainSaveXML(conn, statusDir, obj->def, xml)) diff --git a/src/libvirt.c b/src/libvirt.c index f4a7fa7..c8926b3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2730,6 +2730,12 @@ virDomainGetXMLDesc(virDomainPtr domain, int flags) goto error; }

+ if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { + virLibConnError(conn, VIR_ERR_OPERATION_DENIED, + _("virDomainGetXMLDesc with internal status flag")); + goto error; + } + if (conn->driver->domainDumpXML) { char *ret; ret = conn->driver->domainDumpXML (domain, flags);

Hum, that's very confusing. Why expose that flag at the API level but forbid it's use from the API ? Seems to me adding an extra parameter to the internal function virDomainDefParseXML() is a far cleaner way to do things by looking at this patch.

It'd be nice to only have 1 flag parameter for the internal methods. Having 'flags' and 'privateFlags' to the same method is just going to lead to code errors, passing the wrong flag in and it silently failing We should not be adding this to the public API header file though. Since we only have 2 flags in use currently, lets just mask off the top 16 bits for internal use. So in domain_conf.h add a enum starting at the 16th bit typedef enum { VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ } virDomainXMLInternalFlags; And to be sure no one abusing this from public API, in virDomainGetXMLDesc() scrub the incoming flags flags = flags & 0xffff; Regards, Daniel -- |: Red Hat, Engineering, London -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 Mon, 2009-07-20 at 17:03 +0100, Daniel P. Berrange wrote:

On Mon, Jul 20, 2009 at 02:54:34PM +0100, Mark McLoughlin wrote:

...

On Mon, 2009-07-20 at 14:06 +0100, Daniel P. Berrange wrote:

...

On Mon, Jul 20, 2009 at 02:18:38PM +0200, Daniel Veillard wrote:

...

Hum, that's very confusing. Why expose that flag at the API level but forbid it's use from the API ? Seems to me adding an extra parameter to the internal function virDomainDefParseXML() is a far cleaner way to do things by looking at this patch.

It'd be nice to only have 1 flag parameter for the internal methods. Having 'flags' and 'privateFlags' to the same method is just going to lead to code errors, passing the wrong flag in and it silently failing

We should not be adding this to the public API header file though.

Since we only have 2 flags in use currently, lets just mask off the top 16 bits for internal use.

So in domain_conf.h add a enum starting at the 16th bit

typedef enum { VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ } virDomainXMLInternalFlags;

And to be sure no one abusing this from public API, in virDomainGetXMLDesc() scrub the incoming flags

flags = flags & 0xffff;

How's this?

Cheers, Mark.

From: Mark McLoughlin <markmc@redhat.com> Subject: [PATCH 01/14] Add internal XML parsing/formatting flag

We need to store things like device names and PCI slot numbers in the qemu domain state file so that we don't lose that information on libvirtd restart. Add a flag to indicate that this information should be parsed or formatted.

Make bit 16 and above of the flags bitmask for internal use only and consume the first bit for this new status flag.

* include/libvirt/libvirt.h: add VIR_DOMAIN_XML_FLAGS_MASK

* src/libvirt.c: reject private flags in virDomainGetXMLDesc()

* src/domain_conf.h: add VIR_DOMAIN_XML_INTERNAL_STATUS

* src/domain_conf.c: pass the flag from virDomainObjParseXML() and virDomainSaveStatus --- include/libvirt/libvirt.h | 5 +++-- include/libvirt/libvirt.h.in | 5 +++-- src/domain_conf.c | 8 ++++---- src/domain_conf.h | 5 +++++ src/libvirt.c | 6 ++++++ 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h index 90007a1..d1cf5fb 100644 --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -584,8 +584,9 @@ int virDomainGetSecurityLabel (virDomainPtr domain, */

typedef enum { - VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ + VIR_DOMAIN_XML_SECURE = (1<<0), /* dump security sensitive information too */ + VIR_DOMAIN_XML_INACTIVE = (1<<1), /* dump inactive domain information */ + VIR_DOMAIN_XML_FLAGS_MASK = 0xffff, /* bits 16 and above are for internal use */ } virDomainXMLFlags;

IMHO, this should be kept in the private header file. The fact that we have hack reserving some portion for our internal use doesn't need to be exposed to applications.

Okay, I put it in libvirt_internal.h, assuming you don't want domain_conf.h included in libvirt.c

...

diff --git a/src/libvirt.c b/src/libvirt.c index f4a7fa7..5507750 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2730,6 +2730,12 @@ virDomainGetXMLDesc(virDomainPtr domain, int flags) goto error; }

+ if (flags != (flags & VIR_DOMAIN_XML_FLAGS_MASK)) { + virLibConnError(conn, VIR_ERR_OPERATION_DENIED, + _("virDomainGetXMLDesc with internal flags")); + goto error; + }

Here I just think we should be masking the flags off silently. There's no need to explicit tell apps about the existance of internal flags.

Okay, done Cheers, Mark.

On Wed, 2009-07-22 at 11:24 +0100, Daniel P. Berrange wrote:

On Mon, Jul 20, 2009 at 06:20:06PM +0100, Mark McLoughlin wrote:

...

On Mon, 2009-07-20 at 17:03 +0100, Daniel P. Berrange wrote:

...

On Mon, Jul 20, 2009 at 02:54:34PM +0100, Mark McLoughlin wrote:

...

diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h index 90007a1..d1cf5fb 100644 --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -584,8 +584,9 @@ int virDomainGetSecurityLabel (virDomainPtr domain, */

typedef enum { - VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ + VIR_DOMAIN_XML_SECURE = (1<<0), /* dump security sensitive information too */ + VIR_DOMAIN_XML_INACTIVE = (1<<1), /* dump inactive domain information */ + VIR_DOMAIN_XML_FLAGS_MASK = 0xffff, /* bits 16 and above are for internal use */ } virDomainXMLFlags;

IMHO, this should be kept in the private header file. The fact that we have hack reserving some portion for our internal use doesn't need to be exposed to applications.

Okay, I put it in libvirt_internal.h, assuming you don't want domain_conf.h included in libvirt.c

...

...

diff --git a/src/libvirt.c b/src/libvirt.c index f4a7fa7..5507750 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2730,6 +2730,12 @@ virDomainGetXMLDesc(virDomainPtr domain, int flags) goto error; }

+ if (flags != (flags & VIR_DOMAIN_XML_FLAGS_MASK)) { + virLibConnError(conn, VIR_ERR_OPERATION_DENIED, + _("virDomainGetXMLDesc with internal flags")); + goto error; + }

Here I just think we should be masking the flags off silently. There's no need to explicit tell apps about the existance of internal flags.

Okay, done

ACK to this whole series now. I've done some automated testing of it and it seems to be working reasonably reliably.

Thanks, pushed now. Cheers, Mark.