June 2015 - Devel - libvirt List Archives

[libvirt] [PATCH] rpc: RH1026137: Fix slow volume download (virsh vol-download)
by Ossi Herrala 03 Aug '15

03 Aug '15

Use I/O vector (iovec) instead of one huge memory buffer as suggested in https://bugzilla.redhat.com/show_bug.cgi?id=1026137#c7. This avoids doing memmove() to big buffers and performance doesn't degrade if source (virNetClientStreamQueuePacket()) is faster than sink (virNetClientStreamRecvPacket()). --- src/rpc/virnetclientstream.c | 134 +++++++++++++++++++++++++---------------- 1 files changed, 82 insertions(+), 52 deletions(-) diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c index b428f4b..18c6e8b 100644 --- a/src/rpc/virnetclientstream.c +++ b/src/rpc/virnetclientstream.c @@ -49,9 +49,9 @@ struct _virNetClientStream { * time by stopping consuming any incoming data * off the socket.... */ - char *incoming; - size_t incomingOffset; - size_t incomingLength; + struct iovec *incomingVec; /* I/O Vector to hold data */ + size_t writeVec; /* Vectors produced */ + size_t readVec; /* Vectors consumed */ bool incomingEOF; virNetClientStreamEventCallback cb; @@ -86,9 +86,9 @@ virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st) if (!st->cb) return; - VIR_DEBUG("Check timer offset=%zu %d", st->incomingOffset, st->cbEvents); + VIR_DEBUG("Check timer readVec %zu writeVec %zu %d", st->readVec, st->writeVec, st->cbEvents); - if (((st->incomingOffset || st->incomingEOF) && + if ((((st->readVec < st->writeVec) || st->incomingEOF) && (st->cbEvents & VIR_STREAM_EVENT_READABLE)) || (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) { VIR_DEBUG("Enabling event timer"); @@ -110,13 +110,14 @@ virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) if (st->cb && (st->cbEvents & VIR_STREAM_EVENT_READABLE) && - (st->incomingOffset || st->incomingEOF)) + ((st->readVec < st->writeVec) || st->incomingEOF)) events |= VIR_STREAM_EVENT_READABLE; if (st->cb && (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) events |= VIR_STREAM_EVENT_WRITABLE; - VIR_DEBUG("Got Timer dispatch %d %d offset=%zu", events, st->cbEvents, st->incomingOffset); + VIR_DEBUG("Got Timer dispatch %d %d readVec %zu writeVec %zu", events, st->cbEvents, + st->readVec, st->writeVec); if (events) { virNetClientStreamEventCallback cb = st->cb; void *cbOpaque = st->cbOpaque; @@ -161,7 +162,7 @@ void virNetClientStreamDispose(void *obj) virNetClientStreamPtr st = obj; virResetError(&st->err); - VIR_FREE(st->incoming); + VIR_FREE(st->incomingVec); virObjectUnref(st->prog); } @@ -265,38 +266,49 @@ int virNetClientStreamQueuePacket(virNetClientStreamPtr st, virNetMessagePtr msg) { int ret = -1; - size_t need; + struct iovec iov; + char *base; + size_t piece, pieces, length, offset = 0, size = 1024*1024; virObjectLock(st); - need = msg->bufferLength - msg->bufferOffset; - if (need) { - size_t avail = st->incomingLength - st->incomingOffset; - if (need > avail) { - size_t extra = need - avail; - if (VIR_REALLOC_N(st->incoming, - st->incomingLength + extra) < 0) { - VIR_DEBUG("Out of memory handling stream data"); - goto cleanup; - } - st->incomingLength += extra; - } - memcpy(st->incoming + st->incomingOffset, - msg->buffer + msg->bufferOffset, - msg->bufferLength - msg->bufferOffset); - st->incomingOffset += (msg->bufferLength - msg->bufferOffset); - } else { + length = msg->bufferLength - msg->bufferOffset; + + if (length == 0) { st->incomingEOF = true; + goto end; } - VIR_DEBUG("Stream incoming data offset %zu length %zu EOF %d", - st->incomingOffset, st->incomingLength, - st->incomingEOF); + pieces = (length + size - 1) / size; + for (piece = 0; piece < pieces; piece++) { + if (size > length - offset) + size = length - offset; + + if (VIR_ALLOC_N(base, size)) { + VIR_DEBUG("Allocation failed"); + goto cleanup; + } + + memcpy(base, msg->buffer + msg->bufferOffset + offset, size); + iov.iov_base = base; + iov.iov_len = size; + offset += size; + + if (VIR_APPEND_ELEMENT(st->incomingVec, st->writeVec, iov) < 0) { + VIR_DEBUG("Append failed"); + VIR_FREE(base); + goto cleanup; + } + VIR_DEBUG("Wrote piece of vector. readVec %zu, writeVec %zu size %zu", st->readVec, st->writeVec, size); + } + + end: virNetClientStreamEventTimerUpdate(st); - ret = 0; cleanup: + VIR_DEBUG("Stream incoming data readVec %zu writeVec %zu EOF %d", + st->readVec, st->writeVec, st->incomingEOF); virObjectUnlock(st); return ret; } @@ -361,17 +373,21 @@ int virNetClientStreamRecvPacket(virNetClientStreamPtr st, size_t nbytes, bool nonblock) { - int rv = -1; + int ret = -1; + size_t partial, offset; + + virObjectLock(st); + VIR_DEBUG("st=%p client=%p data=%p nbytes=%zu nonblock=%d", st, client, data, nbytes, nonblock); - virObjectLock(st); - if (!st->incomingOffset && !st->incomingEOF) { + + if ((st->readVec >= st->writeVec) && !st->incomingEOF) { virNetMessagePtr msg; - int ret; + int rv; if (nonblock) { VIR_DEBUG("Non-blocking mode and no data available"); - rv = -2; + ret = -2; goto cleanup; } @@ -387,37 +403,51 @@ int virNetClientStreamRecvPacket(virNetClientStreamPtr st, VIR_DEBUG("Dummy packet to wait for stream data"); virObjectUnlock(st); - ret = virNetClientSendWithReplyStream(client, msg, st); + rv = virNetClientSendWithReplyStream(client, msg, st); virObjectLock(st); virNetMessageFree(msg); - if (ret < 0) + if (rv < 0) goto cleanup; } - VIR_DEBUG("After IO %zu", st->incomingOffset); - if (st->incomingOffset) { - int want = st->incomingOffset; - if (want > nbytes) - want = nbytes; - memcpy(data, st->incoming, want); - if (want < st->incomingOffset) { - memmove(st->incoming, st->incoming + want, st->incomingOffset - want); - st->incomingOffset -= want; + offset = 0; + partial = nbytes; + + while (st->incomingVec && (st->readVec < st->writeVec)) { + struct iovec *iov = st->incomingVec + st->readVec; + + if (!iov || !iov->iov_base) { + VIR_DEBUG("NULL pointer"); + goto cleanup; + } + + if (partial < iov->iov_len) { + memcpy(data+offset, iov->iov_base, partial); + memmove(iov->iov_base, (char*)iov->iov_base+partial, iov->iov_len-partial); + iov->iov_len -= partial; + offset += partial; + VIR_DEBUG("Consumed %zu, left %zu", partial, iov->iov_len); + break; } else { - VIR_FREE(st->incoming); - st->incomingOffset = st->incomingLength = 0; + memcpy(data+offset, iov->iov_base, iov->iov_len); + VIR_DEBUG("Consumed %zu. Moving to next piece", iov->iov_len); + partial -= iov->iov_len; + offset += iov->iov_len; + VIR_FREE(iov->iov_base); + iov->iov_len = 0; + st->readVec++; } - rv = want; - } else { - rv = 0; + + VIR_DEBUG("Read piece of vector. read %zu readVec %zu, writeVec %zu", offset, st->readVec, st->writeVec); } + ret = offset; virNetClientStreamEventTimerUpdate(st); cleanup: virObjectUnlock(st); - return rv; + return ret; } -- 1.7.1

2 3

[libvirt] [PATCH 1/2] storage: allow zero capacity with non-backing file to be created
by Chris J Arges 24 Jul '15

24 Jul '15

In commit fbcf7da95, a change was introduced that no longer allowed defining volumes via XML with a capacity of '0'. Because we check for info.size_arg to be non-zero, this use-case fails. This patch allows info.size_arg to be zero if no backing store is specified. Signed-off-by: Chris J Arges <chris.j.arges(a)canonical.com> --- src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ce59f63..c661662 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1068,7 +1068,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, if (info.inputPath) virCommandAddArg(cmd, info.inputPath); virCommandAddArg(cmd, info.path); - if (!info.inputPath && info.size_arg) + if (!info.inputPath && (info.size_arg || !info.backingPath)) virCommandAddArgFormat(cmd, "%lluK", info.size_arg); return cmd; -- 1.9.1

3 5

[libvirt] Continuation of Admin APIs
by Martin Kletzander 22 Jul '15

22 Jul '15

So now that we have the Admin API backend merged in, the code is prepared to have new APIs added in. There are many things to be added, but we should start out slowly with the most desired ones. I'll try to outline what I have in my mind so the discussion can sprout into existence. As Erik (Cc'd) expressed his interest in this, I'm just summarizing my ideas here, feel free to trash them if you feel like your idea is better. Apart from adding new APIs, we also need to split virsh to create virt-admin. I've hit a few bumps on that and Erik started looking at that either, but that's outside of the scope of this email. * What to start with ** Changing debugging settings This is something that makes sense and should be easy to do. I imagine something along the lines of virAdmSetDebug() being able to setup log_level, log_filters, log_outputs (and maybe log_buffer_size). The question is whether we want these to be three (or four) parameters (since that number in unlikely to change) or typed parameters. Even though I like the extensibility of typed parameters, I'd probably vote for the former. Mainly because this might become syntactic sugar to some heavier API (see below). There might be flags for persisting such settings, but that's highly debatable and might be also done later on. The only situation in which we might not want this API is if there is another one that is able to stream debug logs using virStream. There must be some "double-buffering" done for this since we would ran into bunch of problems. ** Reporting connection/client information This is a harder one to design since there is not primary key associated with each client. We'll need to come up with some, maybe combined one (based on more information, e.g. socket info, connection time). And if there's nothing better, then I guess associating a ID with each new connection (be it uint64 or UUID) will have to do. Then we have to come up with how to represent the client data so it's scalable. We might need to resort to something else than a structure, for binary extensibility. ** Forcibly disconnecting a client I haven't checked this thoroughly, but I can certainly see the problem where setting client->wantClose = true might just not be enough. If all non-workers are occupied with blocking API, disconnecting clients won't help with making the server accessible again. And that leads me to another one. ** Changing worker pool parameters In case the (maximum) number of workers needs to be changed, it should be possible by this API. However, there's yet again a question whether this is needed if we'll have a bigger gun for all daemon settings. That's described in the next section. * Future ideas (for consideration) ** Changing any settings for the libvirt daemon. I had an idea that we should have a "universal" API for changing any settings that comes from config files. The API that would implement this would have a string parameter (or list of strings) that would say what setting(s) to change, similarly to augtool. So let's say instead of: $ augtool -s set /files/etc/libvirt/libvirtd.conf/max_workers 10 one would call: virAdmSetConfig("libvirtd.conf/max_workers", "10", ...); Good thing about this is that it is highly scalable (even to qemu.conf). Bad thing is that we need to be able to say whether there is a setting that we don't yet support (return error_unsupported) and also we'd need to rework the configuration module to be able to do such thing and have a function to call for each change. Having this would render changing max_workers, log_level etc. obsolete (although I feel like changing debug parameters still deserves its own API function). ** Reloading the server's TLS certificates? This was requested by Lee (Cc'd) in a reply to one of the PoC series. It is something that I feel like we really want as well, but I haven't yet put much thought into it. That's all from me now. I won't have much access to my mail in couple of following days, so I'll be slower to respond, even slower than my usual slow. Have a nice day, Martin

2 2

[libvirt] [PATCH 0/5] Additional SCSI generated device address checks
by John Ferlan 21 Jul '15

21 Jul '15

https://bugzilla.redhat.com/show_bug.cgi?id=1210587 These patches will resolve a couple issues with generation of the <address type='drive' .../> for a SCSI <disk> and <hostdev>. The <disk> generation algorithm 'assumes' that when presented with <target dev='sda'.../> that it can use controller=0 and unit=0 since sda would conceivably be the first device; however, a <hostdev> could attempt to assign itself to that address and it doesn't have a target device name, so it bypasses the virDomainDiskDefDstDuplicates checks that would normally 'catch' two <disk>'s attempting to use the same name. Likewise, if a <hostdev> occupies an <address> and we attempt to hotplug a <disk> without providing an address, the address generation could attempt to place the disk on the already existing host device. John Ferlan (5): conf: Enforce SCSI hostdev address type conf: Add 'bus' and 'target' to SCSI address conflict checks conf: Add SCSI hostdev check for disk drive address already in use conf: Refactor virDomainDiskDefParseXML to pass vmdef conf: Check for hostdev conflicts when assign default disk address docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 115 ++++++++++++++++++++++++++++++++-------------- src/conf/domain_conf.h | 3 +- src/qemu/qemu_command.c | 4 +- src/vmx/vmx.c | 22 +++++---- src/vmx/vmx.h | 3 +- 6 files changed, 101 insertions(+), 50 deletions(-) -- 2.1.0

3 19

[libvirt] [PATCH] nodedev: add RDMA and tx-udp_tnl-segmentation NIC capabilities
by Moshe Levi 19 Jul '15

19 Jul '15

Adding functionality to libvirt that will allow it query the interface for the availability of RDMA and tx-udp_tnl-segmentation Offloading NIC capabilities Here is an example of the feature XML definition: <device> <name>net_eth4_90_e2_ba_5e_a5_45</name> <path>/sys/devices/pci0000:00/0000:00:03.0/0000:08:00.1/net/eth4</path> <parent>pci_0000_08_00_1</parent> <capability type='net'> <interface>eth4</interface> <address>90:e2:ba:5e:a5:45</address> <link speed='10000' state='up'/> <feature name='rx'/> <feature name='tx'/> <feature name='sg'/> <feature name='tso'/> <feature name='gso'/> <feature name='gro'/> <feature name='rxvlan'/> <feature name='txvlan'/> <feature name='rxhash'/> <feature name='rdma'/> <feature name='tx-udp_tnl-segmentation'/> <capability type='80203'/> </capability> </device> --- docs/formatnode.html.in | 2 + src/conf/device_conf.c | 4 +- src/conf/device_conf.h | 2 + src/util/virnetdev.c | 97 ++++++++++++++++++++++++++++++++++++++-------- src/util/virnetdev.h | 1 + 5 files changed, 88 insertions(+), 18 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 3ff1bef..9b32dd1 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -199,6 +199,8 @@ <dt><code>txvlan</code></dt><dd>tx-vlan-offload</dd> <dt><code>ntuple</code></dt><dd>ntuple-filters</dd> <dt><code>rxhash</code></dt><dd>receive-hashing</dd> + <dt><code>rdma</code></dt><dd>remote-direct-memory-access</dd> + <dt><code>tx-udp_tnl-segmentation</code></dt><dd>tx-udp-tunnel-segmentation</dd> </dl> </dd> <dt><code>capability</code></dt> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 98808e2..8e8d557 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -51,7 +51,9 @@ VIR_ENUM_IMPL(virNetDevFeature, "rxvlan", "txvlan", "ntuple", - "rxhash") + "rxhash", + "rdma", + "tx-udp_tnl-segmentation") int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..07298c9 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -74,6 +74,8 @@ typedef enum { VIR_NET_DEV_FEAT_TXVLAN, VIR_NET_DEV_FEAT_NTUPLE, VIR_NET_DEV_FEAT_RXHASH, + VIR_NET_DEV_FEAT_RDMA, + VIR_NET_DEV_FEAT_TX_UDP_TNL_SEGMENTATION, VIR_NET_DEV_FEAT_LAST } virNetDevFeature; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e4fcd81..3086616 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -87,6 +87,14 @@ VIR_LOG_INIT("util.netdev"); # define VIR_IFF_ALLMULTI 0 #endif +#define RESOURCE_FILE_LEN 4096 +#define TX_UDP_TNL 25 +#define GFEATURES_SIZE 2 +#define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field) +#define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U) +#define FEATURE_BIT_IS_SET(blocks, index, field) \ + (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index)) + typedef enum { VIR_MCAST_TYPE_INDEX_TOKEN, VIR_MCAST_TYPE_NAME_TOKEN, @@ -2943,6 +2951,58 @@ int virNetDevGetRxFilter(const char *ifname, return ret; } + +/** + * virNetDevRDMAFeature + * This function checks for the availability of RDMA feature + * and add it to bitmap + * + * @ifname: name of the interface + * @out: add RDMA feature if exist to bitmap + * + * Returns 0 on success, -1 on failure. + */ +static int +virNetDevRDMAFeature(const char *ifname, + virBitmapPtr *out) +{ + char *eth_devpath = NULL; + char *ib_devpath = NULL; + char *eth_res_buf = NULL; + char *ib_res_buf = NULL; + struct dirent *dp; + + DIR *dirp = opendir(SYSFS_INFINIBAND_DIR); + if (dirp == NULL) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + SYSFS_INFINIBAND_DIR); + return -1; + } + + if (virAsprintf(&eth_devpath, SYSFS_NET_DIR "%s/device/resource", ifname) < 0) + return -1; + if (!virFileExists(eth_devpath)) + return 0; + if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, &eth_res_buf) < 0) + return -1; + while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { + if (STREQ(dp->d_name, ".") || + STREQ(dp->d_name, "..")) + continue; + + if (virAsprintf(&ib_devpath, SYSFS_INFINIBAND_DIR "%s/device/resource", dp->d_name) < 0) + continue; + if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, &ib_res_buf) < 0) + continue; + if (STREQ(eth_res_buf, ib_res_buf)) { + ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); + break; + } + } + return 0; +} + #if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ) /** @@ -2952,12 +3012,10 @@ int virNetDevGetRxFilter(const char *ifname, * @ifname: name of the interface * @cmd: reference to an ethtool command structure * - * Returns 0 on success, -1 on failure. */ -static int -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) +static void +virNetDevFeatureAvailable(const char *ifname, void *cmd) { - int ret = -1; int sock = -1; virIfreq ifr; @@ -2969,8 +3027,7 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) memset(&ifr, 0, sizeof(ifr)); strcpy(ifr.ifr_name, ifname); - ifr.ifr_data = (void*) cmd; - + ifr.ifr_data = cmd; if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) { switch (errno) { case EPERM: @@ -2988,12 +3045,9 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) } } - ret = cmd->data > 0 ? 1: 0; cleanup: if (sock) VIR_FORCE_CLOSE(sock); - - return ret; } @@ -3013,7 +3067,7 @@ virNetDevGetFeatures(const char *ifname, { size_t i = -1; struct ethtool_value cmd = { 0 }; - + struct ethtool_gfeatures g_cmd = { 0 }; struct elem{ const int cmd; const virNetDevFeature feat; @@ -3037,7 +3091,8 @@ virNetDevGetFeatures(const char *ifname, for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { cmd.cmd = cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd)) + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0) ignore_value(virBitmapSetBit(*out, cmds[i].feat)); } @@ -3061,14 +3116,22 @@ virNetDevGetFeatures(const char *ifname, }; cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd)) { - for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { - if (cmd.data & flags[j].cmd) - ignore_value(virBitmapSetBit(*out, flags[j].feat)); + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0) { + for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { + if (cmd.data & flags[j].cmd) + ignore_value(virBitmapSetBit(*out, flags[j].feat)); + } } - } -# endif + g_cmd.cmd = ETHTOOL_GFEATURES; + g_cmd.size = GFEATURES_SIZE; + virNetDevFeatureAvailable(ifname, &g_cmd); + if FEATURE_BIT_IS_SET(g_cmd.features, TX_UDP_TNL, active) + ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TX_UDP_TNL_SEGMENTATION)); +# endif + if (virNetDevRDMAFeature(ifname, out)) + return -1; return 0; } #else diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 190b70e..fff881c 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -210,6 +210,7 @@ int virNetDevGetRcvAllMulti(const char *ifname, bool *receive) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; # define SYSFS_NET_DIR "/sys/class/net/" +# define SYSFS_INFINIBAND_DIR "/sys/class/infiniband/" int virNetDevSysfsFile(char **pf_sysfs_device_link, const char *ifname, const char *file) -- 1.7.1

2 2

[libvirt] GITHUB readonly mirror site
by Daniel P. Berrange 17 Jul '15

17 Jul '15

FYI, further to the previous discussion I have created a libvirt organization on github and populated the repositories we have. They are setup in read-only mode and are synced from main libvirt repo automagically. https://github.com/libvirtproject Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

6 13

[libvirt] [PATCH 0/6] storage: fs: tweak dir perms handling on build
by Cole Robinson 17 Jul '15

17 Jul '15

Consider the following issue - Using virt-manager with qemu:///session - User adds a storage pool pointing at /tmp. No explicit permissions are requested in the XML - virt-manager calls PoolDefine, then PoolBuild - libvirt tries to unconditionally chmod 755 /tmp. This fails because my user doesn't own root. Pool build fails, virt-manager reports failure Yes there's a couple ways we could avoid this specific case in virt-manager, but I think it makes more sense to have pool.build on a directory be a no-op in this case. The following patches address this. - Patch 1 is an error reporting tweak - Patch 2 is a feature, but implementing it simplifies later patches - Patch 3 makes pool.build not even attempt mkdir if the dir already exists. - Patch 4 makes pool.build skip dir chown'ing unless user explicitly requested uid or gid via the XML - Patch 5-6 make pool.build skip dir chmod unless the user explicitly requested <mode> via the XML. If a mode is required for mkdir, continue to use the previous default. Cole Robinson (6): storage: fs: Don't overwrite virDirCreate error storage: fs: Fill in permissions on pool refresh storage: fs: Don't attempt directory creation if it already exists storage: fs: Don't try to chown directory unless user requested storage: conf: Don't set any default <mode> in the XML storage: fs: Only force directory permissions if required docs/schemas/storagecommon.rng | 5 +- src/conf/storage_conf.c | 42 +++++------ src/storage/storage_backend.c | 20 ++++-- src/storage/storage_backend.h | 3 + src/storage/storage_backend_fs.c | 81 ++++++++++++++++------ src/storage/storage_backend_logical.c | 4 +- src/util/virfile.c | 47 ++++++++----- tests/storagepoolxml2xmlin/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 +- tests/storagevolxml2xmlin/vol-file.xml | 6 +- tests/storagevolxml2xmlout/vol-file.xml | 6 +- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 +- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 +- 14 files changed, 147 insertions(+), 77 deletions(-) -- 2.3.6

4 21

[libvirt] [PATCH v2 0/3] driver level connection close event
by nshirokovskiy＠virtuozzo.com 16 Jul '15

16 Jul '15

Notify of connection close event from parallels driver (possibly) wrapped in the remote driver. Changes from v1: 1. fix comment style issues 2. remove spurious whitespaces 3. move rpc related part from vz patch to second(rpc) patch 4. remove unnecessary locks for immutable closeCallback in first patch. Discussion. In 1 and 2 patch we forced to some decisions because we don't have a weak reference mechanics. 1 patch. ----------- virConnectCloseCallback is introduced because we can not reference the connection object itself when setting a network layer callback because of how connection close works. A connection close procedure is next: 1. client closes connection 2. a this point nobody else referencing a connection and it is disposed 3. connection dispose unreferencing network connection 4. network connection disposes Thus if we referece a connection in network close callback we never get step 2. virConnectCloseCallback broke this cycle but at cost that clients MUST unregister explicitly before closing connection. This is not good as this unregistration is not really neaded. Client is not telling that it does not want to receive events anymore but rather forced to obey some implementation-driven rules. 2 patch. ----------- We impose requirements on driver implementations which is fragile. Moreover we again need to make explicit unregistrations. Implementation of domain events illustrates this point. remoteDispatchConnectDomainEventRegister does not reference NetClient and makes unregistration before NetClient is disposed but drivers do not meet the formulated requirements. Object event system release lock before delivering event for re-entrance purposes. Shortly we have 2 undesired consequences here. 1. Mandatory unregistration. 2. Imposing multi-threading requirements. Introduction of weak pointers could free us from these artifacts. Next weak reference workflow illustrates this. 1. Take weak reference on object of interest before passing to party. This doesn't break disposing mechanics as weak eference does not prevent from disposing object. Object is disposed but memory is not freed yet if there are weak references. 2. When callback is called we are safe to check if pointer dangling as we make a weak reference before. 3. Release weak reference and this trigger memory freeing if there are no more weak references. daemon/libvirtd.h | 1 + daemon/remote.c | 86 +++++++++++++++++++++++++++++++ src/datatypes.c | 115 +++++++++++++++++++++++++++++++---------- src/datatypes.h | 21 ++++++-- src/driver-hypervisor.h | 12 ++++ src/libvirt-host.c | 77 +++++++++------------------- src/remote/remote_driver.c | 106 +++++++++++++++++++++++++++++--------- src/remote/remote_protocol.x | 24 ++++++++- src/remote_protocol-structs | 6 ++ src/vz/vz_driver.c | 26 +++++++++ src/vz/vz_sdk.c | 29 +++++++++++ src/vz/vz_utils.h | 3 +

2 4

[libvirt] [PATCH] libxl: rework reference counting
by Jim Fehlig 14 Jul '15

14 Jul '15

Similar to commit 540c339a for the QEMU driver, rework reference counting in the libxl driver to make it more deterministic and the code a bit cleaner. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com> --- I've been testing this patch on and off for a few weeks now using libvirt-tck domain tests, local test scripts, and some manual tests for good measure. I sent the patch to Anthony Perard (cc'd) nearly two weeks ago for testing in his OpenStack+Xen+libvirt CI loop, although I haven't received any feedback thus far. Also included Martin in the cc since he encouraged this patch https://www.redhat.com/archives/libvir-list/2015-April/msg00014.html src/libxl/libxl_domain.c | 32 ++---- src/libxl/libxl_domain.h | 5 +- src/libxl/libxl_driver.c | 274 ++++++++++++++++++-------------------------- src/libxl/libxl_migration.c | 6 +- 4 files changed, 128 insertions(+), 189 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0652270..ce188ee 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -96,13 +96,12 @@ libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv) #define LIBXL_JOB_WAIT_TIME (1000ull * 30) /* - * obj must be locked before calling, libxlDriverPrivatePtr must NOT be locked + * obj must be locked before calling * * This must be called by anything that will change the VM state - * in any way + * in any way. * - * Upon successful return, the object will have its ref count increased, - * successful calls must be followed by EndJob eventually + * Successful calls must eventually result in a call to EndJob. */ int libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, @@ -117,8 +116,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, return -1; then = now + LIBXL_JOB_WAIT_TIME; - virObjectRef(obj); - while (priv->job.active) { VIR_DEBUG("Wait normal job condition for starting job: %s", libxlDomainJobTypeToString(job)); @@ -149,21 +146,16 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, virReportSystemError(errno, "%s", _("cannot acquire job mutex")); - virObjectUnref(obj); return -1; } /* - * obj must be locked before calling + * obj must be locked and have a reference before calling * * To be called after completing the work associated with the * earlier libxlDomainBeginJob() call - * - * Returns true if the remaining reference count on obj is - * non-zero, false if the reference count has dropped to zero - * and obj is disposed. */ -bool +void libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, virDomainObjPtr obj) { @@ -175,8 +167,6 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, libxlDomainObjResetJob(priv); virCondSignal(&priv->job.cond); - - return virObjectUnref(obj); } static void * @@ -485,12 +475,11 @@ libxlDomainShutdownThread(void *opaque) } endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); + virObjectUnref(vm); if (dom_event) libxlDomainEventQueue(driver, dom_event); libxl_event_free(cfg->ctx, ev); @@ -528,6 +517,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) VIR_INFO("Received event for unknown domain ID %d", event->domid); goto error; } + virObjectRef(vm); /* * Start a thread to handle shutdown. We don't want to be tying up @@ -558,8 +548,10 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event) /* Cast away any const */ libxl_event_free(cfg->ctx, (libxl_event *)event); virObjectUnref(cfg); - if (vm) + if (vm) { virObjectUnlock(vm); + virObjectUnref(vm); + } VIR_FREE(shutdown_info); } diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 8c73cc4..714ed91 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -83,10 +83,9 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver, enum libxlDomainJob job) ATTRIBUTE_RETURN_CHECK; -bool +void libxlDomainObjEndJob(libxlDriverPrivatePtr driver, - virDomainObjPtr obj) - ATTRIBUTE_RETURN_CHECK; + virDomainObjPtr obj); void libxlDomainEventQueue(libxlDriverPrivatePtr driver, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a7be745..c0061b3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -282,7 +282,7 @@ libxlDomObjFromDomain(virDomainPtr dom) libxlDriverPrivatePtr driver = dom->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); + vm = virDomainObjListFindByUUIDRef(driver->domains, dom->uuid); if (!vm) { virUUIDFormat(dom->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -294,6 +294,25 @@ libxlDomObjFromDomain(virDomainPtr dom) return vm; } +/* + * Finish working with a domain object in an API. This function + * clears whatever was left of a domain that was gathered using + * libxlDomObjFromDomain(). Currently that means only unlocking and + * decrementing the reference counter of that domain. And in order to + * make sure the caller does not access the domain, the pointer is + * cleared. + */ +static void +libxlDomObjEndAPI(virDomainObjPtr *vm) +{ + if (!*vm) + return; + + virObjectUnlock(*vm); + virObjectUnref(*vm); + *vm = NULL; +} + static int libxlAutostartDomain(virDomainObjPtr vm, void *opaque) @@ -303,6 +322,7 @@ libxlAutostartDomain(virDomainObjPtr vm, int ret = -1; virObjectLock(vm); + virObjectRef(vm); virResetLastError(); if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { @@ -322,8 +342,10 @@ libxlAutostartDomain(virDomainObjPtr vm, ret = 0; endjob: - if (libxlDomainObjEndJob(driver, vm)) - virObjectUnlock(vm); + libxlDomainObjEndJob(driver, vm); + + virObjectUnlock(vm); + virObjectUnref(vm); return ret; } @@ -908,19 +930,19 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; + virObjectRef(vm); def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } goto cleanup; } if (libxlDomainStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1) < 0) { - virDomainObjListRemove(driver->domains, vm); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm); goto endjob; } @@ -929,13 +951,11 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, dom->id = vm->def->id; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return dom; } @@ -1060,12 +1080,10 @@ libxlDomainSuspend(virDomainPtr dom) ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -1117,12 +1135,10 @@ libxlDomainResume(virDomainPtr dom) ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -1183,8 +1199,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1232,8 +1247,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1281,12 +1295,10 @@ libxlDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -1315,8 +1327,7 @@ libxlDomainGetOSType(virDomainPtr dom) goto cleanup; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return type; } @@ -1335,8 +1346,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom) ret = virDomainDefGetMemoryActual(vm->def); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -1455,12 +1465,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1513,8 +1521,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1540,8 +1547,7 @@ libxlDomainGetState(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -1638,7 +1644,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - bool remove_dom = false; #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME virReportUnsupportedError(); @@ -1670,21 +1675,15 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, goto endjob; if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -1735,10 +1734,8 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } goto cleanup; } @@ -1746,15 +1743,13 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, if (ret < 0 && !vm->persistent) virDomainObjListRemove(driver->domains, vm); - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: if (VIR_CLOSE(fd) < 0) virReportSystemError(errno, "%s", _("cannot close file")); virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1772,7 +1767,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virObjectEventPtr event = NULL; - bool remove_dom = false; bool paused = false; int ret = -1; @@ -1828,7 +1822,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); } ret = 0; @@ -1846,16 +1840,10 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) } endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -1869,7 +1857,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm = NULL; char *name = NULL; int ret = -1; - bool remove_dom = false; virCheckFlags(0, -1); @@ -1902,21 +1889,15 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) goto endjob; if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); VIR_FREE(name); return ret; } @@ -1960,8 +1941,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = vm->hasManagedSave; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -1990,8 +1970,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -2119,13 +2098,11 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, ret = virDomainSaveConfig(cfg->configDir, def); endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: VIR_FREE(bitmask); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2187,8 +2164,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) ret = (flags & VIR_DOMAIN_VCPU_MAXIMUM) ? def->maxvcpus : def->vcpus; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -2270,12 +2246,10 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, } endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virBitmapFree(pcpumap); virObjectUnref(cfg); return ret; @@ -2354,8 +2328,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, cleanup: virBitmapFree(allcpumap); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2418,8 +2391,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, ret = maxinfo; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2442,8 +2414,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefFormatConvertXMLFlags(flags)); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -2618,12 +2589,10 @@ libxlDomainCreateWithFlags(virDomainPtr dom, dom->id = vm->def->id; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -2760,8 +2729,7 @@ libxlDomainUndefineFlags(virDomainPtr dom, cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -3618,14 +3586,12 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, } endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -3726,14 +3692,12 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, } endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -3833,8 +3797,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -3964,8 +3927,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -4029,14 +3991,12 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart) ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4091,8 +4051,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) ignore_value(VIR_STRDUP(ret, name)); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4157,8 +4116,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4241,12 +4199,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, ret = 0; endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4316,8 +4272,7 @@ libxlDomainOpenConsole(virDomainPtr dom, } cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -4452,8 +4407,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom, VIR_FREE(nodeset); virBitmapFree(nodes); libxl_bitmap_dispose(&nodemap); - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4474,8 +4428,7 @@ libxlDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + libxlDomObjEndAPI(&obj); return ret; } @@ -4494,8 +4447,7 @@ libxlDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + libxlDomObjEndAPI(&obj); return ret; } @@ -4514,8 +4466,7 @@ libxlDomainIsUpdated(virDomainPtr dom) ret = vm->updated; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -4780,6 +4731,7 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain, { const char *xmlin = NULL; virDomainObjPtr vm = NULL; + char *ret = NULL; #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME virReportUnsupportedError(); @@ -4798,19 +4750,20 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain, if (!(vm = libxlDomObjFromDomain(domain))) return NULL; - if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); - return NULL; - } + if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - virObjectUnlock(vm); - return NULL; + goto cleanup; } - return libxlDomainMigrationBegin(domain->conn, vm, xmlin); + ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin); + + cleanup: + libxlDomObjEndAPI(&vm); + return ret; } static int @@ -4919,8 +4872,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + libxlDomObjEndAPI(&vm); return ret; } @@ -4963,23 +4915,20 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn, NULLSTR(dname)); return NULL; } + virObjectRef(vm); - if (virDomainMigrateFinish3ParamsEnsureACL(dconn, vm->def) < 0) { - virDomainObjEndAPI(&vm); - return NULL; - } + if (virDomainMigrateFinish3ParamsEnsureACL(dconn, vm->def) < 0) + goto cleanup; - if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - virDomainObjEndAPI(&vm); - return NULL; - } + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; ret = libxlDomainMigrationFinish(dconn, vm, flags, cancelled); - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); - virDomainObjEndAPI(&vm); + cleanup: + libxlDomObjEndAPI(&vm); return ret; } @@ -4995,6 +4944,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain, { libxlDriverPrivatePtr driver = domain->conn->privateData; virDomainObjPtr vm = NULL; + int ret = -1; #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME virReportUnsupportedError(); @@ -5008,12 +4958,14 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain, if (!(vm = libxlDomObjFromDomain(domain))) return -1; - if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); - return -1; - } + if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; - return libxlDomainMigrationConfirm(driver, vm, flags, cancelled); + ret = libxlDomainMigrationConfirm(driver, vm, flags, cancelled); + + cleanup: + libxlDomObjEndAPI(&vm); + return ret; } static int libxlNodeGetSecurityModel(virConnectPtr conn, diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 39e4a65..665b21a 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -257,13 +257,9 @@ libxlDomainMigrationBegin(virConnectPtr conn, xml = virDomainDefFormat(def, VIR_DOMAIN_DEF_FORMAT_SECURE); endjob: - if (!libxlDomainObjEndJob(driver, vm)) - vm = NULL; + libxlDomainObjEndJob(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); - virDomainDefFree(tmpdef); virObjectUnref(cfg); return xml; -- 2.3.7

3 4

[libvirt] [PATCH v3] nodeinfo: fix to parse present cpus rather than possible cpus
by Kothapally Madhu Pavan 13 Jul '15

13 Jul '15

Currently we are parsing all the possible cpus to get the nodeinfo. This fix will perform a check for present cpus before parsing. Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com> --- src/nodeinfo.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2fafe2d..5689c9b 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -43,6 +43,7 @@ #include "c-ctype.h" #include "viralloc.h" #include "nodeinfopriv.h" +#include "nodeinfo.h" #include "physmem.h" #include "virerror.h" #include "count-one-bits.h" @@ -418,6 +419,7 @@ virNodeParseNode(const char *node, int processors = 0; DIR *cpudir = NULL; struct dirent *cpudirent = NULL; + virBitmapPtr present_cpumap = NULL; int sock_max = 0; cpu_set_t sock_map; int sock; @@ -438,12 +440,17 @@ virNodeParseNode(const char *node, goto cleanup; } + present_cpumap = nodeGetPresentCPUBitmap(); + /* enumerate sockets in the node */ CPU_ZERO(&sock_map); while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; + if (present_cpumap && !(virBitmapIsSet(present_cpumap, cpu))) + continue; + if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0) goto cleanup; @@ -477,6 +484,9 @@ virNodeParseNode(const char *node, if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; + if (present_cpumap && !(virBitmapIsSet(present_cpumap, cpu))) + continue; + if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0) goto cleanup; @@ -537,6 +547,7 @@ virNodeParseNode(const char *node, ret = -1; } VIR_FREE(core_maps); + virBitmapFree(present_cpumap); return ret; }

2 2