[libvirt] [PATCH 00/13] Fix device hotplug with QEMU and -device

The current libvirt code enables use of -device when launching QEMU but forgot to update the equivalent code for hotpluging devices to an already running QEMU. This series implements the latter, doing three core things - Assign a unique PCI slot to the new device - Assign a unique device alias to the new device - Call the device_add function for attaching the device As a minor flaw in the whole plan, it turns out the new -netdev code in QEMU 0.12 does not support hotplugging at all. So this series also switching network setup back to using the old VLAN based syntax. Hopefully we can re-introduce use of -netdev with QEMU 0.13, since it gives important performance benefits.

All the helper functions for building command line arguments now return a 'char *', instead of acepting a 'char **' or virBufferPtr argument * qemu/qemu_conf.c: Standardize syntax for building args * qemu/qemu_conf.h: Export all functions for building args * qemu/qemu_driver.c: Update for changed syntax for building NIC/hostnet args --- src/qemu/qemu_conf.c | 341 +++++++++++++++++++++++------------------------- src/qemu/qemu_conf.h | 56 ++++++-- src/qemu/qemu_driver.c | 6 +- 3 files changed, 211 insertions(+), 192 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index fd2acc1..232dc5d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2057,10 +2057,10 @@ error: return NULL; } -static int + +char * qemuBuildDriveDevStr(virConnectPtr conn, - virDomainDiskDefPtr disk, - char **str) + virDomainDiskDefPtr disk) { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); @@ -2101,17 +2101,20 @@ qemuBuildDriveDevStr(virConnectPtr conn, virBufferVSprintf(&opt, ",drive=drive-%s", disk->info.alias); virBufferVSprintf(&opt, ",id=%s", disk->info.alias); - *str = virBufferContentAndReset(&opt); - return 0; + if (virBufferError(&opt)) { + virReportOOMError(NULL); + goto error; + } + + return virBufferContentAndReset(&opt); error: virBufferFreeAndReset(&opt); - *str = NULL; - return -1; + return NULL; } -static char * +char * qemuBuildControllerDevStr(virDomainControllerDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2131,8 +2134,10 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) if (qemuBuildDeviceAddressStr(&buf, &def->info) < 0) goto error; - if (virBufferError(&buf)) + if (virBufferError(&buf)) { + virReportOOMError(NULL); goto error; + } return virBufferContentAndReset(&buf); @@ -2142,14 +2147,14 @@ error: } -int +char * qemuBuildNicStr(virConnectPtr conn, virDomainNetDefPtr net, const char *prefix, - int vlan, - char **str) + int vlan) { - if (virAsprintf(str, + char *str; + if (virAsprintf(&str, "%smacaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s%s%s%s", prefix ? prefix : "", net->mac[0], net->mac[1], @@ -2161,13 +2166,14 @@ qemuBuildNicStr(virConnectPtr conn, (net->info.alias ? ",name=" : ""), (net->info.alias ? net->info.alias : "")) < 0) { virReportOOMError(conn); - return -1; + return NULL; } - return 0; + return str; } -static char * + +char * qemuBuildNicDevStr(virDomainNetDefPtr net) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2202,23 +2208,25 @@ error: return NULL; } -int + +char * qemuBuildHostNetStr(virConnectPtr conn, virDomainNetDefPtr net, char type_sep, int vlan, - const char *tapfd, - char **str) + const char *tapfd) { + char *str = NULL; + switch (net->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: - if (virAsprintf(str, "tap%cfd=%s,vlan=%d%s%s", + if (virAsprintf(&str, "tap%cfd=%s,vlan=%d%s%s", type_sep, tapfd, vlan, (net->hostnet_name ? ",name=" : ""), (net->hostnet_name ? net->hostnet_name : "")) < 0) { virReportOOMError(conn); - return -1; + return NULL; } break; @@ -2245,10 +2253,10 @@ qemuBuildHostNetStr(virConnectPtr conn, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(conn); - return -1; + return NULL; } - *str = virBufferContentAndReset(&buf); + str = virBufferContentAndReset(&buf); } break; @@ -2270,7 +2278,7 @@ qemuBuildHostNetStr(virConnectPtr conn, break; } - if (virAsprintf(str, "socket%c%s=%s:%d,vlan=%d%s%s", + if (virAsprintf(&str, "socket%c%s=%s:%d,vlan=%d%s%s", type_sep, mode, net->data.socket.address, net->data.socket.port, @@ -2278,40 +2286,41 @@ qemuBuildHostNetStr(virConnectPtr conn, (net->hostnet_name ? ",name=" : ""), (net->hostnet_name ? net->hostnet_name : "")) < 0) { virReportOOMError(conn); - return -1; + return NULL; } } break; case VIR_DOMAIN_NET_TYPE_USER: default: - if (virAsprintf(str, "user%cvlan=%d%s%s", + if (virAsprintf(&str, "user%cvlan=%d%s%s", type_sep, vlan, (net->hostnet_name ? ",name=" : ""), (net->hostnet_name ? net->hostnet_name : "")) < 0) { virReportOOMError(conn); - return -1; + return NULL; } break; } - return 0; + return str; } -static int +char * qemuBuildNetDevStr(virConnectPtr conn, virDomainNetDefPtr net, - const char *tapfd, - char **str) + const char *tapfd) { + char *str = NULL; + switch (net->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: - if (virAsprintf(str, "tap,fd=%s,id=%s", + if (virAsprintf(&str, "tap,fd=%s,id=%s", tapfd, net->hostnet_name) < 0) { virReportOOMError(conn); - return -1; + return NULL; } break; @@ -2331,10 +2340,10 @@ qemuBuildNetDevStr(virConnectPtr conn, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(conn); - return -1; + return NULL; } - *str = virBufferContentAndReset(&buf); + str = virBufferContentAndReset(&buf); } break; @@ -2356,32 +2365,33 @@ qemuBuildNetDevStr(virConnectPtr conn, break; } - if (virAsprintf(str, "socket,%s=%s:%d,id=%s", + if (virAsprintf(&str, "socket,%s=%s:%d,id=%s", mode, net->data.socket.address, net->data.socket.port, net->hostnet_name) < 0) { virReportOOMError(conn); - return -1; + return NULL; } } break; case VIR_DOMAIN_NET_TYPE_USER: default: - if (virAsprintf(str, "user,id=%s", + if (virAsprintf(&str, "user,id=%s", net->hostnet_name) < 0) { virReportOOMError(conn); - return -1; + return NULL; } break; } - return 0; + return str; } -static char *qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev) +char * +qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2410,7 +2420,8 @@ error: } -static char *qemuBuildUSBInputDevStr(virDomainInputDefPtr dev) +char * +qemuBuildUSBInputDevStr(virDomainInputDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2432,7 +2443,7 @@ error: } -static char * +char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2468,7 +2479,7 @@ error: } -static char * +char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2494,43 +2505,46 @@ error: return NULL; } + /* This function outputs a -chardev command line option which describes only the * host side of the character device */ -static void qemudBuildCommandLineChrDevChardevStr(virDomainChrDefPtr dev, - virBufferPtr buf) +char * +qemuBuildChrChardevStr(virDomainChrDefPtr dev) { + virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; + switch(dev->type) { case VIR_DOMAIN_CHR_TYPE_NULL: - virBufferVSprintf(buf, "null,id=%s", dev->info.alias); + virBufferVSprintf(&buf, "null,id=%s", dev->info.alias); break; case VIR_DOMAIN_CHR_TYPE_VC: - virBufferVSprintf(buf, "vc,id=%s", dev->info.alias); + virBufferVSprintf(&buf, "vc,id=%s", dev->info.alias); break; case VIR_DOMAIN_CHR_TYPE_PTY: - virBufferVSprintf(buf, "pty,id=%s", dev->info.alias); + virBufferVSprintf(&buf, "pty,id=%s", dev->info.alias); break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferVSprintf(buf, "tty,id=%s,path=%s", dev->info.alias, dev->data.file.path); + virBufferVSprintf(&buf, "tty,id=%s,path=%s", dev->info.alias, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_FILE: - virBufferVSprintf(buf, "file,id=%s,path=%s", dev->info.alias, dev->data.file.path); + virBufferVSprintf(&buf, "file,id=%s,path=%s", dev->info.alias, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferVSprintf(buf, "pipe,id=%s,path=%s", dev->info.alias, dev->data.file.path); + virBufferVSprintf(&buf, "pipe,id=%s,path=%s", dev->info.alias, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_STDIO: - virBufferVSprintf(buf, "stdio,id=%s", dev->info.alias); + virBufferVSprintf(&buf, "stdio,id=%s", dev->info.alias); break; case VIR_DOMAIN_CHR_TYPE_UDP: - virBufferVSprintf(buf, + virBufferVSprintf(&buf, "udp,id=%s,host=%s,port=%s,localaddr=%s,localport=%s", dev->info.alias, dev->data.udp.connectHost, @@ -2541,7 +2555,7 @@ static void qemudBuildCommandLineChrDevChardevStr(virDomainChrDefPtr dev, case VIR_DOMAIN_CHR_TYPE_TCP: telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; - virBufferVSprintf(buf, + virBufferVSprintf(&buf, "socket,id=%s,host=%s,port=%s%s%s", dev->info.alias, dev->data.tcp.host, @@ -2551,49 +2565,66 @@ static void qemudBuildCommandLineChrDevChardevStr(virDomainChrDefPtr dev, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferVSprintf(buf, + virBufferVSprintf(&buf, "socket,id=%s,path=%s%s", dev->info.alias, dev->data.nix.path, dev->data.nix.listen ? ",server,nowait" : ""); break; } + + if (virBufferError(&buf)) { + virReportOOMError(NULL); + goto error; + } + + return virBufferContentAndReset(&buf); + +error: + virBufferFreeAndReset(&buf); + return NULL; } -static void qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, - virBufferPtr buf) + +char * +qemuBuildChrArgStr(virDomainChrDefPtr dev, const char *prefix) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (prefix) + virBufferAdd(&buf, prefix, strlen(prefix)); + switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_NULL: - virBufferAddLit(buf, "null"); + virBufferAddLit(&buf, "null"); break; case VIR_DOMAIN_CHR_TYPE_VC: - virBufferAddLit(buf, "vc"); + virBufferAddLit(&buf, "vc"); break; case VIR_DOMAIN_CHR_TYPE_PTY: - virBufferAddLit(buf, "pty"); + virBufferAddLit(&buf, "pty"); break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferStrcat(buf, dev->data.file.path, NULL); + virBufferStrcat(&buf, dev->data.file.path, NULL); break; case VIR_DOMAIN_CHR_TYPE_FILE: - virBufferVSprintf(buf, "file:%s", dev->data.file.path); + virBufferVSprintf(&buf, "file:%s", dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferVSprintf(buf, "pipe:%s", dev->data.file.path); + virBufferVSprintf(&buf, "pipe:%s", dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_STDIO: - virBufferAddLit(buf, "stdio"); + virBufferAddLit(&buf, "stdio"); break; case VIR_DOMAIN_CHR_TYPE_UDP: - virBufferVSprintf(buf, "udp:%s:%s@%s:%s", + virBufferVSprintf(&buf, "udp:%s:%s@%s:%s", dev->data.udp.connectHost, dev->data.udp.connectService, dev->data.udp.bindHost, @@ -2602,12 +2633,12 @@ static void qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, case VIR_DOMAIN_CHR_TYPE_TCP: if (dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET) { - virBufferVSprintf(buf, "telnet:%s:%s%s", + virBufferVSprintf(&buf, "telnet:%s:%s%s", dev->data.tcp.host, dev->data.tcp.service, dev->data.tcp.listen ? ",server,nowait" : ""); } else { - virBufferVSprintf(buf, "tcp:%s:%s%s", + virBufferVSprintf(&buf, "tcp:%s:%s%s", dev->data.tcp.host, dev->data.tcp.service, dev->data.tcp.listen ? ",server,nowait" : ""); @@ -2615,21 +2646,32 @@ static void qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferVSprintf(buf, "unix:%s%s", + virBufferVSprintf(&buf, "unix:%s%s", dev->data.nix.path, dev->data.nix.listen ? ",server,nowait" : ""); break; } + + if (virBufferError(&buf)) { + virReportOOMError(NULL); + goto error; + } + + return virBufferContentAndReset(&buf); + +error: + virBufferFreeAndReset(&buf); + return NULL; } static int -qemudBuildCommandLineCPU(virConnectPtr conn, - const struct qemud_driver *driver, - const virDomainDefPtr def, - const char *emulator, - const struct utsname *ut, - char **opt) +qemuBuildCpuArgStr(virConnectPtr conn, + const struct qemud_driver *driver, + const virDomainDefPtr def, + const char *emulator, + const struct utsname *ut, + char **opt) { const virCPUDefPtr host = driver->caps->host.cpu; virCPUDefPtr guest = NULL; @@ -2715,9 +2757,9 @@ no_memory: } static char * -qemudBuildCommandLineSmp(virConnectPtr conn, - const virDomainDefPtr def, - int qemuCmdFlags) +qemuBuildSmpArgStr(virConnectPtr conn, + const virDomainDefPtr def, + int qemuCmdFlags) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2947,7 +2989,7 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT(def->os.machine); } - if (qemudBuildCommandLineCPU(conn, driver, def, emulator, &ut, &cpu) < 0) + if (qemuBuildCpuArgStr(conn, driver, def, emulator, &ut, &cpu) < 0) goto error; if (cpu) { @@ -2987,7 +3029,7 @@ int qemudBuildCommandLine(virConnectPtr conn, } ADD_ARG_LIT("-smp"); - if (!(smp = qemudBuildCommandLineSmp(conn, def, qemuCmdFlags))) + if (!(smp = qemuBuildSmpArgStr(conn, def, qemuCmdFlags))) goto error; ADD_ARG(smp); @@ -3031,25 +3073,14 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-nodefaults"); if (monitor_chr) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - + char *chrdev; /* Use -chardev if it's available */ if (qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) { - qemudBuildCommandLineChrDevChardevStr(monitor_chr, &buf); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } ADD_ARG_LIT("-chardev"); - ADD_ARG(virBufferContentAndReset(&buf)); - - virBufferVSprintf(&buf, "chardev=%s", monitor_chr->info.alias); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } + if (!(chrdev = qemuBuildChrChardevStr(monitor_chr))) + goto error; + ADD_ARG(chrdev); ADD_ARG_LIT("-mon"); if (monitor_json) @@ -3057,18 +3088,14 @@ int qemudBuildCommandLine(virConnectPtr conn, else ADD_ARG_LIT("chardev=monitor,mode=readline"); } else { + const char *prefix = NULL; if (monitor_json) - virBufferAddLit(&buf, "control,"); - - qemudBuildCommandLineChrDevStr(monitor_chr, &buf); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } + prefix = "control,"; ADD_ARG_LIT("-monitor"); - ADD_ARG(virBufferContentAndReset(&buf)); + if (!(chrdev = qemuBuildChrArgStr(monitor_chr, prefix))) + goto error; + ADD_ARG(chrdev); } } @@ -3250,7 +3277,7 @@ int qemudBuildCommandLine(virConnectPtr conn, } else { ADD_ARG_LIT("-device"); - if (qemuBuildDriveDevStr(conn, disk, &optstr) < 0) + if (!(optstr = qemuBuildDriveDevStr(conn, disk))) goto error; ADD_ARG(optstr); } @@ -3357,7 +3384,7 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ADD_ARG_LIT("-netdev"); - if (qemuBuildNetDevStr(conn, net, tapfd_name, &host) < 0) + if (!(host = qemuBuildNetDevStr(conn, net, tapfd_name))) goto error; ADD_ARG(host); @@ -3367,13 +3394,13 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG(nic); } else { ADD_ARG_LIT("-net"); - if (qemuBuildNicStr(conn, net, "nic,", net->vlan, &nic) < 0) + if (!(nic = qemuBuildNicStr(conn, net, "nic,", net->vlan))) goto error; ADD_ARG(nic); ADD_ARG_LIT("-net"); - if (qemuBuildHostNetStr(conn, net, ',', - net->vlan, tapfd_name, &host) < 0) + if (!(host = qemuBuildHostNetStr(conn, net, ',', + net->vlan, tapfd_name))) goto error; ADD_ARG(host); } @@ -3388,40 +3415,26 @@ int qemudBuildCommandLine(virConnectPtr conn, } } else { for (i = 0 ; i < def->nserials ; i++) { - virBuffer buf = VIR_BUFFER_INITIALIZER; virDomainChrDefPtr serial = def->serials[i]; + char *devstr; /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - qemudBuildCommandLineChrDevChardevStr(serial, &buf); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } - ADD_ARG_LIT("-chardev"); - ADD_ARG(virBufferContentAndReset(&buf)); - - virBufferVSprintf(&buf, "isa-serial,chardev=%s", serial->info.alias); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } + if (!(devstr = qemuBuildChrChardevStr(serial))) + goto error; + ADD_ARG(devstr); ADD_ARG_LIT("-device"); - ADD_ARG(virBufferContentAndReset(&buf)); - } - - else { - qemudBuildCommandLineChrDevStr(serial, &buf); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); + if (virAsprintf(&devstr, "isa-serial,chardev=%s", serial->info.alias) < 0) goto no_memory; - } - + ADD_ARG(devstr); + } else { ADD_ARG_LIT("-serial"); - ADD_ARG(virBufferContentAndReset(&buf)); + if (!(devstr = qemuBuildChrArgStr(serial, NULL))) + goto error; + ADD_ARG(devstr); } } } @@ -3434,47 +3447,33 @@ int qemudBuildCommandLine(virConnectPtr conn, } } else { for (i = 0 ; i < def->nparallels ; i++) { - virBuffer buf = VIR_BUFFER_INITIALIZER; virDomainChrDefPtr parallel = def->parallels[i]; + char *devstr; /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - qemudBuildCommandLineChrDevChardevStr(parallel, &buf); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } - ADD_ARG_LIT("-chardev"); - ADD_ARG(virBufferContentAndReset(&buf)); - - virBufferVSprintf(&buf, "isa-parallel,chardev=%s", parallel->info.alias); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } + if (!(devstr = qemuBuildChrChardevStr(parallel))) + goto error; + ADD_ARG(devstr); ADD_ARG_LIT("-device"); - ADD_ARG(virBufferContentAndReset(&buf)); - } - - else { - qemudBuildCommandLineChrDevStr(parallel, &buf); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); + if (virAsprintf(&devstr, "isa-parallel,chardev=%s", parallel->info.alias) < 0) goto no_memory; - } - + ADD_ARG(devstr); + } else { ADD_ARG_LIT("-parallel"); - ADD_ARG(virBufferContentAndReset(&buf)); + if (!(devstr = qemuBuildChrArgStr(parallel, NULL))) + goto error; + ADD_ARG(devstr); } } } for (i = 0 ; i < def->nchannels ; i++) { - virBuffer buf = VIR_BUFFER_INITIALIZER; virDomainChrDefPtr channel = def->channels[i]; + char *devstr; switch(channel->targetType) { case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: @@ -3485,30 +3484,22 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - qemudBuildCommandLineChrDevChardevStr(channel, &buf); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - goto no_memory; - } - ADD_ARG_LIT("-chardev"); - ADD_ARG(virBufferContentAndReset(&buf)); + if (!(devstr = qemuBuildChrChardevStr(channel))) + goto error; + ADD_ARG(devstr); - const char *addr = virSocketFormatAddr(channel->target.addr); + char *addr = virSocketFormatAddr(channel->target.addr); int port = virSocketGetPort(channel->target.addr); ADD_ARG_LIT("-netdev"); - virBufferVSprintf(&buf, "user,guestfwd=tcp:%s:%i,chardev=%s,id=user-%s", - addr, port, channel->info.alias, channel->info.alias); - - VIR_FREE(addr); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); + if (virAsprintf(&devstr, "user,guestfwd=tcp:%s:%i,chardev=%s,id=user-%s", + addr, port, channel->info.alias, channel->info.alias) < 0) { + VIR_FREE(addr); goto no_memory; } - - ADD_ARG(virBufferContentAndReset(&buf)); + VIR_FREE(addr); + ADD_ARG(devstr); } } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 2530813..b6f128f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -188,23 +188,51 @@ int qemudBuildCommandLine (virConnectPtr conn, int *ntapfds, const char *migrateFrom); -int qemuBuildHostNetStr (virConnectPtr conn, - virDomainNetDefPtr net, - char type_sep, - int vlan, - const char *tapfd, - char **str); +/* Legacy, pre device support */ +char * qemuBuildHostNetStr(virConnectPtr conn, + virDomainNetDefPtr net, + char type_sep, + int vlan, + const char *tapfd); -int qemuBuildNicStr (virConnectPtr conn, - virDomainNetDefPtr net, - const char *prefix, - int vlan, - char **str); +/* Current, best practice */ +char * qemuBuildNetDevStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *tapfd); + + +/* Legacy, pre device support */ +char * qemuBuildNicStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *prefix, + int vlan); + +/* Current, best practice */ +char * qemuBuildNicDevStr(virDomainNetDefPtr net); + +/* Both legacy & current support support */ +char *qemuBuildDriveStr(virDomainDiskDefPtr disk, + int bootable, + int qemuCmdFlags); + +/* Current, best practice */ +char * qemuBuildDriveDevStr(virConnectPtr conn, + virDomainDiskDefPtr disk); +/* Current, best practice */ +char * qemuBuildControllerDevStr(virDomainControllerDefPtr def); + +char * qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev); + +char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev); + +char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound); + +char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev); + +char * qemuBuildChrChardevStr(virDomainChrDefPtr dev); +char * qemuBuildChrArgStr(virDomainChrDefPtr dev, const char *prefix); -char * qemuBuildDriveStr (virDomainDiskDefPtr disk, - int bootable, - int qemuCmdFlags); int qemudNetworkIfaceConnect (virConnectPtr conn, struct qemud_driver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 22593bf..b02c275 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5522,8 +5522,8 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjExitMonitorWithDriver(driver, vm); } - if (qemuBuildHostNetStr(conn, net, ' ', - net->vlan, tapfd_name, &netstr) < 0) + if (!(netstr = qemuBuildHostNetStr(conn, net, ' ', + net->vlan, tapfd_name))) goto try_tapfd_close; qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -5537,7 +5537,7 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, close(tapfd); tapfd = -1; - if (qemuBuildNicStr(conn, net, NULL, net->vlan, &nicstr) < 0) + if (!(nicstr = qemuBuildNicStr(conn, net, NULL, net->vlan))) goto try_remove; qemuDomainObjEnterMonitorWithDriver(driver, vm); -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
All the helper functions for building command line arguments now return a 'char *', instead of acepting a 'char **' or virBufferPtr argument
* qemu/qemu_conf.c: Standardize syntax for building args * qemu/qemu_conf.h: Export all functions for building args * qemu/qemu_driver.c: Update for changed syntax for building NIC/hostnet args ---
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 2530813..b6f128f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -188,23 +188,51 @@ int qemudBuildCommandLine (virConnectPtr conn, int *ntapfds, const char *migrateFrom);
-int qemuBuildHostNetStr (virConnectPtr conn, - virDomainNetDefPtr net, - char type_sep, - int vlan, - const char *tapfd, - char **str); +/* Legacy, pre device support */ +char * qemuBuildHostNetStr(virConnectPtr conn, + virDomainNetDefPtr net, + char type_sep, + int vlan, + const char *tapfd);
-int qemuBuildNicStr (virConnectPtr conn, - virDomainNetDefPtr net, - const char *prefix, - int vlan, - char **str); +/* Current, best practice */ +char * qemuBuildNetDevStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *tapfd); + + +/* Legacy, pre device support */ +char * qemuBuildNicStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *prefix, + int vlan); + +/* Current, best practice */ +char * qemuBuildNicDevStr(virDomainNetDefPtr net); + +/* Both legacy & current support support */
support support?
+char *qemuBuildDriveStr(virDomainDiskDefPtr disk, + int bootable, + int qemuCmdFlags); +
ACK. Matthias

On Mon, Feb 01, 2010 at 06:39:30PM +0000, Daniel P. Berrange wrote:
All the helper functions for building command line arguments now return a 'char *', instead of acepting a 'char **' or virBufferPtr argument
* qemu/qemu_conf.c: Standardize syntax for building args * qemu/qemu_conf.h: Export all functions for building args * qemu/qemu_driver.c: Update for changed syntax for building NIC/hostnet args --- src/qemu/qemu_conf.c | 341 +++++++++++++++++++++++------------------------- src/qemu/qemu_conf.h | 56 ++++++-- src/qemu/qemu_driver.c | 6 +- 3 files changed, 211 insertions(+), 192 deletions(-)
ACK, 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/

To allow for better code reuse from hotplug methods, the code for generating PCI/USB hostdev arg values is split out into separate methods * qemu/qemu_conf.h, qemu/qemu_conf.c: Introduce new APis for qemuBuildPCIHostdevPCIDevStr, qemuBuildUSBHostdevUsbDevStr and qemuBuildUSBHostdevDevStr --- src/qemu/qemu_conf.c | 105 +++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_conf.h | 10 +++++ 2 files changed, 81 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 232dc5d..bbf49fd 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2506,6 +2506,65 @@ error: } +char * +qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev) +{ + char *ret = NULL; + + if (virAsprintf(&ret, "host=%.2x:%.2x.%.1x", + dev->source.subsys.u.pci.bus, + dev->source.subsys.u.pci.slot, + dev->source.subsys.u.pci.function) < 0) + virReportOOMError(NULL); + + return ret; +} + + +char * +qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev) +{ + char *ret = NULL; + + if (!dev->source.subsys.u.usb.bus && + !dev->source.subsys.u.usb.device) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("USB host device is missing bus/device information")); + return NULL; + } + + if (virAsprintf(&ret, "usb-host,hostbus=%.3d,hostaddr=%.3d,id=%s", + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device, + dev->info.alias) < 0) + virReportOOMError(NULL); + + return ret; +} + + +char * +qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev) +{ + char *ret = NULL; + + if (!dev->source.subsys.u.usb.bus && + !dev->source.subsys.u.usb.device) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("USB host device is missing bus/device information")); + return NULL; + } + + if (virAsprintf(&ret, "host:%.3d.%.3d", + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device) < 0) + virReportOOMError(NULL); + + return ret; +} + + + /* This function outputs a -chardev command line option which describes only the * host side of the character device */ char * @@ -3758,10 +3817,8 @@ int qemudBuildCommandLine(virConnectPtr conn, /* Add host passthrough hardware */ for (i = 0 ; i < def->nhostdevs ; i++) { - int ret; - char* usbdev; - char* pcidev; virDomainHostdevDefPtr hostdev = def->hostdevs[i]; + char *devstr; /* USB */ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && @@ -3769,33 +3826,15 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ADD_ARG_LIT("-device"); - if (hostdev->source.subsys.u.usb.vendor) { - ret = virAsprintf(&usbdev, "usb-host,vendor=%.4x,product=%.4x,id=%s", - hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product, - hostdev->info.alias); - } else { - ret = virAsprintf(&usbdev, "usb-host,hostbus=%.3d,hostaddr=%.3d,id=%s", - hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device, - hostdev->info.alias); - } + if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev))) + goto error; + ADD_ARG(devstr); } else { ADD_ARG_LIT("-usbdevice"); - if (hostdev->source.subsys.u.usb.vendor) { - ret = virAsprintf(&usbdev, "host:%.4x:%.4x", - hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); - } else { - ret = virAsprintf(&usbdev, "host:%.3d.%.3d", - hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device); - } + if (!(devstr = qemuBuildUSBHostdevUsbDevStr(hostdev))) + goto error; + ADD_ARG(devstr); } - if (ret < 0) - goto error; - - ADD_ARG(usbdev); } /* PCI */ @@ -3803,21 +3842,19 @@ int qemudBuildCommandLine(virConnectPtr conn, hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ADD_ARG_LIT("-device"); - if (!(pcidev = qemuBuildPCIHostdevDevStr(hostdev))) + if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev))) goto error; + ADD_ARG(devstr); } else if (qemuCmdFlags & QEMUD_CMD_FLAG_PCIDEVICE) { ADD_ARG_LIT("-pcidevice"); - if (virAsprintf(&pcidev, "host=%.2x:%.2x.%.1x", - hostdev->source.subsys.u.pci.bus, - hostdev->source.subsys.u.pci.slot, - hostdev->source.subsys.u.pci.function) < 0) - goto no_memory; + if (!(devstr = qemuBuildPCIHostdevPCIDevStr(hostdev))) + goto error; + ADD_ARG(devstr); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s", _("PCI device assignment is not supported by this version of qemu")); goto error; } - ADD_ARG(pcidev); } } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index b6f128f..c3b196e 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -227,11 +227,21 @@ char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev); char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound); +/* Legacy, pre device support */ +char * qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev); +/* Current, best practice */ char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev); +/* Current, best practice */ char * qemuBuildChrChardevStr(virDomainChrDefPtr dev); +/* Legacy, pre device support */ char * qemuBuildChrArgStr(virDomainChrDefPtr dev, const char *prefix); +/* Legacy, pre device support */ +char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev); +/* Current, best practice */ +char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev); + int qemudNetworkIfaceConnect (virConnectPtr conn, -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
To allow for better code reuse from hotplug methods, the code for generating PCI/USB hostdev arg values is split out into separate methods
* qemu/qemu_conf.h, qemu/qemu_conf.c: Introduce new APis for qemuBuildPCIHostdevPCIDevStr, qemuBuildUSBHostdevUsbDevStr and qemuBuildUSBHostdevDevStr ---
ACK. Matthias

On Mon, Feb 01, 2010 at 06:39:31PM +0000, Daniel P. Berrange wrote:
To allow for better code reuse from hotplug methods, the code for generating PCI/USB hostdev arg values is split out into separate methods
* qemu/qemu_conf.h, qemu/qemu_conf.c: Introduce new APis for qemuBuildPCIHostdevPCIDevStr, qemuBuildUSBHostdevUsbDevStr and qemuBuildUSBHostdevDevStr --- src/qemu/qemu_conf.c | 105 +++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_conf.h | 10 +++++ 2 files changed, 81 insertions(+), 34 deletions(-)
ACK, 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/

The way QEMU is started has been changed to use '-device' and the new style '-drive' syntax. This needs to be mirrored in the hotplug code, requiring addition of two new APIs. * src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs qemuMonitorAddDevice() and qemuMonitorAddDrive() * src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Implement the new monitor APIs --- src/qemu/qemu_monitor.c | 27 +++++++++++++++ src/qemu/qemu_monitor.h | 6 +++ src/qemu/qemu_monitor_json.c | 49 ++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ src/qemu/qemu_monitor_text.c | 77 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 6 +++ 6 files changed, 171 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 817ccd7..9e09876 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1304,3 +1304,30 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, ret = qemuMonitorTextGetAllPCIAddresses(mon, addrs); return ret; } + + +int qemuMonitorAddDevice(qemuMonitorPtr mon, + const char *devicestr) +{ + DEBUG("mon=%p, fd=%d device=%s", mon, mon->fd, devicestr); + int ret; + + if (mon->json) + ret = qemuMonitorJSONAddDevice(mon, devicestr); + else + ret = qemuMonitorTextAddDevice(mon, devicestr); + return ret; +} + +int qemuMonitorAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + DEBUG("mon=%p, fd=%d drive=%s", mon, mon->fd, drivestr); + int ret; + + if (mon->json) + ret = qemuMonitorJSONAddDrive(mon, drivestr); + else + ret = qemuMonitorTextAddDrive(mon, drivestr); + return ret; +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8a405ce..a330eff 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -285,4 +285,10 @@ struct _qemuMonitorPCIAddress { int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, qemuMonitorPCIAddress **addrs); +int qemuMonitorAddDevice(qemuMonitorPtr mon, + const char *devicestr); + +int qemuMonitorAddDrive(qemuMonitorPtr mon, + const char *drivestr); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8e88c7e..a556088 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1785,3 +1785,52 @@ int qemuMonitorJSONGetAllPCIAddresses(qemuMonitorPtr mon ATTRIBUTE_UNUSED, _("query-pci not suppported in JSON mode")); return -1; } + + +int qemuMonitorJSONAddDevice(qemuMonitorPtr mon, + const char *devicestr) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("device_add", + "s:config", devicestr, + NULL); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + +int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("drive_add", + "s:pci_addr", "dummy", + "s:opts", drivestr, + NULL); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 858aac0..ac6458c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -156,4 +156,10 @@ int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon, int qemuMonitorJSONGetAllPCIAddresses(qemuMonitorPtr mon, qemuMonitorPCIAddress **addrs); +int qemuMonitorJSONAddDevice(qemuMonitorPtr mon, + const char *devicestr); + +int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, + const char *drivestr); + #endif /* QEMU_MONITOR_JSON_H */ diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 380bcdc..b2a0c53 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2029,3 +2029,80 @@ error: #undef SKIP_SPACE #undef CHECK_END #undef SKIP_TO + + +int qemuMonitorTextAddDevice(qemuMonitorPtr mon, + const char *devicestr) +{ + char *cmd = NULL; + char *reply = NULL; + char *safedev; + int ret = -1; + + if (!(safedev = qemuMonitorEscapeArg(devicestr))) { + virReportOOMError(NULL); + goto cleanup; + } + + if (virAsprintf(&cmd, "device_add %s", safedev) < 0) { + virReportOOMError(NULL); + goto cleanup; + } + + if (qemuMonitorCommand(mon, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot attach %s device"), devicestr); + goto cleanup; + } + + if (STRNEQ(reply, "")) { + qemudReportError (NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s device failed: %s"), devicestr, reply); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(cmd); + VIR_FREE(reply); + return ret; +} + + +int qemuMonitorTextAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + char *cmd = NULL; + char *reply = NULL; + int ret = -1; + char *safe_str; + + safe_str = qemuMonitorEscapeArg(drivestr); + if (!safe_str) { + virReportOOMError(NULL); + return -1; + } + + ret = virAsprintf(&cmd, "drive_add dummy %s", safe_str); + if (ret == -1) { + virReportOOMError(NULL); + goto cleanup; + } + + if (qemuMonitorCommand(mon, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to close fd in qemu with '%s'"), cmd); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(cmd); + VIR_FREE(reply); + VIR_FREE(safe_str); + return ret; +} + + diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index d6e9ca1..12d75f5 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -160,4 +160,10 @@ int qemuMonitorTextAttachDrive(qemuMonitorPtr mon, int qemuMonitorTextGetAllPCIAddresses(qemuMonitorPtr mon, qemuMonitorPCIAddress **addrs); +int qemuMonitorTextAddDevice(qemuMonitorPtr mon, + const char *devicestr); + +int qemuMonitorTextAddDrive(qemuMonitorPtr mon, + const char *drivestr); + #endif /* QEMU_MONITOR_TEXT_H */ -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The way QEMU is started has been changed to use '-device' and the new style '-drive' syntax. This needs to be mirrored in the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs qemuMonitorAddDevice() and qemuMonitorAddDrive() * src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Implement the new monitor APIs ---
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 380bcdc..b2a0c53 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2029,3 +2029,80 @@ error:
+int qemuMonitorTextAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + char *cmd = NULL; + char *reply = NULL; + int ret = -1; + char *safe_str; + + safe_str = qemuMonitorEscapeArg(drivestr); + if (!safe_str) { + virReportOOMError(NULL); + return -1; + } + + ret = virAsprintf(&cmd, "drive_add dummy %s", safe_str);
dummy looks like a leftover from debugging to me, or does it belong there?
+ if (ret == -1) { + virReportOOMError(NULL); + goto cleanup; + } + + if (qemuMonitorCommand(mon, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to close fd in qemu with '%s'"), cmd); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(cmd); + VIR_FREE(reply); + VIR_FREE(safe_str); + return ret; +} +
ACK Matthias

On Tue, Feb 02, 2010 at 10:12:08AM +0100, Matthias Bolte wrote:
2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The way QEMU is started has been changed to use '-device' and the new style '-drive' syntax. This needs to be mirrored in the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs qemuMonitorAddDevice() and qemuMonitorAddDrive() * src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Implement the new monitor APIs ---
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 380bcdc..b2a0c53 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2029,3 +2029,80 @@ error:
+int qemuMonitorTextAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + char *cmd = NULL; + char *reply = NULL; + int ret = -1; + char *safe_str; + + safe_str = qemuMonitorEscapeArg(drivestr); + if (!safe_str) { + virReportOOMError(NULL); + return -1; + } + + ret = virAsprintf(&cmd, "drive_add dummy %s", safe_str);
dummy looks like a leftover from debugging to me, or does it belong there?
I'm adding a comment about this before committing 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, Feb 01, 2010 at 06:39:32PM +0000, Daniel P. Berrange wrote:
The way QEMU is started has been changed to use '-device' and the new style '-drive' syntax. This needs to be mirrored in the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs qemuMonitorAddDevice() and qemuMonitorAddDrive() * src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Implement the new monitor APIs --- [...] +int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("drive_add", + "s:pci_addr", "dummy", + "s:opts", drivestr, + NULL); [...] +int qemuMonitorTextAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + char *cmd = NULL; + char *reply = NULL; + int ret = -1; + char *safe_str; + + safe_str = qemuMonitorEscapeArg(drivestr); + if (!safe_str) { + virReportOOMError(NULL); + return -1; + } + + ret = virAsprintf(&cmd, "drive_add dummy %s", safe_str);
Like Matthias I'm wondering, it seems to be allowed for network and drive naming, but still a bit surprizing ACK, 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 Tue, Feb 02, 2010 at 03:15:18PM +0100, Daniel Veillard wrote:
On Mon, Feb 01, 2010 at 06:39:32PM +0000, Daniel P. Berrange wrote:
The way QEMU is started has been changed to use '-device' and the new style '-drive' syntax. This needs to be mirrored in the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs qemuMonitorAddDevice() and qemuMonitorAddDrive() * src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Implement the new monitor APIs --- [...] +int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("drive_add", + "s:pci_addr", "dummy", + "s:opts", drivestr, + NULL); [...] +int qemuMonitorTextAddDrive(qemuMonitorPtr mon, + const char *drivestr) +{ + char *cmd = NULL; + char *reply = NULL; + int ret = -1; + char *safe_str; + + safe_str = qemuMonitorEscapeArg(drivestr); + if (!safe_str) { + virReportOOMError(NULL); + return -1; + } + + ret = virAsprintf(&cmd, "drive_add dummy %s", safe_str);
Like Matthias I'm wondering, it seems to be allowed for network and drive naming, but still a bit surprizing
This is a really bizarre bit of QEMU :-) Normally you would put a PCI address in that part of the command. In this cae though, we're not adding a PCI device, but rather adding a disk on a drive controller, therefore there is no relevant PCI address & we put in the placeholder 'dummy'. 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 :|

Since QEMU startup uses the new -device argument, the hotplug code needs todo the same. This converts disk, network and host device hotplug to use the device_add command * src/qemu/qemu_driver.c: Use new device_add monitor APIs whereever possible --- src/qemu/qemu_driver.c | 222 ++++++++++++++++++++++++++++++++++++------------ 1 files changed, 169 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b02c275..25e60c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5192,12 +5192,14 @@ error: static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + int qemuCmdFlags) { int i, ret; const char* type = virDomainDiskBusTypeToString(disk->bus); qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainDevicePCIAddress guestAddr; + char *devstr = NULL; + char *drivestr = NULL; for (i = 0 ; i < vm->def->ndisks ; i++) { if (STREQ(vm->def->disks[i]->dst, disk->dst)) { @@ -5212,28 +5214,52 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1; + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) + goto error; + + if (!(devstr = qemuBuildDriveDevStr(NULL, disk))) + goto error; + } + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { virReportOOMError(conn); goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAddPCIDisk(priv->mon, - disk->src, - type, - &guestAddr); + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) + qemuMonitorAddDevice(priv->mon, devstr); + /* XXX remove the drive upon fail */ + } else { + virDomainDevicePCIAddress guestAddr; + ret = qemuMonitorAddPCIDisk(priv->mon, + disk->src, + type, + &guestAddr); + if (ret == 0) { + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); + } + } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto error; - disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); virDomainDiskInsertPreAlloced(vm->def, disk); + VIR_FREE(devstr); + VIR_FREE(drivestr); + return 0; error: + VIR_FREE(devstr); + VIR_FREE(drivestr); + if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) @@ -5246,10 +5272,13 @@ error: static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainControllerDefPtr controller) + virDomainControllerDefPtr controller, + int qemuCmdFlags) { - int i, ret; + int i; + int ret = -1; const char* type = virDomainControllerTypeToString(controller->type); + char *devstr = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; for (i = 0 ; i < vm->def->ncontrollers ; i++) { @@ -5262,15 +5291,24 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, } } + if (!(devstr = qemuBuildControllerDevStr(controller))) { + virReportOOMError(NULL); + goto cleanup; + } + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers+1) < 0) { - virReportOOMError(conn); - return -1; + virReportOOMError(NULL); + goto cleanup; } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAttachPCIDiskController(priv->mon, - type, - &controller->info.addr.pci); + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + } else { + ret = qemuMonitorAttachPCIDiskController(priv->mon, + type, + &controller->info.addr.pci); + } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret == 0) { @@ -5278,6 +5316,8 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, virDomainControllerInsertPreAlloced(vm->def, controller); } +cleanup: + VIR_FREE(devstr); return ret; } @@ -5286,7 +5326,8 @@ static virDomainControllerDefPtr qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - int controller) + int controller, + int qemuCmdFlags) { int i; virDomainControllerDefPtr cont; @@ -5311,7 +5352,7 @@ qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn, VIR_INFO0("No SCSI controller present, hotplugging one"); if (qemudDomainAttachPciControllerDevice(conn, driver, - vm, cont) < 0) { + vm, cont, qemuCmdFlags) < 0) { VIR_FREE(cont); return NULL; } @@ -5327,9 +5368,9 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, { int i; qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainDeviceDriveAddress driveAddr; virDomainControllerDefPtr cont; char *drivestr = NULL; + char *devstr = NULL; int ret = -1; for (i = 0 ; i < vm->def->ndisks ; i++) { @@ -5352,6 +5393,9 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) goto error; + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + !(devstr = qemuBuildDriveDevStr(NULL, disk))) + goto error; /* We should have an adddress now, so make sure */ if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { @@ -5362,7 +5406,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, } for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { - cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i); + cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i, qemuCmdFlags); if (!cont) goto error; } @@ -5379,26 +5423,42 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAttachDrive(priv->mon, - drivestr, - &cont->info.addr.pci, - &driveAddr); + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + ret = qemuMonitorAddDrive(priv->mon, + drivestr); + if (ret == 0) + ret = qemuMonitorAddDevice(priv->mon, + devstr); + /* XXX should call 'drive_del' on error but this does not exist yet */ + } else { + virDomainDeviceDriveAddress driveAddr; + ret = qemuMonitorAttachDrive(priv->mon, + drivestr, + &cont->info.addr.pci, + &driveAddr); + if (ret == 0) { + /* XXX we should probably validate that the addr matches + * our existing defined addr instead of overwriting */ + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + memcpy(&disk->info.addr.drive, &driveAddr, sizeof(driveAddr)); + } + } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto error; - /* XXX we should probably validate that the addr matches - * our existing defined addr instead of overwriting */ - disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - memcpy(&disk->info.addr.drive, &driveAddr, sizeof(driveAddr)); virDomainDiskInsertPreAlloced(vm->def, disk); + + VIR_FREE(devstr); VIR_FREE(drivestr); return 0; error: + VIR_FREE(devstr); VIR_FREE(drivestr); + if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) @@ -5411,10 +5471,13 @@ error: static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + int qemuCmdFlags) { qemuDomainObjPrivatePtr priv = vm->privateData; int i, ret; + char *drivestr = NULL; + char *devstr = NULL; for (i = 0 ; i < vm->def->ndisks ; i++) { if (STREQ(vm->def->disks[i]->dst, disk->dst)) { @@ -5435,13 +5498,29 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, goto error; } + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) + goto error; + if (!(devstr = qemuBuildDriveDevStr(NULL, disk))) + goto error; + } + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { virReportOOMError(conn); goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAddUSBDisk(priv->mon, disk->src); + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + ret = qemuMonitorAddDrive(priv->mon, + drivestr); + if (ret == 0) + ret = qemuMonitorAddDevice(priv->mon, + devstr); + /* XXX should call 'drive_del' on error but this does not exist yet */ + } else { + ret = qemuMonitorAddUSBDisk(priv->mon, disk->src); + } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) @@ -5449,9 +5528,15 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, virDomainDiskInsertPreAlloced(vm->def, disk); + VIR_FREE(devstr); + VIR_FREE(drivestr); + return 0; error: + VIR_FREE(devstr); + VIR_FREE(drivestr); + if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) @@ -5594,12 +5679,14 @@ no_memory: static int qemudDomainAttachHostPciDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainHostdevDefPtr hostdev) + virDomainHostdevDefPtr hostdev, + int qemuCmdFlags) { qemuDomainObjPrivatePtr priv = vm->privateData; pciDevice *pci; int ret; virDomainDevicePCIAddress guestAddr; + char *devstr = NULL; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) { virReportOOMError(conn); @@ -5626,10 +5713,17 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return -1; } + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + !(devstr = qemuBuildPCIHostdevDevStr(hostdev))) + goto error; + qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAddPCIHostDevice(priv->mon, - &hostdev->source.subsys.u.pci, - &guestAddr); + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) + ret = qemuMonitorAddDevice(priv->mon, devstr); + else + ret = qemuMonitorAddPCIHostDevice(priv->mon, + &hostdev->source.subsys.u.pci, + &guestAddr); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto error; @@ -5638,9 +5732,12 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + VIR_FREE(devstr); + return 0; error: + VIR_FREE(devstr); pciDeviceListDel(conn, driver->activePciHostdevs, pci); return -1; @@ -5650,39 +5747,50 @@ error: static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainHostdevDefPtr hostdev) + virDomainHostdevDefPtr hostdev, + int qemuCmdFlags) { int ret; qemuDomainObjPrivatePtr priv = vm->privateData; + char *devstr = NULL; + + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + !(devstr = qemuBuildPCIHostdevDevStr(hostdev))) + goto error; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) { virReportOOMError(conn); - return -1; + goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (hostdev->source.subsys.u.usb.vendor) { - ret = qemuMonitorAddUSBDeviceMatch(priv->mon, - hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); - } else { + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) + ret = qemuMonitorAddDevice(priv->mon, devstr); + else ret = qemuMonitorAddUSBDeviceExact(priv->mon, hostdev->source.subsys.u.usb.bus, hostdev->source.subsys.u.usb.device); - } qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto error; - if (ret == 0) - vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; - return ret; + VIR_FREE(devstr); + + return 0; + +error: + VIR_FREE(devstr); + return -1; } static int qemudDomainAttachHostDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainHostdevDefPtr hostdev) + virDomainHostdevDefPtr hostdev, + int qemuCmdFlags) { if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, @@ -5698,12 +5806,14 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - if (qemudDomainAttachHostPciDevice(conn, driver, vm, hostdev) < 0) + if (qemudDomainAttachHostPciDevice(conn, driver, vm, + hostdev, qemuCmdFlags) < 0) goto error; break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - if (qemudDomainAttachHostUsbDevice(conn, driver, vm, hostdev) < 0) + if (qemudDomainAttachHostUsbDevice(conn, driver, vm, + hostdev, qemuCmdFlags) < 0) goto error; break; @@ -5794,15 +5904,18 @@ static int qemudDomainAttachDevice(virDomainPtr dom, case VIR_DOMAIN_DISK_DEVICE_DISK: if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { - ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev->data.disk); + ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, + dev->data.disk, qemuCmdFlags); if (ret == 0) dev->data.disk = NULL; } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { - ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, dev->data.disk); + ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, + dev->data.disk, qemuCmdFlags); if (ret == 0) dev->data.disk = NULL; } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { - ret = qemudDomainAttachSCSIDisk(dom->conn, driver, vm, dev->data.disk, qemuCmdFlags); + ret = qemudDomainAttachSCSIDisk(dom->conn, driver, vm, + dev->data.disk, qemuCmdFlags); if (ret == 0) dev->data.disk = NULL; } else { @@ -5825,7 +5938,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom, } } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { - ret = qemudDomainAttachPciControllerDevice(dom->conn, driver, vm, dev->data.controller); + ret = qemudDomainAttachPciControllerDevice(dom->conn, driver, vm, + dev->data.controller, qemuCmdFlags); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("disk controller bus '%s' cannot be hotplugged."), @@ -5833,11 +5947,13 @@ static int qemudDomainAttachDevice(virDomainPtr dom, /* fallthrough */ } } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { - ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev->data.net, qemuCmdFlags); + ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, + dev->data.net, qemuCmdFlags); if (ret == 0) dev->data.net = NULL; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { - ret = qemudDomainAttachHostDevice(dom->conn, driver, vm, dev->data.hostdev); + ret = qemudDomainAttachHostDevice(dom->conn, driver, vm, + dev->data.hostdev, qemuCmdFlags); if (ret == 0) dev->data.hostdev = NULL; } else { -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
Since QEMU startup uses the new -device argument, the hotplug code needs todo the same. This converts disk, network and host device hotplug to use the device_add command
* src/qemu/qemu_driver.c: Use new device_add monitor APIs whereever possible ---
ACK. Matthias

On Mon, Feb 01, 2010 at 06:39:33PM +0000, Daniel P. Berrange wrote:
Since QEMU startup uses the new -device argument, the hotplug code needs todo the same. This converts disk, network and host device hotplug to use the device_add command
* src/qemu/qemu_driver.c: Use new device_add monitor APIs whereever possible --- [...] + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) + qemuMonitorAddDevice(priv->mon, devstr); + /* XXX remove the drive upon fail */
Hum, maybe this small TODO should be added, hopefully it's in a later patch [...]
+ if (ret == 0) + ret = qemuMonitorAddDevice(priv->mon, + devstr); + /* XXX should call 'drive_del' on error but this does not exist yet */
ah, that's why ... ACK, 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 Tue, Feb 02, 2010 at 03:20:43PM +0100, Daniel Veillard wrote:
On Mon, Feb 01, 2010 at 06:39:33PM +0000, Daniel P. Berrange wrote:
Since QEMU startup uses the new -device argument, the hotplug code needs todo the same. This converts disk, network and host device hotplug to use the device_add command
* src/qemu/qemu_driver.c: Use new device_add monitor APIs whereever possible --- [...] + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) + qemuMonitorAddDevice(priv->mon, devstr); + /* XXX remove the drive upon fail */
Hum, maybe this small TODO should be added, hopefully it's in a later patch
[...]
+ if (ret == 0) + ret = qemuMonitorAddDevice(priv->mon, + devstr); + /* XXX should call 'drive_del' on error but this does not exist yet */
ah, that's why ...
ACK,
I've got a bug open against QEMU to get a 'drive_del' command added. 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 :|

The virDomainDeviceInfoIterate() function will provide a convenient way to iterate over all devices in a domain. * src/conf/domain_conf.c, src/conf/domain_conf.h, src/libvirt_private.syms: Add virDomainDeviceInfoIterate() function. --- src/conf/domain_conf.c | 67 +++++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 1 + 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e548d1d..691fc84 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -824,59 +824,82 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) } -static void virDomainDeviceInfoClearField(virDomainDeviceInfoPtr info, int alias, int pciaddr) +static int virDomainDeviceInfoClearAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque ATTRIBUTE_UNUSED) { - if (alias) - VIR_FREE(info->alias); - if (pciaddr && - info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + VIR_FREE(info->alias); + return 0; +} + +static int virDomainDeviceInfoClearPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque ATTRIBUTE_UNUSED) +{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { memset(&info->addr, 0, sizeof(info->addr)); info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; } + return 0; } - -static void virDomainDefClearDeviceInfo(virDomainDefPtr def, int alias, int pciaddr) +int virDomainDeviceInfoIterate(virDomainDefPtr def, + virDomainDeviceInfoCallback cb, + void *opaque) { int i; for (i = 0; i < def->ndisks ; i++) - virDomainDeviceInfoClearField(&def->disks[i]->info, alias, pciaddr); + if (cb(def, &def->disks[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nnets ; i++) - virDomainDeviceInfoClearField(&def->nets[i]->info, alias, pciaddr); + if (cb(def, &def->nets[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nsounds ; i++) - virDomainDeviceInfoClearField(&def->sounds[i]->info, alias, pciaddr); + if (cb(def, &def->sounds[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nhostdevs ; i++) - virDomainDeviceInfoClearField(&def->hostdevs[i]->info, alias, pciaddr); + if (cb(def, &def->hostdevs[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nvideos ; i++) - virDomainDeviceInfoClearField(&def->videos[i]->info, alias, pciaddr); + if (cb(def, &def->videos[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->ncontrollers ; i++) - virDomainDeviceInfoClearField(&def->controllers[i]->info, alias, pciaddr); + if (cb(def, &def->controllers[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nserials ; i++) - virDomainDeviceInfoClearField(&def->serials[i]->info, alias, pciaddr); + if (cb(def, &def->serials[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nparallels ; i++) - virDomainDeviceInfoClearField(&def->parallels[i]->info, alias, pciaddr); + if (cb(def, &def->parallels[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nchannels ; i++) - virDomainDeviceInfoClearField(&def->channels[i]->info, alias, pciaddr); + if (cb(def, &def->channels[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->ninputs ; i++) - virDomainDeviceInfoClearField(&def->inputs[i]->info, alias, pciaddr); + if (cb(def, &def->inputs[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nfss ; i++) - virDomainDeviceInfoClearField(&def->fss[i]->info, alias, pciaddr); + if (cb(def, &def->fss[i]->info, opaque) < 0) + return -1; if (def->watchdog) - virDomainDeviceInfoClearField(&def->watchdog->info, alias, pciaddr); + if (cb(def, &def->watchdog->info, opaque) < 0) + return -1; if (def->console) - virDomainDeviceInfoClearField(&def->console->info, alias, pciaddr); + if (cb(def, &def->console->info, opaque) < 0) + return -1; + return 0; } void virDomainDefClearPCIAddresses(virDomainDefPtr def) { - virDomainDefClearDeviceInfo(def, 0, 1); + virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); } void virDomainDefClearDeviceAliases(virDomainDefPtr def) { - virDomainDefClearDeviceInfo(def, 1, 0); + virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7be090d..be0dc92 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -742,6 +742,14 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); +typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, + virDomainDeviceInfoPtr dev, + void *opaque); + +int virDomainDeviceInfoIterate(virDomainDefPtr def, + virDomainDeviceInfoCallback cb, + void *opaque); + void virDomainDefFree(virDomainDefPtr vm); void virDomainObjRef(virDomainObjPtr vm); /* Returns 1 if the object was freed, 0 if more refs exist */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d56fb7d..e5e8860 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -187,6 +187,7 @@ virDomainDeviceAddressTypeToString; virDomainDefAddDiskControllers; virDomainDefClearPCIAddresses; virDomainDefClearDeviceAliases; +virDomainDeviceInfoIterate; # domain_event.h -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The virDomainDeviceInfoIterate() function will provide a convenient way to iterate over all devices in a domain.
* src/conf/domain_conf.c, src/conf/domain_conf.h, src/libvirt_private.syms: Add virDomainDeviceInfoIterate() function. ---
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:34PM +0000, Daniel P. Berrange wrote:
The virDomainDeviceInfoIterate() function will provide a convenient way to iterate over all devices in a domain.
* src/conf/domain_conf.c, src/conf/domain_conf.h, src/libvirt_private.syms: Add virDomainDeviceInfoIterate() function. --- src/conf/domain_conf.c | 67 +++++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 1 + 3 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e548d1d..691fc84 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -824,59 +824,82 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) }
-static void virDomainDeviceInfoClearField(virDomainDeviceInfoPtr info, int alias, int pciaddr) +static int virDomainDeviceInfoClearAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque ATTRIBUTE_UNUSED) { - if (alias) - VIR_FREE(info->alias); - if (pciaddr && - info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + VIR_FREE(info->alias); + return 0; +} + +static int virDomainDeviceInfoClearPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque ATTRIBUTE_UNUSED) +{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { memset(&info->addr, 0, sizeof(info->addr)); info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; } + return 0; }
- -static void virDomainDefClearDeviceInfo(virDomainDefPtr def, int alias, int pciaddr) +int virDomainDeviceInfoIterate(virDomainDefPtr def, + virDomainDeviceInfoCallback cb, + void *opaque) { int i;
for (i = 0; i < def->ndisks ; i++) - virDomainDeviceInfoClearField(&def->disks[i]->info, alias, pciaddr); + if (cb(def, &def->disks[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nnets ; i++) - virDomainDeviceInfoClearField(&def->nets[i]->info, alias, pciaddr); + if (cb(def, &def->nets[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nsounds ; i++) - virDomainDeviceInfoClearField(&def->sounds[i]->info, alias, pciaddr); + if (cb(def, &def->sounds[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nhostdevs ; i++) - virDomainDeviceInfoClearField(&def->hostdevs[i]->info, alias, pciaddr); + if (cb(def, &def->hostdevs[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nvideos ; i++) - virDomainDeviceInfoClearField(&def->videos[i]->info, alias, pciaddr); + if (cb(def, &def->videos[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->ncontrollers ; i++) - virDomainDeviceInfoClearField(&def->controllers[i]->info, alias, pciaddr); + if (cb(def, &def->controllers[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nserials ; i++) - virDomainDeviceInfoClearField(&def->serials[i]->info, alias, pciaddr); + if (cb(def, &def->serials[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nparallels ; i++) - virDomainDeviceInfoClearField(&def->parallels[i]->info, alias, pciaddr); + if (cb(def, &def->parallels[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nchannels ; i++) - virDomainDeviceInfoClearField(&def->channels[i]->info, alias, pciaddr); + if (cb(def, &def->channels[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->ninputs ; i++) - virDomainDeviceInfoClearField(&def->inputs[i]->info, alias, pciaddr); + if (cb(def, &def->inputs[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->nfss ; i++) - virDomainDeviceInfoClearField(&def->fss[i]->info, alias, pciaddr); + if (cb(def, &def->fss[i]->info, opaque) < 0) + return -1; if (def->watchdog) - virDomainDeviceInfoClearField(&def->watchdog->info, alias, pciaddr); + if (cb(def, &def->watchdog->info, opaque) < 0) + return -1; if (def->console) - virDomainDeviceInfoClearField(&def->console->info, alias, pciaddr); + if (cb(def, &def->console->info, opaque) < 0) + return -1; + return 0; }
void virDomainDefClearPCIAddresses(virDomainDefPtr def) { - virDomainDefClearDeviceInfo(def, 0, 1); + virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); }
void virDomainDefClearDeviceAliases(virDomainDefPtr def) { - virDomainDefClearDeviceInfo(def, 1, 0); + virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7be090d..be0dc92 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -742,6 +742,14 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def);
+typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, + virDomainDeviceInfoPtr dev, + void *opaque); + +int virDomainDeviceInfoIterate(virDomainDefPtr def, + virDomainDeviceInfoCallback cb, + void *opaque); + void virDomainDefFree(virDomainDefPtr vm); void virDomainObjRef(virDomainObjPtr vm); /* Returns 1 if the object was freed, 0 if more refs exist */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d56fb7d..e5e8860 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -187,6 +187,7 @@ virDomainDeviceAddressTypeToString; virDomainDefAddDiskControllers; virDomainDefClearPCIAddresses; virDomainDefClearDeviceAliases; +virDomainDeviceInfoIterate;
# domain_event.h
Patch really tries hard to reconciliate things it should not, but looks fine ! ACK, 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/

The current QEMU code allocates PCI addresses incrementally starting at 4. This is not satisfactory because the user may have given some addresses in their XML config, which need to be skipped over when allocating addresses to remaining devices. It is thus neccessary to maintain a list of already allocated PCI addresses and then only allocate ones that remain unused. This is also required for domain device hotplug to work properly later. * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating list of existing PCI addresses, and allocating new addresses. Refactor address assignment to use this code * src/qemu/qemu_driver.c: Pull PCI address assignment up into the qemuStartVMDaemon() method, as a prelude to moving it into the 'define' method. Update list of allocated addresses when connecting to a running VM at daemon startup. * tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c, tests/qemuxml2xmltest.c: Remove USB product test since all passthrough is done based on address * tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil unused data files --- src/qemu/qemu_conf.c | 228 +++++++++++++++++--- src/qemu/qemu_conf.h | 12 + src/qemu/qemu_driver.c | 23 ++ tests/qemuargv2xmltest.c | 1 - .../qemuxml2argv-hostdev-usb-product.args | 1 - .../qemuxml2argv-hostdev-usb-product.xml | 36 --- tests/qemuxml2argvtest.c | 13 +- tests/qemuxml2xmltest.c | 1 - 8 files changed, 245 insertions(+), 70 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bbf49fd..9bfd181 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1588,28 +1588,174 @@ qemuAssignDeviceAliases(virDomainDefPtr def) } -static void -qemuAssignDevicePCISlot(virDomainDeviceInfoPtr info, - int slot) +#define QEMU_PCI_ADDRESS_LAST_SLOT 31 +struct _qemuDomainPCIAddressSet { + virHashTablePtr used; + int nextslot; + /* XXX add domain, bus later when QEMU allows > 1 */ +}; + + +static char *qemuPCIAddressAsString(virDomainDeviceInfoPtr dev) { - info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - info->addr.pci.domain = 0; - info->addr.pci.bus = 0; - info->addr.pci.slot = slot; - info->addr.pci.function = 0; + char *addr; + + if (virAsprintf(&addr, "%d:%d:%d", + dev->addr.pci.domain, + dev->addr.pci.bus, + dev->addr.pci.slot) < 0) { + virReportOOMError(NULL); + return NULL; + } + return addr; } -static void -qemuAssignDevicePCISlots(virDomainDefPtr def) + +static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr dev, + void *opaque) +{ + qemuDomainPCIAddressSetPtr addrs = opaque; + + if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + char *addr = qemuPCIAddressAsString(dev); + + if (virHashAddEntry(addrs->used, addr, addr) < 0) { + VIR_FREE(addr); + return -1; + } + } + + return 0; +} + + +qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def) +{ + qemuDomainPCIAddressSetPtr addrs; + + if (VIR_ALLOC(addrs) < 0) + goto no_memory; + + if (!(addrs->used = virHashCreate(10))) + goto no_memory; + + if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0) + goto error; + + return addrs; + +no_memory: + virReportOOMError(NULL); +error: + qemuDomainPCIAddressSetFree(addrs); + return NULL; +} + +int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs, + int slot) +{ + virDomainDeviceInfo dev; + char *addr; + + dev.addr.pci.domain = 0; + dev.addr.pci.bus = 0; + dev.addr.pci.slot = slot; + + addr = qemuPCIAddressAsString(&dev); + if (!addr) + return -1; + + if (virHashLookup(addrs->used, addr)) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("unable to reserve PCI address %s"), addr); + VIR_FREE(addr); + return -1; + } + + if (virHashAddEntry(addrs->used, addr, addr)) { + VIR_FREE(addr); + return -1; + } + + return 0; +} + + +static void qemuDomainPCIAddressSetFreeEntry(void *payload, const char *name ATTRIBUTE_UNUSED) +{ + VIR_FREE(payload); +} + + +void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) +{ + if (!addrs) + return; + + virHashFree(addrs->used, qemuDomainPCIAddressSetFreeEntry); +} + + +int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) { int i; - /* - * slot = 0 -> Host bridge - * slot = 1 -> PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) - * slot = 2 -> VGA - * slot = 3 -> VirtIO Balloon - */ - int nextslot = 4; + + for (i = addrs->nextslot ; i <= QEMU_PCI_ADDRESS_LAST_SLOT ; i++) { + virDomainDeviceInfo maybe; + char *addr; + + memset(&maybe, 0, sizeof(maybe)); + maybe.addr.pci.domain = 0; + maybe.addr.pci.bus = 0; + maybe.addr.pci.slot = i; + + addr = qemuPCIAddressAsString(&maybe); + + if (virHashLookup(addrs->used, addr)) { + VIR_FREE(addr); + continue; + } + + if (virHashAddEntry(addrs->used, addr, addr) < 0) { + VIR_FREE(addr); + return -1; + } + + dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + dev->addr.pci.domain = 0; + dev->addr.pci.bus = 0; + dev->addr.pci.slot = i; + + addrs->nextslot = i + 1; + + return 0; + } + + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("No more available PCI addresses")); + return -1; +} + + +int +qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) +{ + int i; + + /* Host bridge */ + if (qemuDomainPCIAddressReserve(addrs, 0) < 0) + goto error; + /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) */ + if (qemuDomainPCIAddressReserve(addrs, 1) < 0) + goto error; + /* VGA */ + if (qemuDomainPCIAddressReserve(addrs, 2) < 0) + goto error; + /* VirtIO Balloon */ + if (qemuDomainPCIAddressReserve(addrs, 3) < 0) + goto error; for (i = 0; i < def->ndisks ; i++) { if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) @@ -1619,13 +1765,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def) if (def->disks[i]->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) continue; - qemuAssignDevicePCISlot(&def->disks[i]->info, nextslot++); + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->disks[i]->info) < 0) + goto error; } for (i = 0; i < def->nnets ; i++) { if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; - qemuAssignDevicePCISlot(&def->nets[i]->info, nextslot++); + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->nets[i]->info) < 0) + goto error; } for (i = 0; i < def->nsounds ; i++) { @@ -1636,7 +1784,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def) def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) continue; - qemuAssignDevicePCISlot(&def->sounds[i]->info, nextslot++); + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->sounds[i]->info) < 0) + goto error; } for (i = 0; i < def->nhostdevs ; i++) { @@ -1646,14 +1795,23 @@ qemuAssignDevicePCISlots(virDomainDefPtr def) def->hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - qemuAssignDevicePCISlot(&def->hostdevs[i]->info, nextslot++); + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->hostdevs[i]->info) < 0) + goto error; } for (i = 0; i < def->nvideos ; i++) { + if (def->videos[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + continue; /* First VGA is hardcoded slot=2 */ - if (i == 0) - qemuAssignDevicePCISlot(&def->videos[i]->info, 2); - else - qemuAssignDevicePCISlot(&def->videos[i]->info, nextslot++); + if (i == 0) { + def->videos[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->videos[i]->info.addr.pci.domain = 0; + def->videos[i]->info.addr.pci.bus = 0; + def->videos[i]->info.addr.pci.slot = 2; + def->videos[i]->info.addr.pci.function = 0; + } else { + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->videos[i]->info) < 0) + goto error; + } } for (i = 0; i < def->ncontrollers ; i++) { if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) @@ -1665,10 +1823,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def) /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && def->controllers[i]->idx == 0) { - qemuAssignDevicePCISlot(&def->controllers[i]->info, 1); + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = 1; def->controllers[i]->info.addr.pci.function = 1; } else { - qemuAssignDevicePCISlot(&def->controllers[i]->info, nextslot++); + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info) < 0) + goto error; } } for (i = 0; i < def->ninputs ; i++) { @@ -1684,9 +1846,16 @@ qemuAssignDevicePCISlots(virDomainDefPtr def) /* Nada - none are PCI based (yet) */ /* XXX virtio-serial will need one */ } - if (def->watchdog) { - qemuAssignDevicePCISlot(&def->watchdog->info, nextslot++); + if (def->watchdog && + def->watchdog->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->watchdog->info) < 0) + goto error; } + + return 0; + +error: + return -1; } @@ -2885,7 +3054,6 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { qemuAssignDeviceAliases(def); - qemuAssignDevicePCISlots(def); } else { qemuAssignDiskAliases(def, qemuCmdFlags); } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c3b196e..bcddf3c 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -146,6 +146,8 @@ struct qemud_driver { pciDeviceList *activePciHostdevs; }; +typedef struct _qemuDomainPCIAddressSet qemuDomainPCIAddressSet; +typedef qemuDomainPCIAddressSet *qemuDomainPCIAddressSetPtr; /* Port numbers used for KVM migration. */ #define QEMUD_MIGRATION_FIRST_PORT 49152 @@ -272,4 +274,14 @@ virDomainDefPtr qemuParseCommandLineString(virConnectPtr conn, virCapsPtr caps, const char *args); +qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def); +int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs, + int slot); +int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev); +void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); +int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs); + + + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 25e60c2..099e678 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -93,6 +93,8 @@ struct _qemuDomainObjPrivate { int nvcpupids; int *vcpupids; + + qemuDomainPCIAddressSetPtr pciaddrs; }; static int qemudShutdown(void); @@ -146,6 +148,7 @@ static void qemuDomainObjPrivateFree(void *data) { qemuDomainObjPrivatePtr priv = data; + qemuDomainPCIAddressSetFree(priv->pciaddrs); virDomainChrDefFree(priv->monConfig); VIR_FREE(priv->vcpupids); @@ -843,11 +846,14 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq { virDomainObjPtr obj = payload; struct qemud_driver *driver = opaque; + qemuDomainObjPrivatePtr priv; virDomainObjLock(obj); VIR_DEBUG("Reconnect monitor to %p '%s'", obj, obj->def->name); + priv = obj->privateData; + /* XXX check PID liveliness & EXE path */ if (qemuConnectMonitor(obj) < 0) goto error; @@ -856,6 +862,9 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq goto error; } + if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(obj->def))) + goto error; + if (driver->securityDriver && driver->securityDriver->domainReserveSecurityLabel && driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0) @@ -2635,6 +2644,18 @@ static int qemudStartVMDaemon(virConnectPtr conn, goto cleanup; } + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (priv->pciaddrs) { + qemuDomainPCIAddressSetFree(priv->pciaddrs); + priv->pciaddrs = NULL; + } + if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(vm->def))) + goto cleanup; + + if (qemuAssignDevicePCISlots(vm->def, priv->pciaddrs) < 0) + goto cleanup; + } + vm->def->id = driver->nextvmid++; if (qemudBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, qemuCmdFlags, &argv, &progenv, @@ -2867,6 +2888,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, virDomainDefClearPCIAddresses(vm->def); virDomainDefClearDeviceAliases(vm->def); + qemuDomainPCIAddressSetFree(priv->pciaddrs); + priv->pciaddrs = NULL; qemuDomainReAttachHostDevices(conn, driver, vm->def); diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 8326c57..1f1914b 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -213,7 +213,6 @@ mymain(int argc, char **argv) DO_TEST("sound", 0); DO_TEST("watchdog", 0); - DO_TEST("hostdev-usb-product", 0); DO_TEST("hostdev-usb-address", 0); DO_TEST("hostdev-pci-address", QEMUD_CMD_FLAG_PCIDEVICE); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args deleted file mode 100644 index 6084745..0000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args +++ /dev/null @@ -1 +0,0 @@ -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 none -serial none -parallel none -usb -usbdevice host:0204:6025 -usbdevice host:1234:0000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml deleted file mode 100644 index 3dc8eeb..0000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml +++ /dev/null @@ -1,36 +0,0 @@ -<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'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <hostdev mode='subsystem' type='usb' managed='no'> - <source> - <vendor id='0x0204'/> - <product id='0x6025'/> - </source> - </hostdev> - <hostdev mode='subsystem' type='usb' managed='no'> - <source> - <vendor id='0x1234'/> - <product id='0x0000'/> - </source> - </hostdev> - </devices> -</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fc237c2..90c1cb0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -62,6 +62,18 @@ static int testCompareXMLToArgvFiles(const char *xml, if (qemudCanonicalizeMachine(&driver, vmdef) < 0) goto fail; + if (flags & QEMUD_CMD_FLAG_DEVICE) { + qemuDomainPCIAddressSetPtr pciaddrs; + if (!(pciaddrs = qemuDomainPCIAddressSetCreate(vmdef))) + goto fail; + + if (qemuAssignDevicePCISlots(vmdef, pciaddrs) < 0) + goto fail; + + qemuDomainPCIAddressSetFree(pciaddrs); + } + + if (qemudBuildCommandLine(NULL, &driver, vmdef, &monitor_chr, 0, flags, &argv, &qenv, @@ -303,7 +315,6 @@ mymain(int argc, char **argv) DO_TEST("sound", 0); DO_TEST("sound-device", QEMUD_CMD_FLAG_DEVICE); - DO_TEST("hostdev-usb-product", 0); DO_TEST("hostdev-usb-address", 0); DO_TEST("hostdev-usb-address-device", QEMUD_CMD_FLAG_DEVICE); DO_TEST("hostdev-pci-address", QEMUD_CMD_FLAG_PCIDEVICE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0302696..5b706bb 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -134,7 +134,6 @@ mymain(int argc, char **argv) DO_TEST("console-compat"); DO_TEST("channel-guestfwd"); - DO_TEST("hostdev-usb-product"); DO_TEST("hostdev-usb-address"); DO_TEST("hostdev-pci-address"); -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The current QEMU code allocates PCI addresses incrementally starting at 4. This is not satisfactory because the user may have given some addresses in their XML config, which need to be skipped over when allocating addresses to remaining devices.
It is thus neccessary to maintain a list of already allocated PCI addresses and then only allocate ones that remain unused. This is also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating list of existing PCI addresses, and allocating new addresses. Refactor address assignment to use this code * src/qemu/qemu_driver.c: Pull PCI address assignment up into the qemuStartVMDaemon() method, as a prelude to moving it into the 'define' method. Update list of allocated addresses when connecting to a running VM at daemon startup. * tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c, tests/qemuxml2xmltest.c: Remove USB product test since all passthrough is done based on address * tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil unused data files ---
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bbf49fd..9bfd181 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c
+ +void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) +{ + if (!addrs) + return; + + virHashFree(addrs->used, qemuDomainPCIAddressSetFreeEntry);
Maybe add addrs->used = NULL after virHashFree to prevent potential double frees or segfaults due to the dangling pointer. ACK Matthias

On Tue, Feb 02, 2010 at 10:15:24AM +0100, Matthias Bolte wrote:
2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The current QEMU code allocates PCI addresses incrementally starting at 4. This is not satisfactory because the user may have given some addresses in their XML config, which need to be skipped over when allocating addresses to remaining devices.
It is thus neccessary to maintain a list of already allocated PCI addresses and then only allocate ones that remain unused. This is also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating list of existing PCI addresses, and allocating new addresses. Refactor address assignment to use this code * src/qemu/qemu_driver.c: Pull PCI address assignment up into the qemuStartVMDaemon() method, as a prelude to moving it into the 'define' method. Update list of allocated addresses when connecting to a running VM at daemon startup. * tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c, tests/qemuxml2xmltest.c: Remove USB product test since all passthrough is done based on address * tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil unused data files ---
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bbf49fd..9bfd181 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c
+ +void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) +{ + if (!addrs) + return; + + virHashFree(addrs->used, qemuDomainPCIAddressSetFreeEntry);
Maybe add addrs->used = NULL after virHashFree to prevent potential double frees or segfaults due to the dangling pointer.
Yep, added that too 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, Feb 01, 2010 at 06:39:35PM +0000, Daniel P. Berrange wrote:
The current QEMU code allocates PCI addresses incrementally starting at 4. This is not satisfactory because the user may have given some addresses in their XML config, which need to be skipped over when allocating addresses to remaining devices.
Ah, right !
It is thus neccessary to maintain a list of already allocated PCI addresses and then only allocate ones that remain unused. This is also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating list of existing PCI addresses, and allocating new addresses. Refactor address assignment to use this code * src/qemu/qemu_driver.c: Pull PCI address assignment up into the qemuStartVMDaemon() method, as a prelude to moving it into the 'define' method. Update list of allocated addresses when connecting to a running VM at daemon startup. * tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c, tests/qemuxml2xmltest.c: Remove USB product test since all passthrough is done based on address * tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil unused data files [...]
+ addr = qemuPCIAddressAsString(&dev); + if (!addr) + return -1; + + if (virHashLookup(addrs->used, addr)) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("unable to reserve PCI address %s"), addr); + VIR_FREE(addr); + return -1; + } + + if (virHashAddEntry(addrs->used, addr, addr)) { + VIR_FREE(addr); + return -1; + }
is using a hash table really that simpler in that case ? I doubt we need an optimized lookup, and this mean always saving to a standardized string to then do string comparisons instead of directly checking domain/bus/slot values. But that's a minor point. ACK, 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/

PCI disk, disk controllers, net devices and host devices need to have PCI addresses assigned before they are hot-plugged * src/qemu/qemu_conf.c: Add APIs for ensuring a device has an address and releasing unused addresses * src/qemu/qemu_driver.c: Ensure all devices have addresses when hotplugging. --- src/qemu/qemu_conf.c | 60 ++++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_conf.h | 11 +++++++- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 9bfd181..a70848c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1652,17 +1652,12 @@ error: return NULL; } -int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs, - int slot) +int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) { - virDomainDeviceInfo dev; char *addr; - dev.addr.pci.domain = 0; - dev.addr.pci.bus = 0; - dev.addr.pci.slot = slot; - - addr = qemuPCIAddressAsString(&dev); + addr = qemuPCIAddressAsString(dev); if (!addr) return -1; @@ -1681,6 +1676,29 @@ int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs, return 0; } +int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, + int slot) +{ + virDomainDeviceInfo dev; + + dev.addr.pci.domain = 0; + dev.addr.pci.bus = 0; + dev.addr.pci.slot = slot; + + return qemuDomainPCIAddressReserveAddr(addrs, &dev); +} + + +int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{ + int ret = 0; + if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + ret = qemuDomainPCIAddressReserveAddr(addrs, dev); + else + ret = qemuDomainPCIAddressSetNextAddr(addrs, dev); + return ret; +} static void qemuDomainPCIAddressSetFreeEntry(void *payload, const char *name ATTRIBUTE_UNUSED) { @@ -1688,6 +1706,24 @@ static void qemuDomainPCIAddressSetFreeEntry(void *payload, const char *name ATT } +int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{ + char *addr; + int ret; + + addr = qemuPCIAddressAsString(dev); + if (!addr) + return -1; + + ret = virHashRemoveEntry(addrs->used, addr, qemuDomainPCIAddressSetFreeEntry); + + VIR_FREE(addr); + + return ret; +} + + void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) { if (!addrs) @@ -1745,16 +1781,16 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) int i; /* Host bridge */ - if (qemuDomainPCIAddressReserve(addrs, 0) < 0) + if (qemuDomainPCIAddressReserveSlot(addrs, 0) < 0) goto error; /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) */ - if (qemuDomainPCIAddressReserve(addrs, 1) < 0) + if (qemuDomainPCIAddressReserveSlot(addrs, 1) < 0) goto error; /* VGA */ - if (qemuDomainPCIAddressReserve(addrs, 2) < 0) + if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) goto error; /* VirtIO Balloon */ - if (qemuDomainPCIAddressReserve(addrs, 3) < 0) + if (qemuDomainPCIAddressReserveSlot(addrs, 3) < 0) goto error; for (i = 0; i < def->ndisks ; i++) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index bcddf3c..b94153f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -275,10 +275,17 @@ virDomainDefPtr qemuParseCommandLineString(virConnectPtr conn, const char *args); qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def); -int qemuDomainPCIAddressReserve(qemuDomainPCIAddressSetPtr addrs, - int slot); +int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, + int slot); +int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev); int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev); +int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev); +int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev); + void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 099e678..df7f8e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5238,6 +5238,9 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, return -1; if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &disk->info) < 0) + goto error; + if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) goto error; @@ -5283,6 +5286,11 @@ error: VIR_FREE(devstr); VIR_FREE(drivestr); + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &disk->info) < 0) + VIR_WARN("Unable to release PCI address on %s", disk->src); + if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) @@ -5314,6 +5322,10 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, } } + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) + goto cleanup; + if (!(devstr = qemuBuildControllerDevStr(controller))) { virReportOOMError(NULL); goto cleanup; @@ -5340,6 +5352,12 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, } cleanup: + if ((ret != 0) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &controller->info) < 0) + VIR_WARN0("Unable to release PCI address on controller"); + VIR_FREE(devstr); return ret; } @@ -5610,6 +5628,10 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, qemuAssignNetNames(vm->def, net) < 0) goto no_memory; + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &net->info) < 0) + goto cleanup; + /* Choose a vlan value greater than all other values since * older versions did not store the value in the state file. */ @@ -5663,6 +5685,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, vm->def->nets[vm->def->nnets++] = net; cleanup: + if ((ret != 0) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &net->info) < 0) + VIR_WARN0("Unable to release PCI address on NIC"); + VIR_FREE(nicstr); VIR_FREE(netstr); VIR_FREE(tapfd_name); @@ -5736,9 +5764,13 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return -1; } - if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && - !(devstr = qemuBuildPCIHostdevDevStr(hostdev))) - goto error; + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &hostdev->info) < 0) + goto error; + + if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev))) + goto error; + } qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) @@ -5760,6 +5792,11 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return 0; error: + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + (hostdev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &hostdev->info) < 0) + VIR_WARN0("Unable to release PCI address on host device"); + VIR_FREE(devstr); pciDeviceListDel(conn, driver->activePciHostdevs, pci); -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
PCI disk, disk controllers, net devices and host devices need to have PCI addresses assigned before they are hot-plugged
* src/qemu/qemu_conf.c: Add APIs for ensuring a device has an address and releasing unused addresses * src/qemu/qemu_driver.c: Ensure all devices have addresses when hotplugging. ---
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:36PM +0000, Daniel P. Berrange wrote:
PCI disk, disk controllers, net devices and host devices need to have PCI addresses assigned before they are hot-plugged
* src/qemu/qemu_conf.c: Add APIs for ensuring a device has an address and releasing unused addresses * src/qemu/qemu_driver.c: Ensure all devices have addresses when hotplugging. --- src/qemu/qemu_conf.c | 60 ++++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_conf.h | 11 +++++++- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 17 deletions(-) [...] cleanup: + if ((ret != 0) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &net->info) < 0) + VIR_WARN0("Unable to release PCI address on NIC"); +
Wondering to which extend (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) check shouldn't be handled down in allocation and releasing routines instead of before each invocation, but it's minor. ACK, 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/

The QEMU 0.12.x tree has the -netdev command line argument, but not corresponding monitor command. We can't enable the former, without the latter since it will break hotplug/unplug. * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Disable -netdev usage until 0.13 at earliest * tests/qemuxml2argvtest.c: Add test for -netdev syntax * tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml: Test data files for -netdev syntax --- src/qemu/qemu_conf.c | 46 +++++++++++++++++--- src/qemu/qemu_conf.h | 4 +- .../qemuxml2argv-net-virtio-device.args | 2 +- .../qemuxml2argv-net-virtio-netdev.args | 1 + .../qemuxml2argv-net-virtio-netdev.xml | 26 +++++++++++ tests/qemuxml2argvtest.c | 1 + 6 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a70848c..c998fe2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1154,6 +1154,17 @@ static unsigned int qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_BALLOON; if (strstr(help, "-device")) flags |= QEMUD_CMD_FLAG_DEVICE; + /* Keep disabled till we're actually ready to turn on netdev mode + * The plan is todo it in 0.13.0 QEMU, but lets wait & see... */ +#if 0 + if (strstr(help, "-netdev")) { + /* Disable -netdev on 0.12 since although it exists, + * the corresponding netdev_add/remove monitor commands + * do not, and we need them to be able todo hotplug */ + if (version >= 13000) + flags |= QEMUD_CMD_FLAG_NETDEV; + } +#endif if (strstr(help, "-sdl")) flags |= QEMUD_CMD_FLAG_SDL; if (strstr(help, "cores=") && @@ -2379,7 +2390,7 @@ qemuBuildNicStr(virConnectPtr conn, char * -qemuBuildNicDevStr(virDomainNetDefPtr net) +qemuBuildNicDevStr(virDomainNetDefPtr net, int qemuCmdFlags) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *nic; @@ -2392,7 +2403,11 @@ qemuBuildNicDevStr(virDomainNetDefPtr net) nic = net->model; } - virBufferVSprintf(&buf, "%s,netdev=%s", nic, net->hostnet_name); + virBufferAdd(&buf, nic, strlen(nic)); + if (qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) + virBufferVSprintf(&buf, ",netdev=%s", net->hostnet_name); + else + virBufferVSprintf(&buf, ",vlan=%d", net->vlan); virBufferVSprintf(&buf, ",id=%s", net->info.alias); virBufferVSprintf(&buf, ",mac=%02x:%02x:%02x:%02x:%02x:%02x", net->mac[0], net->mac[1], @@ -3620,7 +3635,12 @@ int qemudBuildCommandLine(virConnectPtr conn, char *nic, *host; char tapfd_name[50]; - net->vlan = i; + /* VLANs are not used with -netdev, so don't record them */ + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) + net->vlan = -1; + else + net->vlan = i; if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { @@ -3645,14 +3665,24 @@ int qemudBuildCommandLine(virConnectPtr conn, goto no_memory; } - if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + /* Possible combinations: + * + * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 + * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 + * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 + * + * NB, no support for -netdev without use of -device + */ + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { ADD_ARG_LIT("-netdev"); if (!(host = qemuBuildNetDevStr(conn, net, tapfd_name))) goto error; ADD_ARG(host); - + } + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ADD_ARG_LIT("-device"); - if (!(nic = qemuBuildNicDevStr(net))) + if (!(nic = qemuBuildNicDevStr(net, qemuCmdFlags))) goto error; ADD_ARG(nic); } else { @@ -3660,7 +3690,9 @@ int qemudBuildCommandLine(virConnectPtr conn, if (!(nic = qemuBuildNicStr(conn, net, "nic,", net->vlan))) goto error; ADD_ARG(nic); - + } + if (!((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))) { ADD_ARG_LIT("-net"); if (!(host = qemuBuildHostNetStr(conn, net, ',', net->vlan, tapfd_name))) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index b94153f..3787e28 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -81,6 +81,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_DEVICE = (1 << 26), /* Is the new -device arg available */ QEMUD_CMD_FLAG_SDL = (1 << 27), /* Is the new -sdl arg available */ QEMUD_CMD_FLAG_SMP_TOPOLOGY = (1 << 28), /* Is sockets=s,cores=c,threads=t available for -smp? */ + QEMUD_CMD_FLAG_NETDEV = (1 << 29), /* The -netdev flag & netdev_add/remove monitor commands */ }; /* Main driver state */ @@ -210,7 +211,8 @@ char * qemuBuildNicStr(virConnectPtr conn, int vlan); /* Current, best practice */ -char * qemuBuildNicDevStr(virDomainNetDefPtr net); +char * qemuBuildNicDevStr(virDomainNetDefPtr net, + int qemuCmdFlags); /* Both legacy & current support support */ char *qemuBuildDriveStr(virDomainDiskDefPtr disk, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args index 698ad3a..32d2843 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -netdev user,id=netdev0 -device virtio-net-pci,netdev=netdev0,id=virtio-nic0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -device virtio-net-pci,vlan=0,id=virtio-nic0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -net user,vlan=0,name=netdev0 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args new file mode 100644 index 0000000..698ad3a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.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 -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -netdev user,id=netdev0 -device virtio-net-pci,netdev=netdev0,id=virtio-nic0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml new file mode 100644 index 0000000..5d34bd4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml @@ -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' bus='ide'/> + </disk> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + </interface> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 90c1cb0..67dc47e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -280,6 +280,7 @@ mymain(int argc, char **argv) DO_TEST("net-user", 0); DO_TEST("net-virtio", 0); DO_TEST("net-virtio-device", QEMUD_CMD_FLAG_DEVICE); + DO_TEST("net-virtio-netdev", QEMUD_CMD_FLAG_DEVICE | QEMUD_CMD_FLAG_NETDEV); DO_TEST("net-eth", 0); DO_TEST("net-eth-ifname", 0); DO_TEST("net-eth-names", QEMUD_CMD_FLAG_NET_NAME); -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The QEMU 0.12.x tree has the -netdev command line argument, but not corresponding monitor command. We can't enable the former, without the latter since it will break hotplug/unplug.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Disable -netdev usage until 0.13 at earliest * tests/qemuxml2argvtest.c: Add test for -netdev syntax * tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml: Test data files for -netdev syntax ---
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:37PM +0000, Daniel P. Berrange wrote:
The QEMU 0.12.x tree has the -netdev command line argument, but not corresponding monitor command. We can't enable the former, without the latter since it will break hotplug/unplug.
Argh, nasty
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Disable -netdev usage until 0.13 at earliest * tests/qemuxml2argvtest.c: Add test for -netdev syntax * tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml: Test data files for -netdev syntax
ACK, 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/

The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged * src/conf/domain_conf.c, src/conf/domain_conf.h: Remove hostnet_name and vlan fields from virNetworkDefPtr * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h, src/qemu/qemu_driver.c: Use a single network alias naming scheme regardless of NIC type or backend type. Determine VLANs from the alias name. * tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args: Update for new simpler naming scheme --- src/conf/domain_conf.c | 32 -- src/conf/domain_conf.h | 4 - src/qemu/qemu_conf.c | 310 ++++++-------------- src/qemu/qemu_conf.h | 9 +- src/qemu/qemu_driver.c | 45 ++-- .../qemuxml2argv-net-eth-names.args | 2 +- .../qemuxml2argv-net-virtio-device.args | 2 +- .../qemuxml2argv-net-virtio-netdev.args | 2 +- 8 files changed, 127 insertions(+), 279 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 691fc84..b434fc5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -433,7 +433,6 @@ void virDomainNetDefFree(virDomainNetDefPtr def) } VIR_FREE(def->ifname); - VIR_FREE(def->hostnet_name); virDomainDeviceInfoClear(&def->info); @@ -1631,10 +1630,7 @@ virDomainNetDefParseXML(virConnectPtr conn, char *port = NULL; char *model = NULL; char *internal = NULL; - char *nic_name = NULL; - char *hostnet_name = NULL; char *devaddr = NULL; - char *vlan = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -1704,10 +1700,7 @@ virDomainNetDefParseXML(virConnectPtr conn, } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ - nic_name = virXMLPropString(cur, "nic"); - hostnet_name = virXMLPropString(cur, "hostnet"); devaddr = virXMLPropString(cur, "devaddr"); - vlan = virXMLPropString(cur, "vlan"); } } cur = cur->next; @@ -1735,8 +1728,6 @@ virDomainNetDefParseXML(virConnectPtr conn, goto error; } def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - def->info.alias = nic_name; - nic_name = NULL; } else { if (virDomainDeviceInfoParseXML(conn, node, &def->info, flags) < 0) goto error; @@ -1751,16 +1742,6 @@ virDomainNetDefParseXML(virConnectPtr conn, goto error; } - def->hostnet_name = hostnet_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) { @@ -1880,10 +1861,7 @@ cleanup: VIR_FREE(model); VIR_FREE(type); VIR_FREE(internal); - VIR_FREE(nic_name); - VIR_FREE(hostnet_name); VIR_FREE(devaddr); - VIR_FREE(vlan); return def; @@ -4826,16 +4804,6 @@ virDomainNetDefFormat(virConnectPtr conn, virBufferEscapeString(buf, " <model type='%s'/>\n", def->model); - if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { - /* Legacy back-compat. Don't add any more attributes here */ - virBufferAddLit(buf, " <state"); - if (def->hostnet_name) - virBufferEscapeString(buf, " hostnet='%s'", def->hostnet_name); - if (def->vlan > 0) - virBufferVSprintf(buf, " vlan='%d'", def->vlan); - virBufferAddLit(buf, "/>\n"); - } - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index be0dc92..0b79e88 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -247,10 +247,6 @@ struct _virDomainNetDef { } data; char *ifname; virDomainDeviceInfo info; - /* XXX figure out how to remove this */ - char *hostnet_name; - /* XXX figure out how to remove this */ - int vlan; }; enum virDomainChrTargetType { diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index c998fe2..b255503 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1515,6 +1515,28 @@ cleanup: } +static int qemuDomainDeviceAliasIndex(virDomainDeviceInfoPtr info, + const char *prefix) +{ + int idx; + + if (!info->alias) + return -1; + if (!STRPREFIX(info->alias, prefix)) + return -1; + + if (virStrToLong_i(info->alias + strlen(prefix), NULL, 10, &idx) < 0) + return -1; + + return idx; +} + + +int qemuDomainNetVLAN(virDomainNetDefPtr def) +{ + return qemuDomainDeviceAliasIndex(&def->info, "net"); +} + static int qemuAssignDeviceAliases(virDomainDefPtr def) { @@ -1535,14 +1557,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def) } } for (i = 0; i < def->nnets ; i++) { - if (def->nets[i]->model) { - if (virAsprintf(&def->nets[i]->info.alias, "%s-nic%d", def->nets[i]->model, i) < 0) - goto no_memory; - } else { - if (virAsprintf(&def->nets[i]->info.alias, "nic%d", i) < 0) - goto no_memory; - } - if (virAsprintf(&def->nets[i]->hostnet_name, "netdev%d", i) < 0) + if (virAsprintf(&def->nets[i]->info.alias, "net%d", i) < 0) goto no_memory; } @@ -2025,68 +2040,31 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, } -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"; - } -} - 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]->info.alias || !def->nets[i]->hostnet_name) - continue; + int i; + int lastidx = -1; - 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; + for (i = 0; i < def->nnets && def->nets[i] != net ; i++) { + int idx; - if (STREQ(qemuNetTypeToHostNet(def->nets[i]->type), - qemuNetTypeToHostNet(net->type))) - ++hostnet_index; - } + if ((idx = qemuDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for network device")); + return -1; + } - if (virAsprintf(&nic_name, "%s.%d", - net->model ? net->model : "nic", - nic_index) < 0) { - virReportOOMError(NULL); - return -1; + if (idx > lastidx) + lastidx = idx; } - if (virAsprintf(&hostnet_name, "%s.%d", - qemuNetTypeToHostNet(net->type), - hostnet_index) < 0) { + if (virAsprintf(&net->info.alias, "net%d", lastidx + 1) < 0) { virReportOOMError(NULL); - VIR_FREE(nic_name); return -1; } - net->info.alias = nic_name; - net->hostnet_name = hostnet_name; - return 0; } @@ -2390,7 +2368,7 @@ qemuBuildNicStr(virConnectPtr conn, char * -qemuBuildNicDevStr(virDomainNetDefPtr net, int qemuCmdFlags) +qemuBuildNicDevStr(virDomainNetDefPtr net, int vlan) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *nic; @@ -2404,10 +2382,10 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, int qemuCmdFlags) } virBufferAdd(&buf, nic, strlen(nic)); - if (qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) - virBufferVSprintf(&buf, ",netdev=%s", net->hostnet_name); + if (vlan == -1) + virBufferVSprintf(&buf, ",netdev=host%s", net->info.alias); else - virBufferVSprintf(&buf, ",vlan=%d", net->vlan); + virBufferVSprintf(&buf, ",vlan=%d", vlan); virBufferVSprintf(&buf, ",id=%s", net->info.alias); virBufferVSprintf(&buf, ",mac=%02x:%02x:%02x:%02x:%02x:%02x", net->mac[0], net->mac[1], @@ -2436,177 +2414,79 @@ qemuBuildHostNetStr(virConnectPtr conn, int vlan, const char *tapfd) { - char *str = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; switch (net->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: - if (virAsprintf(&str, "tap%cfd=%s,vlan=%d%s%s", - type_sep, tapfd, vlan, - (net->hostnet_name ? ",name=" : ""), - (net->hostnet_name ? net->hostnet_name : "")) < 0) { - virReportOOMError(conn); - return NULL; - } + virBufferAddLit(&buf, "tap"); + virBufferVSprintf(&buf, "%cfd=%s", type_sep, tapfd); + type_sep = ','; break; case VIR_DOMAIN_NET_TYPE_ETHERNET: - { - virBuffer buf = VIR_BUFFER_INITIALIZER; - - 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 (net->hostnet_name) { - virBufferVSprintf(&buf, "%cname=%s", type_sep, - net->hostnet_name); - type_sep = ','; /* dead-store, but leave it, in case... */ - } - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(conn); - return NULL; - } - - str = virBufferContentAndReset(&buf); + 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 = ','; } 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, "socket%c%s=%s:%d,vlan=%d%s%s", - type_sep, mode, - net->data.socket.address, - net->data.socket.port, - vlan, - (net->hostnet_name ? ",name=" : ""), - (net->hostnet_name ? net->hostnet_name : "")) < 0) { - virReportOOMError(conn); - return NULL; - } + virBufferAddLit(&buf, "socket"); + switch (net->type) { + case VIR_DOMAIN_NET_TYPE_CLIENT: + virBufferVSprintf(&buf, "%cconnect=%s:%d", + type_sep, + net->data.socket.address, + net->data.socket.port); + break; + case VIR_DOMAIN_NET_TYPE_SERVER: + virBufferVSprintf(&buf, "%clisten=%s:%d", + type_sep, + net->data.socket.address, + net->data.socket.port); + break; + case VIR_DOMAIN_NET_TYPE_MCAST: + virBufferVSprintf(&buf, "%cmcast=%s:%d", + type_sep, + net->data.socket.address, + net->data.socket.port); + break; } + type_sep = ','; break; case VIR_DOMAIN_NET_TYPE_USER: default: - if (virAsprintf(&str, "user%cvlan=%d%s%s", - type_sep, vlan, - (net->hostnet_name ? ",name=" : ""), - (net->hostnet_name ? net->hostnet_name : "")) < 0) { - virReportOOMError(conn); - return NULL; - } + virBufferAddLit(&buf, "user"); break; } - return str; -} - - -char * -qemuBuildNetDevStr(virConnectPtr conn, - virDomainNetDefPtr net, - const char *tapfd) -{ - char *str = NULL; - - switch (net->type) { - case VIR_DOMAIN_NET_TYPE_NETWORK: - case VIR_DOMAIN_NET_TYPE_BRIDGE: - if (virAsprintf(&str, "tap,fd=%s,id=%s", - tapfd, net->hostnet_name) < 0) { - virReportOOMError(conn); - return NULL; - } - break; - - case VIR_DOMAIN_NET_TYPE_ETHERNET: - { - virBuffer buf = VIR_BUFFER_INITIALIZER; - - 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); - if (net->hostnet_name) - virBufferVSprintf(&buf, ",id=%s", - net->hostnet_name); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(conn); - return NULL; - } - - 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, "socket,%s=%s:%d,id=%s", - mode, - net->data.socket.address, - net->data.socket.port, - net->hostnet_name) < 0) { - virReportOOMError(conn); - return NULL; - } - } - break; + if (vlan >= 0) { + virBufferVSprintf(&buf, "%cvlan=%d", type_sep, vlan); + if (net->info.alias) + virBufferVSprintf(&buf, ",name=host%s", + net->info.alias); + } else { + virBufferVSprintf(&buf, "%cid=host%s", + type_sep, net->info.alias); + } - case VIR_DOMAIN_NET_TYPE_USER: - default: - if (virAsprintf(&str, "user,id=%s", - net->hostnet_name) < 0) { - virReportOOMError(conn); - return NULL; - } - break; + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(conn); + return NULL; } - return str; + return virBufferContentAndReset(&buf); } @@ -3634,13 +3514,14 @@ int qemudBuildCommandLine(virConnectPtr conn, virDomainNetDefPtr net = def->nets[i]; char *nic, *host; char tapfd_name[50]; + int vlan; /* VLANs are not used with -netdev, so don't record them */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) - net->vlan = -1; + vlan = -1; else - net->vlan = i; + vlan = i; if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { @@ -3676,18 +3557,19 @@ int qemudBuildCommandLine(virConnectPtr conn, if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { ADD_ARG_LIT("-netdev"); - if (!(host = qemuBuildNetDevStr(conn, net, tapfd_name))) + if (!(host = qemuBuildHostNetStr(conn, net, ',', + vlan, tapfd_name))) goto error; ADD_ARG(host); } if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ADD_ARG_LIT("-device"); - if (!(nic = qemuBuildNicDevStr(net, qemuCmdFlags))) + if (!(nic = qemuBuildNicDevStr(net, vlan))) goto error; ADD_ARG(nic); } else { ADD_ARG_LIT("-net"); - if (!(nic = qemuBuildNicStr(conn, net, "nic,", net->vlan))) + if (!(nic = qemuBuildNicStr(conn, net, "nic,", vlan))) goto error; ADD_ARG(nic); } @@ -3695,7 +3577,7 @@ int qemudBuildCommandLine(virConnectPtr conn, (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))) { ADD_ARG_LIT("-net"); if (!(host = qemuBuildHostNetStr(conn, net, ',', - net->vlan, tapfd_name))) + vlan, tapfd_name))) goto error; ADD_ARG(host); } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 3787e28..7b439fd 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -191,19 +191,13 @@ int qemudBuildCommandLine (virConnectPtr conn, int *ntapfds, const char *migrateFrom); -/* Legacy, pre device support */ +/* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virConnectPtr conn, virDomainNetDefPtr net, char type_sep, int vlan, const char *tapfd); -/* Current, best practice */ -char * qemuBuildNetDevStr(virConnectPtr conn, - virDomainNetDefPtr net, - const char *tapfd); - - /* Legacy, pre device support */ char * qemuBuildNicStr(virConnectPtr conn, virDomainNetDefPtr net, @@ -291,6 +285,7 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs); +int qemuDomainNetVLAN(virDomainNetDefPtr def); #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df7f8e7..f00bb75 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5595,11 +5595,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, { qemuDomainObjPrivatePtr priv = vm->privateData; char *tapfd_name = NULL; - int i, tapfd = -1; + int tapfd = -1; char *nicstr = NULL; char *netstr = NULL; int ret = -1; virDomainDevicePCIAddress guestAddr; + int vlan; if (!(qemuCmdFlags & QEMUD_CMD_FLAG_HOST_NET_ADD)) { qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", @@ -5626,22 +5627,16 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && qemuAssignNetNames(vm->def, net) < 0) - goto no_memory; + goto cleanup; if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &net->info) < 0) goto cleanup; - /* 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; + vlan = qemuDomainNetVLAN(net); if (tapfd != -1) { - if (virAsprintf(&tapfd_name, "fd-%s", net->hostnet_name) < 0) + if (virAsprintf(&tapfd_name, "fd-%s", net->info.alias) < 0) goto no_memory; qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -5653,7 +5648,7 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, } if (!(netstr = qemuBuildHostNetStr(conn, net, ' ', - net->vlan, tapfd_name))) + vlan, tapfd_name))) goto try_tapfd_close; qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -5667,7 +5662,7 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, close(tapfd); tapfd = -1; - if (!(nicstr = qemuBuildNicStr(conn, net, NULL, net->vlan))) + if (!(nicstr = qemuBuildNicStr(conn, net, NULL, vlan))) goto try_remove; qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -5700,14 +5695,18 @@ cleanup: return ret; try_remove: - if (!net->hostnet_name || net->vlan == 0) + if (vlan < 0) { VIR_WARN0(_("Unable to remove network backend")); - else { + } else { + char *hostnet_name; + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) + goto no_memory; qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorRemoveHostNetwork(priv->mon, net->vlan, net->hostnet_name) < 0) + if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) VIR_WARN(_("Failed to remove network backend for vlan %d, net %s"), - net->vlan, net->hostnet_name); + vlan, hostnet_name); qemuDomainObjExitMonitorWithDriver(driver, vm); + VIR_FREE(hostnet_name); } goto cleanup; @@ -6174,6 +6173,8 @@ qemudDomainDetachNetDevice(virConnectPtr conn, int i, ret = -1; virDomainNetDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; + int vlan; + char *hostnet_name = NULL; for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr net = vm->def->nets[i]; @@ -6200,9 +6201,14 @@ qemudDomainDetachNetDevice(virConnectPtr conn, goto cleanup; } - if (detach->vlan < 0 || !detach->hostnet_name) { + if ((vlan = qemuDomainNetVLAN(detach)) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - "%s", _("network device cannot be detached - device state missing")); + "%s", _("unable to determine original VLAN")); + goto cleanup; + } + + if (virAsprintf(&hostnet_name, "host%s", detach->info.alias) < 0) { + virReportOOMError(NULL); goto cleanup; } @@ -6213,7 +6219,7 @@ qemudDomainDetachNetDevice(virConnectPtr conn, goto cleanup; } - if (qemuMonitorRemoveHostNetwork(priv->mon, detach->vlan, detach->hostnet_name) < 0) { + if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); goto cleanup; } @@ -6248,6 +6254,7 @@ qemudDomainDetachNetDevice(virConnectPtr conn, ret = 0; cleanup: + VIR_FREE(hostnet_name); return ret; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args index 70a10cd..ad33a55 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args @@ -1 +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 +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=net0 -net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 -net nic,macaddr=00:11:22:33:44:56,vlan=1,model=e1000,name=net1 -net tap,script=/etc/qemu-ifup,vlan=1,name=hostnet1 -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args index 32d2843..48423a2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -device virtio-net-pci,vlan=0,id=virtio-nic0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -net user,vlan=0,name=netdev0 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -device virtio-net-pci,vlan=0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -net user,vlan=0,name=hostnet0 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args index 698ad3a..951bab2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -netdev user,id=netdev0 -device virtio-net-pci,netdev=netdev0,id=virtio-nic0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -netdev user,id=hostnet0 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x4 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged
* src/conf/domain_conf.c, src/conf/domain_conf.h: Remove hostnet_name and vlan fields from virNetworkDefPtr * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h, src/qemu/qemu_driver.c: Use a single network alias naming scheme regardless of NIC type or backend type. Determine VLANs from the alias name. * tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args: Update for new simpler naming scheme ---
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:38PM +0000, Daniel P. Berrange wrote:
The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged
ACK, this really simplifies the code, but isn't there a small risk of not being able to properly handle domains say after a libvirt upgrade and restart ? 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 Tue, Feb 02, 2010 at 03:58:19PM +0100, Daniel Veillard wrote:
On Mon, Feb 01, 2010 at 06:39:38PM +0000, Daniel P. Berrange wrote:
The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged
ACK, this really simplifies the code, but isn't there a small risk of not being able to properly handle domains say after a libvirt upgrade and restart ?
Yes & no. Any guests that were running before the upgrade will continue to run fine. You won't be able to hotplug further NICs to those guests until you restart them though. I did try to maintain the ability to hotplug into running guests after an upgrade, but it was getting more complex than I could cope with due to the really wierd naming scheme used by the old code. 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 Tue, Feb 02, 2010 at 03:01:46PM +0000, Daniel P. Berrange wrote:
On Tue, Feb 02, 2010 at 03:58:19PM +0100, Daniel Veillard wrote:
On Mon, Feb 01, 2010 at 06:39:38PM +0000, Daniel P. Berrange wrote:
The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged
ACK, this really simplifies the code, but isn't there a small risk of not being able to properly handle domains say after a libvirt upgrade and restart ?
Yes & no. Any guests that were running before the upgrade will continue to run fine. You won't be able to hotplug further NICs to those guests until you restart them though. I did try to maintain the ability to hotplug into running guests after an upgrade, but it was getting more complex than I could cope with due to the really wierd naming scheme used by the old code.
Okay, yes there is a very small tradeoff there but I think it's the right things to do, thanks ! 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/

This patch re-arranges the QEMU device alias assignment code to make it easier to call into the same codeblock when performing device hotplug. The new code has the ability to skip over already assigned names to facilitate hotplug * src/qemu/qemu_driver.c: Call qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_conf.h: Export qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_driver.c: Merge the legacy disk/network alias assignment code into the main methods --- src/qemu/qemu_conf.c | 295 +++++++++++++++++++++++++----------------------- src/qemu/qemu_conf.h | 4 +- src/qemu/qemu_driver.c | 14 ++- 3 files changed, 166 insertions(+), 147 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b255503..e8f2678 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1537,30 +1537,166 @@ int qemuDomainNetVLAN(virDomainNetDefPtr def) return qemuDomainDeviceAliasIndex(&def->info, "net"); } + +/* Names used before -drive existed */ +static int qemuAssignDeviceDiskAliasLegacy(virDomainDiskDefPtr disk) +{ + char *devname; + + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && + STREQ(disk->dst, "hdc")) + devname = strdup("cdrom"); + else + devname = strdup(disk->dst); + + if (!devname) { + virReportOOMError(NULL); + return -1; + } + + disk->info.alias = devname; + return 0; +} + + +/* Names used before -drive supported the id= option */ +static int qemuAssignDeviceDiskAliasFixed(virDomainDiskDefPtr disk) +{ + int busid, devid; + int ret; + char *devname; + + if (virDiskNameToBusDeviceIndex(disk, &busid, &devid) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot convert disk '%s' to bus/device index"), + disk->dst); + return -1; + } + + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + if (disk->device== VIR_DOMAIN_DISK_DEVICE_DISK) + ret = virAsprintf(&devname, "ide%d-hd%d", busid, devid); + else + ret = virAsprintf(&devname, "ide%d-cd%d", busid, devid); + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) + ret = virAsprintf(&devname, "scsi%d-hd%d", busid, devid); + else + ret = virAsprintf(&devname, "scsi%d-cd%d", busid, devid); + break; + case VIR_DOMAIN_DISK_BUS_FDC: + ret = virAsprintf(&devname, "floppy%d", devid); + break; + case VIR_DOMAIN_DISK_BUS_VIRTIO: + ret = virAsprintf(&devname, "virtio%d", devid); + break; + case VIR_DOMAIN_DISK_BUS_XEN: + ret = virAsprintf(&devname, "xenblk%d", devid); + break; + default: + qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_SUPPORT, + _("Unsupported disk name mapping for bus '%s'"), + virDomainDiskBusTypeToString(disk->bus)); + return -1; + } + + if (ret == -1) { + virReportOOMError(NULL); + return -1; + } + + disk->info.alias = devname; + + return 0; +} + + +/* Our custom -drive naming scheme used with id= */ +static int qemuAssignDeviceDiskAliasCustom(virDomainDiskDefPtr disk) +{ + const char *prefix = virDomainDiskBusTypeToString(disk->bus); + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + if (virAsprintf(&disk->info.alias, "%s%d-%d-%d", prefix, + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.unit) < 0) + goto no_memory; + } else { + int idx = virDiskNameToIndex(disk->dst); + if (virAsprintf(&disk->info.alias, "%s-disk%d", prefix, idx) < 0) + goto no_memory; + } + + return 0; + +no_memory: + virReportOOMError(NULL); + return -1; +} + + static int -qemuAssignDeviceAliases(virDomainDefPtr def) +qemuAssignDeviceDiskAlias(virDomainDiskDefPtr def, int qemuCmdFlags) +{ + if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) { + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) + return qemuAssignDeviceDiskAliasCustom(def); + else + return qemuAssignDeviceDiskAliasFixed(def); + } else { + return qemuAssignDeviceDiskAliasLegacy(def); + } +} + + +int +qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx) +{ + if (idx == -1) { + int i; + idx = 0; + for (i = 0 ; i < def->nnets ; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for network device")); + return -1; + } + if (thisidx >= idx) + idx = thisidx + 1; + } + } + + if (virAsprintf(&net->info.alias, "net%d", idx) < 0) { + virReportOOMError(NULL); + return -1; + } + + return 0; +} + +static int +qemuAssignDeviceAliases(virDomainDefPtr def, int qemuCmdFlags) { int i; for (i = 0; i < def->ndisks ; i++) { - const char *prefix = virDomainDiskBusTypeToString(def->disks[i]->bus); - if (def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { - if (virAsprintf(&def->disks[i]->info.alias, "%s%d-%d-%d", prefix, - def->disks[i]->info.addr.drive.controller, - def->disks[i]->info.addr.drive.bus, - def->disks[i]->info.addr.drive.unit) < 0) - goto no_memory; - } else { - int idx = virDiskNameToIndex(def->disks[i]->dst); - if (virAsprintf(&def->disks[i]->info.alias, "%s-disk%d", prefix, idx) < 0) - goto no_memory; - } + if (qemuAssignDeviceDiskAlias(def->disks[i], qemuCmdFlags) < 0) + return -1; } - for (i = 0; i < def->nnets ; i++) { - if (virAsprintf(&def->nets[i]->info.alias, "net%d", i) < 0) - goto no_memory; + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) || + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { + for (i = 0; i < def->nnets ; i++) { + if (qemuAssignDeviceNetAlias(def, def->nets[i], i) < 0) + return -1; + } } + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) + return 0; + for (i = 0; i < def->nsounds ; i++) { if (virAsprintf(&def->sounds[i]->info.alias, "sound%d", i) < 0) goto no_memory; @@ -1921,92 +2057,6 @@ error: } -static char *qemuDiskLegacyName(const virDomainDiskDefPtr disk) -{ - char *devname; - - if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && - STREQ(disk->dst, "hdc")) - devname = strdup("cdrom"); - else - devname = strdup(disk->dst); - - if (!devname) - virReportOOMError(NULL); - - return NULL; -} - -/* Return the -drive QEMU disk name for use in monitor commands */ -static char *qemuDiskDriveName(const virDomainDiskDefPtr disk) -{ - int busid, devid; - int ret; - char *devname; - - if (virDiskNameToBusDeviceIndex(disk, &busid, &devid) < 0) { - qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot convert disk '%s' to bus/device index"), - disk->dst); - return NULL; - } - - switch (disk->bus) { - case VIR_DOMAIN_DISK_BUS_IDE: - if (disk->device== VIR_DOMAIN_DISK_DEVICE_DISK) - ret = virAsprintf(&devname, "ide%d-hd%d", busid, devid); - else - ret = virAsprintf(&devname, "ide%d-cd%d", busid, devid); - break; - case VIR_DOMAIN_DISK_BUS_SCSI: - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) - ret = virAsprintf(&devname, "scsi%d-hd%d", busid, devid); - else - ret = virAsprintf(&devname, "scsi%d-cd%d", busid, devid); - break; - case VIR_DOMAIN_DISK_BUS_FDC: - ret = virAsprintf(&devname, "floppy%d", devid); - break; - case VIR_DOMAIN_DISK_BUS_VIRTIO: - ret = virAsprintf(&devname, "virtio%d", devid); - break; - case VIR_DOMAIN_DISK_BUS_XEN: - ret = virAsprintf(&devname, "xenblk%d", devid); - break; - default: - qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_SUPPORT, - _("Unsupported disk name mapping for bus '%s'"), - virDomainDiskBusTypeToString(disk->bus)); - return NULL; - } - - if (ret == -1) { - virReportOOMError(NULL); - return NULL; - } - - return devname; -} - -static int -qemuAssignDiskAliases(virDomainDefPtr def, int qemuCmdFlags) -{ - int i; - - for (i = 0 ; i < def->ndisks ; i++) { - if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) - def->disks[i]->info.alias = - qemuDiskDriveName(def->disks[i]); - else - def->disks[i]->info.alias = - qemuDiskLegacyName(def->disks[i]); - - if (!def->disks[i]->info.alias) - return -1; - } - return 0; -} - static int qemuBuildDeviceAddressStr(virBufferPtr buf, virDomainDeviceInfoPtr info) @@ -2040,34 +2090,6 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, } -int -qemuAssignNetNames(virDomainDefPtr def, - virDomainNetDefPtr net) -{ - int i; - int lastidx = -1; - - for (i = 0; i < def->nnets && def->nets[i] != net ; i++) { - int idx; - - if ((idx = qemuDomainDeviceAliasIndex(&def->nets[i]->info, "net")) < 0) { - qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to determine device index for network device")); - return -1; - } - - if (idx > lastidx) - lastidx = idx; - } - - if (virAsprintf(&net->info.alias, "net%d", lastidx + 1) < 0) { - virReportOOMError(NULL); - return -1; - } - - return 0; -} - #define QEMU_SERIAL_PARAM_ACCEPTED_CHARS \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_" @@ -2983,11 +3005,8 @@ int qemudBuildCommandLine(virConnectPtr conn, uname_normalize(&ut); - if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - qemuAssignDeviceAliases(def); - } else { - qemuAssignDiskAliases(def, qemuCmdFlags); - } + if (qemuAssignDeviceAliases(def, qemuCmdFlags) < 0) + return -1; virUUIDFormat(def->uuid, uuid); @@ -3540,12 +3559,6 @@ int qemudBuildCommandLine(virConnectPtr conn, goto no_memory; } - if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && - !(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - if (qemuAssignNetNames(def, net) < 0) - goto no_memory; - } - /* Possible combinations: * * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 7b439fd..b7b1666 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -247,9 +247,6 @@ int qemudNetworkIfaceConnect (virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags); -int qemuAssignNetNames (virDomainDefPtr def, - virDomainNetDefPtr net); - int qemudProbeMachineTypes (const char *binary, virCapsGuestMachinePtr **machines, int *nmachines); @@ -286,6 +283,7 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs); int qemuDomainNetVLAN(virDomainNetDefPtr def); +int qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx); #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f00bb75..23bbd6b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5625,9 +5625,11 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) goto no_memory; - if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) && - qemuAssignNetNames(vm->def, net) < 0) - goto cleanup; + if ((qemuCmdFlags & QEMUD_CMD_FLAG_NET_NAME) || + (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { + if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) + goto cleanup; + } if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &net->info) < 0) @@ -5635,6 +5637,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn, vlan = qemuDomainNetVLAN(net); + if (vlan < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s", + _("Unable to attach network devices without vlan")); + goto cleanup; + } + if (tapfd != -1) { if (virAsprintf(&tapfd_name, "fd-%s", net->info.alias) < 0) goto no_memory; -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
This patch re-arranges the QEMU device alias assignment code to make it easier to call into the same codeblock when performing device hotplug. The new code has the ability to skip over already assigned names to facilitate hotplug
* src/qemu/qemu_driver.c: Call qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_conf.h: Export qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_driver.c: Merge the legacy disk/network alias assignment code into the main methods ---
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:39PM +0000, Daniel P. Berrange wrote:
This patch re-arranges the QEMU device alias assignment code to make it easier to call into the same codeblock when performing device hotplug. The new code has the ability to skip over already assigned names to facilitate hotplug
* src/qemu/qemu_driver.c: Call qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_conf.h: Export qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_driver.c: Merge the legacy disk/network alias assignment code into the main methods --- src/qemu/qemu_conf.c | 295 +++++++++++++++++++++++++----------------------- src/qemu/qemu_conf.h | 4 +- src/qemu/qemu_driver.c | 14 ++- 3 files changed, 166 insertions(+), 147 deletions(-)
ACK, 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/

--- src/qemu/qemu_monitor_text.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index b2a0c53..d6cbea8 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -44,6 +44,8 @@ #define QEMU_CMD_PROMPT "\n(qemu) " #define QEMU_PASSWD_PROMPT "Password: " +#define DEBUG_IO 0 + /* Return -1 for error, 0 for success */ typedef int qemuMonitorExtraPromptHandler(qemuMonitorPtr mon, const char *buf, @@ -67,7 +69,7 @@ typedef int qemuMonitorExtraPromptHandler(qemuMonitorPtr mon, int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, const char *data, - size_t len, + size_t len ATTRIBUTE_UNUSED, qemuMonitorMessagePtr msg) { int used = 0; @@ -79,18 +81,24 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* We see the greeting prefix, but not postfix, so pretend we've not consumed anything. We'll restart when more data arrives. */ if (!offset) { +#if DEBUG_IO VIR_DEBUG0("Partial greeting seen, getting out & waiting for more"); +#endif return 0; } used = offset - data + strlen(GREETING_POSTFIX); +#if DEBUG_IO VIR_DEBUG0("Discarded monitor greeting"); +#endif } /* Don't print raw data in debug because its full of control chars */ /*VIR_DEBUG("Process data %d byts of data [%s]", len - used, data + used);*/ +#if DEBUG_IO VIR_DEBUG("Process data %d byts of data", (int)(len - used)); +#endif /* Look for a non-zero reply followed by prompt */ if (msg && !msg->finished) { @@ -138,7 +146,9 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* We might get a prompt for a password before the (qemu) prompt */ passwd = strstr(start, PASSWORD_PROMPT); if (passwd) { +#if DEBUG_IO VIR_DEBUG("Seen a passwowrd prompt [%s]", data + used); +#endif if (msg->passwordHandler) { int i; /* Try and handle the prompt */ @@ -176,9 +186,11 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, memcpy(msg->rxBuffer + msg->rxLength, start, want); msg->rxLength += want; msg->rxBuffer[msg->rxLength] = '\0'; +#if DEBUG_IO VIR_DEBUG("Finished %d byte reply [%s]", want, msg->rxBuffer); } else { VIR_DEBUG0("Finished 0 byte reply"); +#endif } msg->finished = 1; used += end - (data + used); @@ -186,7 +198,9 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } } +#if DEBUG_IO VIR_DEBUG("Total used %d", used); +#endif return used; } -- 1.6.5.2

On Mon, Feb 01, 2010 at 06:39:40PM +0000, Daniel P. Berrange wrote:
--- src/qemu/qemu_monitor_text.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index b2a0c53..d6cbea8 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -44,6 +44,8 @@ #define QEMU_CMD_PROMPT "\n(qemu) " #define QEMU_PASSWD_PROMPT "Password: "
+#define DEBUG_IO 0 + /* Return -1 for error, 0 for success */ typedef int qemuMonitorExtraPromptHandler(qemuMonitorPtr mon, const char *buf, @@ -67,7 +69,7 @@ typedef int qemuMonitorExtraPromptHandler(qemuMonitorPtr mon,
int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, const char *data, - size_t len, + size_t len ATTRIBUTE_UNUSED, qemuMonitorMessagePtr msg) { int used = 0; @@ -79,18 +81,24 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* We see the greeting prefix, but not postfix, so pretend we've not consumed anything. We'll restart when more data arrives. */ if (!offset) { +#if DEBUG_IO VIR_DEBUG0("Partial greeting seen, getting out & waiting for more"); +#endif return 0; }
used = offset - data + strlen(GREETING_POSTFIX);
+#if DEBUG_IO VIR_DEBUG0("Discarded monitor greeting"); +#endif }
/* Don't print raw data in debug because its full of control chars */ /*VIR_DEBUG("Process data %d byts of data [%s]", len - used, data + used);*/ +#if DEBUG_IO VIR_DEBUG("Process data %d byts of data", (int)(len - used)); +#endif
/* Look for a non-zero reply followed by prompt */ if (msg && !msg->finished) { @@ -138,7 +146,9 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* We might get a prompt for a password before the (qemu) prompt */ passwd = strstr(start, PASSWORD_PROMPT); if (passwd) { +#if DEBUG_IO VIR_DEBUG("Seen a passwowrd prompt [%s]", data + used); +#endif if (msg->passwordHandler) { int i; /* Try and handle the prompt */ @@ -176,9 +186,11 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, memcpy(msg->rxBuffer + msg->rxLength, start, want); msg->rxLength += want; msg->rxBuffer[msg->rxLength] = '\0'; +#if DEBUG_IO VIR_DEBUG("Finished %d byte reply [%s]", want, msg->rxBuffer); } else { VIR_DEBUG0("Finished 0 byte reply"); +#endif } msg->finished = 1; used += end - (data + used); @@ -186,7 +198,9 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } }
+#if DEBUG_IO VIR_DEBUG("Total used %d", used); +#endif return used; }
ACK, this was really filling up full debug logs ! 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/

To allow devices to be hot(un-)plugged it is neccessary to ensure they all have a unique device aliases. This fixes the hotplug methods to assign device aliases before invoking the monitor commands which need them * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Expose methods for assigning device aliases for disks, host devices and controllers * src/qemu/qemu_driver.c: Assign device aliases when hotplugging all types of device * tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args: Update for changed hostdev naming scheme --- src/qemu/qemu_conf.c | 61 +++++++++++++++---- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_driver.c | 35 +++++++----- .../qemuxml2argv-hostdev-pci-address-device.args | 2 +- .../qemuxml2argv-hostdev-usb-address-device.args | 2 +- 5 files changed, 74 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e8f2678..074fb7b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1637,7 +1637,7 @@ no_memory: } -static int +int qemuAssignDeviceDiskAlias(virDomainDiskDefPtr def, int qemuCmdFlags) { if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) { @@ -1677,6 +1677,49 @@ qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx) return 0; } + +int +qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev, int idx) +{ + if (idx == -1) { + int i; + idx = 0; + for (i = 0 ; i < def->nhostdevs ; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&def->hostdevs[i]->info, "hostdev")) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for hostdevwork device")); + return -1; + } + if (thisidx >= idx) + idx = thisidx + 1; + } + } + + if (virAsprintf(&hostdev->info.alias, "hostdev%d", idx) < 0) { + virReportOOMError(NULL); + return -1; + } + + return 0; +} + + +int +qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) +{ + const char *prefix = virDomainControllerTypeToString(controller->type); + + if (virAsprintf(&controller->info.alias, "%s%d", prefix, + controller->idx) < 0) { + virReportOOMError(NULL); + return -1; + } + + return 0; +} + + static int qemuAssignDeviceAliases(virDomainDefPtr def, int qemuCmdFlags) { @@ -1702,24 +1745,16 @@ qemuAssignDeviceAliases(virDomainDefPtr def, int qemuCmdFlags) goto no_memory; } for (i = 0; i < def->nhostdevs ; i++) { - if (def->hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { - const char *prefix = virDomainHostdevSubsysTypeToString - (def->hostdevs[i]->source.subsys.type); - if (virAsprintf(&def->hostdevs[i]->info.alias, "host%s%d", prefix, i) < 0) - goto no_memory; - } else { - if (virAsprintf(&def->hostdevs[i]->info.alias, "host%d",i) < 0) - goto no_memory; - } + if (qemuAssignDeviceHostdevAlias(def, def->hostdevs[i], i) < 0) + return -1; } for (i = 0; i < def->nvideos ; i++) { if (virAsprintf(&def->videos[i]->info.alias, "video%d", i) < 0) goto no_memory; } for (i = 0; i < def->ncontrollers ; i++) { - const char *prefix = virDomainControllerTypeToString(def->controllers[i]->type); - if (virAsprintf(&def->controllers[i]->info.alias, "%s%d", prefix, i) < 0) - goto no_memory; + if (qemuAssignDeviceControllerAlias(def->controllers[i]) < 0) + return -1; } for (i = 0; i < def->ninputs ; i++) { if (virAsprintf(&def->inputs[i]->info.alias, "input%d", i) < 0) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index b7b1666..1a0fac0 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -284,6 +284,9 @@ int qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr ad int qemuDomainNetVLAN(virDomainNetDefPtr def); int qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx); +int qemuAssignDeviceDiskAlias(virDomainDiskDefPtr def, int qemuCmdFlags); +int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr net, int idx); +int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller); #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 23bbd6b..5e58ce2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5240,6 +5240,8 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &disk->info) < 0) goto error; + if (qemuAssignDeviceDiskAlias(disk, qemuCmdFlags) < 0) + goto error; if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) goto error; @@ -5322,9 +5324,12 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, } } - if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && - qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) - goto cleanup; + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) + goto cleanup; + if (qemuAssignDeviceControllerAlias(controller) < 0) + goto cleanup; + } if (!(devstr = qemuBuildControllerDevStr(controller))) { virReportOOMError(NULL); @@ -5428,17 +5433,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1; - /* This func allocates the bus/unit IDs so must be before - * we search for controller - */ - if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) - goto error; - - if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && - !(devstr = qemuBuildDriveDevStr(NULL, disk))) - goto error; - - /* We should have an adddress now, so make sure */ + /* We should have an adddress already, so make sure */ if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unexpected disk address type %s"), @@ -5446,6 +5441,16 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, goto error; } + if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (qemuAssignDeviceDiskAlias(disk, qemuCmdFlags) < 0) + goto error; + if (!(devstr = qemuBuildDriveDevStr(NULL, disk))) + goto error; + } + + if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) + goto error; + for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i, qemuCmdFlags); if (!cont) @@ -5540,6 +5545,8 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, } if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { + if (qemuAssignDeviceDiskAlias(disk, qemuCmdFlags) < 0) + goto error; if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) goto error; if (!(devstr = qemuBuildDriveDevStr(NULL, disk))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args index 1b8a130..868f892 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest2 -usb -device pci-assign,host=06:12.5,id=hostpci0,bus=pci.0,addr=0x4 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest2 -usb -device pci-assign,host=06:12.5,id=hostdev0,bus=pci.0,addr=0x4 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args index cdab007..0d39928 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -usb -device usb-host,hostbus=014,hostaddr=006,id=hostusb0 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -usb -device usb-host,hostbus=014,hostaddr=006,id=hostdev0 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
To allow devices to be hot(un-)plugged it is neccessary to ensure they all have a unique device aliases. This fixes the hotplug methods to assign device aliases before invoking the monitor commands which need them
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Expose methods for assigning device aliases for disks, host devices and controllers * src/qemu/qemu_driver.c: Assign device aliases when hotplugging all types of device * tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args: Update for changed hostdev naming scheme ---
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 23bbd6b..5e58ce2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -5428,17 +5433,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1;
- /* This func allocates the bus/unit IDs so must be before - * we search for controller - */ - if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) - goto error; - - if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && - !(devstr = qemuBuildDriveDevStr(NULL, disk))) - goto error; - - /* We should have an adddress now, so make sure */ + /* We should have an adddress already, so make sure */
You could fix the ddd typo here.
if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unexpected disk address type %s"),
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:41PM +0000, Daniel P. Berrange wrote:
To allow devices to be hot(un-)plugged it is neccessary to ensure they all have a unique device aliases. This fixes the hotplug methods to assign device aliases before invoking the monitor commands which need them
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Expose methods for assigning device aliases for disks, host devices and controllers * src/qemu/qemu_driver.c: Assign device aliases when hotplugging all types of device * tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args: Update for changed hostdev naming scheme --- src/qemu/qemu_conf.c | 61 +++++++++++++++---- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_driver.c | 35 +++++++----- .../qemuxml2argv-hostdev-pci-address-device.args | 2 +- .../qemuxml2argv-hostdev-usb-address-device.args | 2 +- 5 files changed, 74 insertions(+), 29 deletions(-)
ACK, 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/

When attaching a USB host device based on vendor/product, libvirt will resolve the vendor/product into a device/bus pair. This means that when printing XML we should allow device/bus info to be printed at any time if present * src/conf/domain_conf.c, docs/schemas/domain.rng: Allow USB device bus info alongside vendor/product --- docs/schemas/domain.rng | 7 ++++++- src/conf/domain_conf.c | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 827ff6f..bb6d00d 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1179,7 +1179,12 @@ <group> <element name="source"> <choice> - <ref name="usbproduct"/> + <group> + <ref name="usbproduct"/> + <optional> + <ref name="usbaddress"/> + </optional> + </group> <ref name="usbaddress"/> <element name="address"> <ref name="pciaddress"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b434fc5..766993c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5244,11 +5244,12 @@ virDomainHostdevDefFormat(virConnectPtr conn, def->source.subsys.u.usb.vendor); virBufferVSprintf(buf, " <product id='0x%.4x'/>\n", def->source.subsys.u.usb.product); - } else { + } + if (def->source.subsys.u.usb.bus || + def->source.subsys.u.usb.device) virBufferVSprintf(buf, " <address bus='%d' device='%d'/>\n", def->source.subsys.u.usb.bus, def->source.subsys.u.usb.device); - } } else if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { virBufferVSprintf(buf, " <address domain='0x%.4x' bus='0x%.2x' slot='0x%.2x' function='0x%.1x'/>\n", def->source.subsys.u.pci.domain, -- 1.6.5.2

2010/2/1 Daniel P. Berrange <berrange@redhat.com>:
When attaching a USB host device based on vendor/product, libvirt will resolve the vendor/product into a device/bus pair. This means that when printing XML we should allow device/bus info to be printed at any time if present
* src/conf/domain_conf.c, docs/schemas/domain.rng: Allow USB device bus info alongside vendor/product ---
ACK Matthias

On Mon, Feb 01, 2010 at 06:39:42PM +0000, Daniel P. Berrange wrote:
When attaching a USB host device based on vendor/product, libvirt will resolve the vendor/product into a device/bus pair. This means that when printing XML we should allow device/bus info to be printed at any time if present
* src/conf/domain_conf.c, docs/schemas/domain.rng: Allow USB device bus info alongside vendor/product --- docs/schemas/domain.rng | 7 ++++++- src/conf/domain_conf.c | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 827ff6f..bb6d00d 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1179,7 +1179,12 @@ <group> <element name="source"> <choice> - <ref name="usbproduct"/> + <group> + <ref name="usbproduct"/> + <optional> + <ref name="usbaddress"/> + </optional> + </group> <ref name="usbaddress"/> <element name="address"> <ref name="pciaddress"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b434fc5..766993c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5244,11 +5244,12 @@ virDomainHostdevDefFormat(virConnectPtr conn, def->source.subsys.u.usb.vendor); virBufferVSprintf(buf, " <product id='0x%.4x'/>\n", def->source.subsys.u.usb.product); - } else { + } + if (def->source.subsys.u.usb.bus || + def->source.subsys.u.usb.device) virBufferVSprintf(buf, " <address bus='%d' device='%d'/>\n", def->source.subsys.u.usb.bus, def->source.subsys.u.usb.device); - } } else if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { virBufferVSprintf(buf, " <address domain='0x%.4x' bus='0x%.2x' slot='0x%.2x' function='0x%.1x'/>\n", def->source.subsys.u.pci.domain,
ACK, 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/
participants (3)
-
Daniel P. Berrange
-
Daniel Veillard
-
Matthias Bolte