- Devel - libvirt List Archives

Re: [libvirt] patch for disk migration verbose progress
by Eric Blake 17 Aug '11

17 Aug '11

[re-adding the list] On 08/17/2011 12:30 AM, Tom Vijlbrief wrote: > Hi Eric > > This is the complete patch including json (not tested) and a typo correction > in an error message... > > Tom > ---------- Doorgestuurd bericht ---------- > Van: "Tom Vijlbrief"<tom(a)nomadbsd.v7f.eu> > Datum: 17 aug. 2011 08:23 > Onderwerp: mon.patch > Aan:<tvijlbrief(a)gmail.com> > > > mon.patch > Thanks again for taking the time to contribute. It is easier to work with patches generated by 'git format-patch' (and 'git send-email' is a nice wrapper that both formats the patch and mails it to the list); as written, your patch had no commit message, so I had to invent one. I also added you to AUTHORS; let me know if I need to update any preferred spellings. > > # NB, must pass the --listen flag to the libvirtd process for this to Is this a stray line in your email, or is it pointing out a further issue? > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 2a9a078..618a45c 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -1894,6 +1894,31 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr reply, > _("migration was active, but RAM 'total' data was missing")); > return -1; > } > + > + virJSONValuePtr disk = virJSONValueObjectGet(ret, "disk"); > + if (!disk) { > + return 0; > + } > + > + unsigned long long t; Our style tends to float declarations to the top, C89 style, even though this is valid as written (we require C99 for other reasons). > + if (virJSONValueObjectGetNumberUlong(disk, "transferred",&t)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("disk migration was active, but DISK 'transferred' data was missing")); > + return -1; > + } > + (*transferred)+= t; > + if (virJSONValueObjectGetNumberUlong(disk, "remaining",&t)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("disk migration was active, but DISK 'remaining' data was missing")); > + return -1; > + } > + (*remaining)+= t; I tweaked spacing between operators (yeah, it doesn't help that thunderbird corrupted spacing even worse in my reply to your mail). And the () are redundant; this can safely be: *remaining += t; > + if (virJSONValueObjectGetNumberUlong(disk, "total",&t)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("disk migration was active, but DISK 'total' data was missing")); > + return -1; > + } > + (*total)+= t; > } > > return 0; > diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c > index 7bf733d..fef6f36 100644 > --- a/src/qemu/qemu_monitor_text.c > +++ b/src/qemu/qemu_monitor_text.c > @@ -149,7 +149,7 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, > passwd = strstr(start, PASSWORD_PROMPT); > if (passwd) { > #if DEBUG_IO > - VIR_DEBUG("Seen a passwowrd prompt [%s]", data + used); > + VIR_DEBUG("Seen a password prompt [%s]", data + used); > #endif > if (msg->passwordHandler) { > int i; > @@ -1183,6 +1183,9 @@ cleanup: > #define MIGRATION_TRANSFER_PREFIX "transferred ram: " > #define MIGRATION_REMAINING_PREFIX "remaining ram: " > #define MIGRATION_TOTAL_PREFIX "total ram: " > +#define MIGRATION_DISK_TRANSFER_PREFIX "transferred disk: " > +#define MIGRATION_DISK_REMAINING_PREFIX "remaining disk: " > +#define MIGRATION_DISK_TOTAL_PREFIX "total disk: " > > int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon, > int *status, > @@ -1246,6 +1249,7 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon, > goto cleanup; > } > *remaining *= 1024; > + tmp = end; > > if (!(tmp = strstr(tmp, MIGRATION_TOTAL_PREFIX))) > goto done; > @@ -1257,7 +1261,53 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon, > goto cleanup; > } > *total *= 1024; > + tmp = end; > + > + /* > + * Check for Optional Disk Migration status > + */ > + > + unsigned long long disk_transferred= 0; > + unsigned long long disk_remaining= 0; > + unsigned long long disk_total= 0; > + > + if (!(tmp = strstr(tmp, MIGRATION_DISK_TRANSFER_PREFIX))) > + goto done; > + tmp += strlen(MIGRATION_DISK_TRANSFER_PREFIX); > > + if (virStrToLong_ull(tmp,&end, 10,&disk_transferred)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse disk migration data transferred statistic %s"), tmp); > + goto cleanup; > + } > + disk_transferred *= 1024; > + (*transferred)+= disk_transferred; For one less assignment, I merged these two lines into: *transferred += disk_transferred * 1024; > + tmp = end; > + > + if (!(tmp = strstr(tmp, MIGRATION_DISK_REMAINING_PREFIX))) > + goto done; > + tmp += strlen(MIGRATION_DISK_REMAINING_PREFIX); > + > + if (virStrToLong_ull(tmp,&end, 10,&disk_remaining)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse disk migration data remaining statistic %s"), tmp); > + goto cleanup; > + } > + disk_remaining *= 1024; > + (*remaining)+= disk_remaining; > + tmp = end; > + > + if (!(tmp = strstr(tmp, MIGRATION_DISK_TOTAL_PREFIX))) > + goto done; > + tmp += strlen(MIGRATION_DISK_TOTAL_PREFIX); > + > + if (virStrToLong_ull(tmp,&end, 10,&disk_total)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse disk migration data total statistic %s"), tmp); > + goto cleanup; > + } > + disk_total *= 1024; > + (*total)+= disk_total; > } > } ACK and pushed with those tweaks. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

1 0

Re: [libvirt] Patch for verbose Disk transfer
by Eric Blake 17 Aug '11

17 Aug '11

[re-adding the list] On 08/16/2011 11:52 PM, Tom Vijlbrief wrote: > Hi Eric, > > Yes json has similar info and I also patched it, but I don't know how to > configure vir-sh so that it uses the json channel for at least testing the > patch. I can send you the patch anyway, it's a simple change... Use of JSON depends on which version of qemu you are running; if you are using Fedora 15 or newer, then JSON should automatically be in use. It's not something you can configure at the virsh command line. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

1 0

[libvirt] [PATCH] qemu: Init reattaching related members of pciDevice before reattach
by Osier Yang 17 Aug '11

17 Aug '11

Otherwise the device will still be bound to pci-stub driver even it's set as "managed=yes" when do detaching. Of course, it won't triger any driver reprobing too. --- src/qemu/qemu_hotplug.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5f449fb..b7fdfa0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1761,6 +1761,7 @@ int qemuDomainDetachHostPciDevice(struct qemud_driver *driver, pciDeviceListDel(driver->activePciHostdevs, pci); if (pciResetDevice(pci, driver->activePciHostdevs, NULL) < 0) ret = -1; + pciDeviceReAttachInit(pci); qemuReattachPciDevice(pci, driver); pciFreeDevice(pci); } -- 1.7.6

2 2

[libvirt] [PATCH] conf: Do not load domain if the domain with same already exists
by Osier Yang 17 Aug '11

17 Aug '11

We don't allow to define domain with same name and different UUID, or with same UUID, so it's reasonable to not load the domain config if there is domain with same name already exists. Otherwise it can cause problem like: 1) % cp /etc/libvirt/qemu/dom.xml /etc/libvirt/qemu/dom_diffuuid.xml 2) remove the line with "uuid" in the "dom_diffuuid.xml" 3) % for i in {1..10}; do kill -SIGHUP $(pidof libvirtd); done 4) % virsh list --all There will be 11 domains listed with the same name, as if there is no UUID specified in domain XML, libvirt will generate one for it, which will be definitely different with the original one. --- src/conf/domain_conf.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce1f3c5..0bed929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10535,7 +10535,8 @@ static virDomainObjPtr virDomainLoadConfig(virCapsPtr caps, /* if the domain is already in our hashtable, we only need to * update the autostart flag */ - if ((dom = virDomainFindByUUID(doms, def->uuid))) { + if ((dom = virDomainFindByUUID(doms, def->uuid)) || + (dom = virDomainFindByName(doms, def->name))) { dom->autostart = autostart; VIR_FREE(configFile); -- 1.7.6

2 2

[libvirt] [PATCH] Ensure stream is aborted when exiting console
by Daniel P. Berrange 17 Aug '11

17 Aug '11

From: "Daniel P. Berrange" <berrange(a)redhat.com> After running 'virsh console' in interactive mode, there was a missing call to virStreamAbort, which meant the server kept the stream resources open * tools/console.c: Abort stream when exiting --- tools/console.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/console.c b/tools/console.c index 171ebc9..82ef537 100644 --- a/tools/console.c +++ b/tools/console.c @@ -90,9 +90,11 @@ static void virConsoleShutdown(virConsolePtr con) { con->quit = true; - virStreamEventRemoveCallback(con->st); - if (con->st) + if (con->st) { + virStreamEventRemoveCallback(con->st); + virStreamAbort(con->st); virStreamFree(con->st); + } if (con->stdinWatch != -1) virEventRemoveHandle(con->stdinWatch); if (con->stdoutWatch != -1) -- 1.7.6

3 6

[libvirt] [PATCH] Ensure async packets never get marked for sync replies
by Daniel P. Berrange 17 Aug '11

17 Aug '11

From: "Daniel P. Berrange" <berrange(a)redhat.com> If a client had initiated a stream abort, it will have a call waiting for a reply in the queue. If more data continues to arrive on the stream, the abort command could mistakenly get signalled as complete. Remove the code from async data processing that looked for waiting calls. Add a sanity check to ensure no async call can ever be marked as needing a reply * src/rpc/virnetclient.c: Ensure async data packets can't trigger a reply --- src/rpc/virnetclient.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index efaac88..055361d 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -627,11 +627,6 @@ static int virNetClientCallDispatchStream(virNetClientPtr client) case VIR_NET_CONTINUE: { if (virNetClientStreamQueuePacket(st, &client->msg) < 0) return -1; - - if (thecall && thecall->expectReply) { - VIR_DEBUG("Got sync data packet completion"); - thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE; - } return 0; } @@ -1193,6 +1188,13 @@ int virNetClientSend(virNetClientPtr client, virNetClientCallPtr call; int ret = -1; + if (expectReply && + (msg->header.status == VIR_NET_CONTINUE)) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempt to send an asynchronous message with a synchronous reply")); + return -1; + } + if (VIR_ALLOC(call) < 0) { virReportOOMError(); return -1; -- 1.7.6

2 2

[libvirt] [PATCH] Don't attempt to read from a stream if it is closed
by Daniel P. Berrange 17 Aug '11

17 Aug '11

From: "Daniel P. Berrange" <berrange(a)redhat.com> The I/O event callback processes incoming packets first, and then does outgoing packets. If the incoming packet caused the stream to close, then the attempt to process outgoing data resulted in an error. This caused libvirt to then send an error back to the client, but the stream had already been stopped. This confused the client since it sees 2 error events. * daemon/stream.c: Don't attempt read if stream is closed --- daemon/stream.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index 7dd9ae7..7d2b367 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -678,7 +678,15 @@ daemonStreamHandleRead(virNetServerClientPtr client, size_t bufferLen = VIR_NET_MESSAGE_PAYLOAD_MAX; int ret; - VIR_DEBUG("client=%p, stream=%p", client, stream); + VIR_DEBUG("client=%p, stream=%p tx=%d closed=%d", + client, stream, stream->tx, stream->closed); + + /* We might have had an event pending before we shut + * down the stream, so if we're marked as closed, + * then do nothing + */ + if (stream->closed) + return 0; /* Shouldn't ever be called unless we're marked able to * transmit, but doesn't hurt to check */ -- 1.7.6

2 1

[libvirt] [RFC] Host CPU passthrough
by Jiri Denemark 17 Aug '11

17 Aug '11

Hi, AFAIK this topic is not new but I think we still do not have a good solution for it. Libvirt already supports specifying what CPU and its features a guest should see but imagine one wants to run a guest on the best possible CPU. The current way is to copy the <cpu> element from capabilities XML into domain XML. This approach is fine since it provides reproducible environment and such domain can even be migrated to a different host. But the CPU shown provided to a guest is not the same as the real host CPU. It's based on a model which doesn't reflect all aspects of real CPUs. Ideally, we would model everything but that's quite complicated and may need updating anytime a new CPU is introduced. I think we should add a possibility to passthrough host CPU into a domain no matter what the CPU looks like. Domains configured in such a way will not be allowed to be migrated since we cannot guarantee the same CPU on destination host but they would be able to see the real CPU. Another benefit is that a single XML configuration would work on any CPU without modifications. Since possible issues with such domains will be harder to debug we can mark them as tainted just like we do if someone calls qemu monitor commands directly. I'm not sure what XML representation makes the most sense so I'll mention several options which I'm thinking about: - "host" ("native", "passthrough", whatever) special model; same name space as real CPU models, I don't like this too much: <cpu match='exact'> <model>host</model> </cpu> - "host" match policy; has nice first-look semantics that the guest CPU matches host's but doesn't play very nice with the defined semantics of "match" attribute: <cpu match='host'/> - new "mode" attribute; IMHO the best one, similar to what we have for SMBIOS: <cpu mode='host'/> vs. <cpu mode='custom' match='...'> <model>...</model. ... </cpu> What do you think? Jirka

3 6

[libvirt] [PATCH] snapshot: tweak misleading wording
by Eric Blake 16 Aug '11

16 Aug '11

Fixes confusion introduced in commit 98369d3. * tools/virsh.c (cmdSnapshotParent): Operates on named snapshot, not current. --- virsh.pod got it right. I'm pushing this under the trivial rule. tools/virsh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 51ba0a8..49917f3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12282,7 +12282,7 @@ cleanup: * "snapshot-parent" command */ static const vshCmdInfo info_snapshot_parent[] = { - {"help", N_("Get the name of the parent of the current snapshot")}, + {"help", N_("Get the name of the parent of a snapshot")}, {"desc", N_("Extract the snapshot's parent, if any")}, {NULL, NULL} }; -- 1.7.4.4

1 0

[libvirt] main: fix some compilation issues on non-linux platforms
by Stefan Berger 16 Aug '11

16 Aug '11

This patch fixes *some* compilation issues on non-Linux platforms (cygwin). Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com> diff --git a/src/util/interface.c b/src/util/interface.c index aec12f5..5d473b7 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -1317,7 +1317,7 @@ ifaceGetPhysicalFunction(const char *ifname, char **pfname) } #else int -ifaceIsVirtualFunction(const char *ifname) +ifaceIsVirtualFunction(const char *ifname ATTRIBUTE_UNUSED) { ifaceError(VIR_ERR_INTERNAL_ERROR, "%s", _("ifaceIsVirtualFunction is not supported on non-linux " @@ -1326,8 +1326,9 @@ ifaceIsVirtualFunction(const char *ifname) } int -ifaceGetVirtualFunctionIndex(const char *pfname, const char *vfname, - int *vf_index) +ifaceGetVirtualFunctionIndex(const char *pfname ATTRIBUTE_UNUSED, + const char *vfname ATTRIBUTE_UNUSED, + int *vf_index ATTRIBUTE_UNUSED) { ifaceError(VIR_ERR_INTERNAL_ERROR, "%s", _("ifaceGetVirtualFunctionIndex is not supported on non-linux " @@ -1336,7 +1337,8 @@ ifaceGetVirtualFunctionIndex(const char *pfname, const char *vfname, } int -ifaceGetPhysicalFunction(const char *ifname, char **pfname) +ifaceGetPhysicalFunction(const char *ifname ATTRIBUTE_UNUSED, + char **pfname ATTRIBUTE_UNUSED) { ifaceError(VIR_ERR_INTERNAL_ERROR, "%s", _("ifaceGetPhysicalFunction is not supported on non-linux " diff --git a/src/util/pci.c b/src/util/pci.c index 7040e55..9873f33 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1685,6 +1685,8 @@ int pciDeviceIsAssignable(pciDevice *dev, return 1; } +#ifdef __linux__ + /* * returns 1 if equal and 0 if not */ @@ -1804,7 +1806,6 @@ out: return ret; } -#ifdef __linux__ /* * Returns Physical function given a virtual function */ @@ -2010,8 +2011,8 @@ out: } #else int -pciGetPhysicalFunction(const char *vf_sysfs_path, - struct pci_config_address **physical_function) +pciGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED, + struct pci_config_address **physical_function ATTRIBUTE_UNUSED) { pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciGetPhysicalFunction is not " "supported on non-linux platforms")); @@ -2019,9 +2020,9 @@ pciGetPhysicalFunction(const char *vf_sysfs_path, } int -pciGetVirtualFunctions(const char *sysfs_path, - struct pci_config_address ***virtual_functions, - unsigned int *num_virtual_functions) +pciGetVirtualFunctions(const char *sysfs_path ATTRIBUTE_UNUSED, + struct pci_config_address ***virtual_functions ATTRIBUTE_UNUSED, + unsigned int *num_virtual_functions ATTRIBUTE_UNUSED) { pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciGetVirtualFunctions is not " "supported on non-linux platforms")); @@ -2029,7 +2030,7 @@ pciGetVirtualFunctions(const char *sysfs_path, } int -pciDeviceIsVirtualFunction(const char *vf_sysfs_device_link) +pciDeviceIsVirtualFunction(const char *vf_sysfs_device_link ATTRIBUTE_UNUSED) { pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciDeviceIsVirtualFunction is " "not supported on non-linux platforms")); @@ -2037,9 +2038,9 @@ pciDeviceIsVirtualFunction(const char *vf_sysfs_device_link) } int -pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link, - const char *vf_sysfs_device_link, - int *vf_index) +pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link ATTRIBUTE_UNUSED, + const char *vf_sysfs_device_link ATTRIBUTE_UNUSED, + int *vf_index ATTRIBUTE_UNUSED) { pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciGetVirtualFunctionIndex is " "not supported on non-linux platforms")); @@ -2048,7 +2049,8 @@ pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link, } int -pciDeviceNetName(char *device_link_sysfs_path, char **netname) +pciDeviceNetName(char *device_link_sysfs_path ATTRIBUTE_UNUSED, + char **netname ATTRIBUTE_UNUSED) { pciReportError(VIR_ERR_INTERNAL_ERROR, _("pciDeviceNetName is not " "supported on non-linux platforms")); diff --git a/src/util/virfile.c b/src/util/virfile.c index 0edf058..1158998 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -44,7 +44,7 @@ int virFileClose(int *fdptr, bool preserve_errno) { - int saved_errno; + int saved_errno = 0; int rc = 0; if (*fdptr >= 0) { @@ -62,7 +62,7 @@ int virFileClose(int *fdptr, bool preserve_errno) int virFileFclose(FILE **file, bool preserve_errno) { - int saved_errno; + int saved_errno = 0; int rc = 0; if (*file) { diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 7dd3c51..1927051 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -213,6 +213,8 @@ int virPidFileReadPathIfAlive(const char *path, #ifdef __linux__ if (virFileLinkPointsTo(procpath, binpath) == 0) *pid = -1; +#else + (void)binpath; #endif VIR_FREE(procpath);

2 8