Re: [libvirt] [PATCH 0/7] Update systemtap probing
Tested by:Royce Lvlvro...@linux.vnet.ibm.com I'm not sure whether I encountered are env/config related problem or bugs,so I submit my installation process here for you to reference. 1.env description: host os:Ubuntu 11.04(kernel 2.6.38-8-generic,x86_64) libvirt:0.9.6 qemu:0.15.50 dtrace:sun D 1.6 systemtap:version 1.3/0.148 non-git sources 2.install systemtaped libvirt (1)configure ./configure --prefix=/usr --with-dtrace (2)make ---error1: dtrace: failed to compile script probes.d: line 2: invalid control directive: #file: solution: delete lines begin with # --error2: Invoking: ld -o probes.o -r probes.tmp.o /usr/lib/dtrace/drti.o ld: i386 architecture of input file `probes.tmp.o' is incompatible with i386:x86-64 output dtrace: failed to link script probes.d: failed to link probes.o: ld exited with status 1 so the script incompatible for 64 system,is it a bug? solution: change DTRACE=/usr/bin/dtrace to DTRACE=/usr/bin/dtrace -64 --error3:(with ./configure --prefix=/usr --enable-dtrace) *** objects probes.o is not portable! /usr/bin/ld: probes.o: relocation R_X86_64_32 against `.rodata' can not be used when making a shared object; recompile with -fPIC probes.o: could not read symbols: Bad value tried: (1)reconfig with ./configure --with-dtrace --disable-share result: compile success,but libvirt_probes.stp size is 0,stp script can't be used (2)add flags with -fPIC result: nothing changed,still the same error (3)tried configure --with-pic result: nothing changed,still the same error 3.Here is my question: (1)Is the system-enabled libvirt support 64bit system? (2)Do you have suggestion about error3? Sorry to bother you so many times,But I do think applying these patches are important and useful for future debugging.Thank you for your time! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] Extend RPC protocol to allow FD passing
On Thu, Oct 27, 2011 at 05:15:40PM -0600, Eric Blake wrote:
On 10/25/2011 10:03 AM, Daniel P. Berrange wrote:
From: Daniel P. Berrangeberra...@redhat.com
Define two new RPC message types VIR_NET_CALL_WITH_FDS and
VIR_NET_REPLY_WITH_FDS. These message types are equivalent
to VIR_NET_CALL and VIR_NET_REPLY, except that between the
message header, and payload there is a 32-bit integer field
specifying how many file descriptors have been passed.
The actual file descriptors are sent/recv'd out of band.
* src/rpc/virnetmessage.c, src/rpc/virnetmessage.h,
src/libvirt_private.syms: Add support for handling
passed file descriptors
* src/rpc/virnetprotocol.x: Extend protocol for FD
passing
---
docs/internals/rpc.html.in | 33 +
Thanks; this fixes my biggest concern from the last round.
p
+ With the two packet types that support passing file descriptors, in
+ between the header and the payload there will be a 4-byte integer
+ specifying the number of file descriptors which are being sent.
+ The actual file handles are sent after the payload has been sent.
+ Each file handle has a single dummy byte transmitted as a carrier
+ for the out of band file descriptor.
gnulib recvfd() ignores this byte value. While sendfd() sends NUL,
it is conceivable that a 3rd-party implementation of the wire
protocol might pick a different value (or that gnulib might change
in the future). Should we add any documentation along the lines of
'sender should send 0' and 'receiver must ignore the dummy byte,
even if it is not 0', just for robustness sake?
+
+
+int virNetMessageDupFD(virNetMessagePtr msg,
+ size_t slot)
+{
+int fd;
+
+if (slot= msg-nfds) {
+virNetError(VIR_ERR_INTERNAL_ERROR,
+_(No FD available at slot %zu), slot);
+return -1;
+}
+
+if ((fd = dup(msg-fds[slot])) 0) {
+virReportSystemError(errno,
+ _(Unable to duplicate FD %d),
+ msg-fds[slot]);
+return -1;
+}
Do we want to be using gnulib's dup_cloexec (cloexec.h) or
fcntl(fd, F_DUPFD_CLOEXEC, 0) here, so that our dup doesn't leak
into a third-party child if we are linked into some larger
multithreaded app?
dup3() is the new glibc API for this ? Does gnulib support
that yet ?
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] Update systemtap probing
On Fri, Oct 28, 2011 at 04:29:47PM +0800, lvroyce wrote: Tested by:Royce Lvlvro...@linux.vnet.ibm.com I'm not sure whether I encountered are env/config related problem or bugs,so I submit my installation process here for you to reference. 1.env description: host os:Ubuntu 11.04(kernel 2.6.38-8-generic,x86_64) libvirt:0.9.6 qemu:0.15.50 dtrace:sun D 1.6 Huh ? Where do you get this Sun dtrace binary from ? The dtrace binary should be the one provided by *systemtap*. systemtap:version 1.3/0.148 non-git sources 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/8 v2] Summary on block IO throttle
On 10/27/2011 11:45 PM, Eric Blake wrote: On 10/27/2011 03:12 AM, Lei Li wrote: 1) Enable the blkio throttling in xml when guest is starting up. Add blkio throttling in xml as follows: disk type='file' device='disk' driver name='qemu' type='raw'/ source file='/var/lib/libvirt/images/kvm-one.img'/ target dev='vda' bus='virtio'/ address type='pci' domain='0x' bus='0x00' slot='0x05' function='0x0'/ iotune bps='n'.../ /disk 2) Enable blkio throttling setting at guest running time. virsh blkiothrottledomain device [--bpsnumber] [--bps_rdnumber] \ [--bps_wrnumber] [--iopsnumber] [--iops_rdnumber] [--iops_wrnumber] 3) The support to get the current block i/o throttling for a device - HMP/QMP. virsh blkiothrottledomain device Given that the XML is named iotune under disk, we should probably name the virsh command 'blkiotune' or 'disk-iotune', not 'blkiothrottle'. Hi Eric, we usediothrottle first, I changed it since Daniel P. Berrange proposediotune for per-disk element instead ofiothrottle when we discussed at RFC V1. The command 'blkiotune' already exist, supported the cgroups blkio-controller, which handles proportional shares and throughput/iops limits on host block devices, global to the domain, but blkio throttling is specified per-disk and can vary across multiple disks. They are different two mechanism. So how about useiothrottle again? :) Also, iotune bps='n'.../ doesn't look right. This should be similar to the top-level XML. Here, taking into account the other proposal for per-block-device values (which can't be tied to individual disk): domain... blkiotune weight800/weight device path/path/to/device/path weight200/weight /device /blkiotune devices disk ... iotune bps800/bps ... /iotune /disk /devices /domain OK. I will do it at v3. daemon/remote.c | 85 + include/libvirt/libvirt.h.in| 25 + Missing changes in docs/formatdomain.html.in (to describe the new XML), docs/schemas/domaincommon.rng (to validate the new xml), and tests/ (probably qemuxml2argvdata), to test it. -- Lei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/8] Enable the blkiothrottle command in virsh
On 10/28/2011 06:02 AM, Adam Litke wrote:
On Thu, Oct 27, 2011 at 05:20:09PM +0800, Lei HH Li wrote:
Signed-off-by: Zhi Yong Wuwu...@linux.vnet.ibm.com
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
tools/virsh.c | 99 +++
tools/virsh.pod | 13 +++
2 files changed, 112 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 72344f0..de86c40 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -6023,6 +6023,104 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
return true;
}
+/*
+ * blkiothrottle command
+ */
+static const vshCmdInfo info_blkiothrottle[] = {
+{help, N_(Set or display a block disk I/O throttle setting.)},
+{desc, N_(Set or display a block disk I/O throttle setting.)},
+{NULL, NULL}
+};
+
+static const vshCmdOptDef opts_blkiothrottle[] = {
+{domain, VSH_OT_DATA, VSH_OFLAG_REQ, N_(domain name, id or uuid)},
+{device, VSH_OT_DATA, VSH_OFLAG_REQ, N_(block device)},
+{bps, VSH_OT_INT, VSH_OFLAG_NONE, N_(total throughput limits in
bytes/s)},
+{bps_rd, VSH_OT_INT, VSH_OFLAG_NONE, N_(read throughput limits in
bytes/s)},
+{bps_wr, VSH_OT_INT, VSH_OFLAG_NONE, N_(write throughput limits in
bytes/s)},
+{iops, VSH_OT_INT, VSH_OFLAG_NONE, N_(total operation limits in
numbers/s)},
+{iops_rd, VSH_OT_INT, VSH_OFLAG_NONE, N_(read operation limits in
numbers/s)},
+{iops_wr, VSH_OT_INT, VSH_OFLAG_NONE, N_(write operation limits in
numbers/s)},
+{NULL, 0, 0, NULL}
+};
+
+static bool
+cmdBlkIoThrottle(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+const char *name, *disk;
+virDomainBlockIoThrottleInfo info;
+virDomainBlockIoThrottleInfo reply;
+unsigned int flags = 0;
+int ret = -1;
+
+memset(info, 0, sizeof(info));
+
+if (!vshConnectionUsability(ctl, ctl-conn))
+goto out;
+
+if (!(dom = vshCommandOptDomain(ctl, cmd,name)))
+goto out;
+
+if (vshCommandOptString(cmd, device,disk) 0)
+goto out;
+
+if (vshCommandOptULongLong(cmd, bps,info.bps) 0) {
+info.bps = 0;
+}
+
+if (vshCommandOptULongLong(cmd, bps_rd,info.bps_rd) 0) {
+info.bps_rd = 0;
+}
+
+if (vshCommandOptULongLong(cmd, bps_wr,info.bps_wr) 0) {
+info.bps_wr = 0;
+}
+
+if (vshCommandOptULongLong(cmd, iops,info.iops) 0) {
+info.iops = 0;
+}
+
+if (vshCommandOptULongLong(cmd, iops_rd,info.iops_rd) 0) {
+info.iops_wr = 0;
+}
+
+if (vshCommandOptULongLong(cmd, iops_wr,info.iops_wr) 0) {
+info.bps_wr = 0;
+}
+
+if ((info.bps == 0) (info.bps_rd == 0) (info.bps_wr == 0)
+ (info.iops == 0) (info.iops_rd == 0) (info.iops_wr == 0)) {
What if I want to set one of these values to zero (ie. erase a current limit)?
Won't this mistakenly just print out the current settings? I think you'll need
a bit more sophistication here.
Yes, it need to be improved, especially when support --current --live --config.
I didn't write it in the summary since I think it be contained in to do list
#5)..
+
+ret = virDomainGetBlockIoThrottle(dom, disk,reply, flags);
+
+if (ret != 0)
+goto out;
+
+vshPrint(ctl, %-15s %llu\n, _(bps:), reply.bps);
+vshPrint(ctl, %-15s %llu\n, _(bps_rd:), reply.bps_rd);
+vshPrint(ctl, %-15s %llu\n, _(bps_wr:), reply.bps_wr);
+vshPrint(ctl, %-15s %llu\n, _(iops:), reply.iops);
+vshPrint(ctl, %-15s %llu\n, _(iops_rd:), reply.iops_rd);
+vshPrint(ctl, %-15s %llu\n, _(iops_wr:), reply.iops_wr);
+
+virDomainFree(dom);
+return true;
+} else {
+flags = 1;
+
+ret = virDomainSetBlockIoThrottle(dom, disk,info, flags);
+
+if (ret == 0) {
+virDomainFree(dom);
+return true;
+}
+}
+
+out:
+virDomainFree(dom);
+return false;
+}
/*
* net-autostart command
@@ -14017,6 +14115,7 @@ static const vshCmdDef domManagementCmds[] = {
{blkiotune, cmdBlkiotune, opts_blkiotune, info_blkiotune, 0},
{blockpull, cmdBlockPull, opts_block_pull, info_block_pull, 0},
{blockjob, cmdBlockJob, opts_block_job, info_block_job, 0},
+{blkiothrottle, cmdBlkIoThrottle, opts_blkiothrottle,
info_blkiothrottle, 0},
#ifndef WIN32
{console, cmdConsole, opts_console, info_console, 0},
#endif
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 775d302..61ec613 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -572,6 +572,19 @@ operation can be checked with Bblockjob.
Ipath specifies fully-qualified path of the disk.
Ibandwidth specifies copying bandwidth limit in Mbps.
+=item Bblkiothrottle Idomain Idevice [[I--bps Bbps] | [[I--bps_rd Bbps_rd] [I--bps_wr
Bbps_wr]] [[I--iops Biops] | [[I--iops_rd Biops_rd] [I--iops_wr Biops_wr]]
+
+Set or display the block disk io limits settting.
+Ipath specifies block disk name.
+I--bps specifies total
Re: [libvirt] [PATCH 6/8] Implement Get Block IO Throttle for qemu driver
On 10/28/2011 05:53 AM, Adam Litke wrote:
On Thu, Oct 27, 2011 at 05:20:08PM +0800, Lei HH Li wrote:
+static int
+qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result,
+ const char *device,
+ virDomainBlockIoThrottleInfoPtr reply)
+{
+virJSONValuePtr io_throttle;
+int ret = -1;
+int i;
+int found = 0;
+
+io_throttle = virJSONValueObjectGet(result, return);
+
+if (!io_throttle ||io_throttle-type != VIR_JSON_TYPE_ARRAY) {
^ need a space between || and io_throttle
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_( block_io_throttle reply was missing device list
));
+goto cleanup;
+}
+
+for (i = 0; i virJSONValueArraySize(io_throttle); i++) {
+virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i);
+virJSONValuePtr inserted;
+const char *current_dev;
+
+ if (!temp_dev || temp_dev-type !=VIR_JSON_TYPE_OBJECT) {
^ watch spaces
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(block io throttle device entry was not in expected
format));
+goto cleanup;
+}
+
+if ((current_dev = virJSONValueObjectGetString(temp_dev, device)) ==
NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(block io throttle device entry was not in expected
format));
+goto cleanup;
+}
+
+if(STRPREFIX(current_dev, QEMU_DRIVE_HOST_PREFIX))
+current_dev += strlen(QEMU_DRIVE_HOST_PREFIX);
Is the drive prefix always going to be there? If so, I would report an error if
it is missing. As written, we'll tolerate either if it's there or not.
New QEMU has separate names for host guest side of the disk
and libvirt gives the host side a 'drive-' prefix. The passed
in dev_name is the guest side though.
It's for read mode, finding the asked block device by name then
get the return value of 'query-block', json-object: 'inserted'.
--
Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/8] Add new API virDomainSetBlockIoThrottle
On 10/28/2011 04:39 AM, Adam Litke wrote:
On Thu, Oct 27, 2011 at 05:20:03PM +0800, Lei HH Li wrote:
This patch add new pulic API virDomainSetBlockIoThrottle to src/libvirt.c
enable iotune setting for a device in domain XML.
You should include the API definition for both virDomainSetBlockIoThrottle and
virDomainGetBlockIoThrottle in the same patch. That way, reviewers can see the
complete picture of all API changes in one place.
I split it since the feature 'block I/O setting was already reviewed at RFC V1',
and 'set' was implemented by supporting qmp command 'block_set_io_throttle'.
'get' was implemented by supporting qmp command 'query-block'.
I thought It will be much clearer to review patches that have been split.
OK, I will merge the Get method into this.
Signed-off-by: Zhi Yong Wuwu...@linux.vnet.ibm.com
Signed-off-by: Lei Lili...@linux.vnet.ibm.com
---
include/libvirt/libvirt.h.in | 17 +
src/conf/domain_conf.c | 76 ++
src/conf/domain_conf.h | 11 ++
src/driver.h | 11 ++
src/libvirt.c| 62 ++
src/libvirt_public.syms |1 +
src/util/xml.h |2 +
7 files changed, 180 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 7102bce..ff2926e 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -1575,6 +1575,23 @@ intvirDomainBlockJobSetSpeed(virDomainPtr dom, const
char *path,
int virDomainBlockPull(virDomainPtr dom, const char *path,
unsigned long bandwidth, unsigned int flags);
+typedef struct _virDomainBlockIoThrottleInfo virDomainBlockIoThrottleInfo;
+struct _virDomainBlockIoThrottleInfo {
+unsigned long long bps;
+unsigned long long bps_rd;
+unsigned long long bps_wr;
+unsigned long long iops;
+unsigned long long iops_rd;
+unsigned long long iops_wr;
In the last series, it was suggested that you change the names of these tuning
parameters to make them more self-explanatory. I suggested:
bps = total_bytes_sec : total throughput limit in bytes per second
bps_rd = read_bytes_sec : read throughput limit in bytes per second
bps_wr = write_bytes_sec : read throughput limit in bytes per second
iops= total_iops_sec : total I/O operations per second
iops_rd = read_iops_sec : read I/O operations per second
iops_wr = write_iops_sec : write I/O operations per second
This applies to the variable names in the above structure and to the attributes
in the XML schema.
+};
+typedef virDomainBlockIoThrottleInfo *virDomainBlockIoThrottleInfoPtr;
+
+int
+virDomainSetBlockIoThrottle(virDomainPtr dom,
+const char *disk,
+virDomainBlockIoThrottleInfoPtr info,
+unsigned int flags);
+
/*
* NUMA support
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7f9c542..8f1c65f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2444,6 +2444,41 @@ virDomainDiskDefParseXML(virCapsPtr caps,
iotag = virXMLPropString(cur, io);
ioeventfd = virXMLPropString(cur, ioeventfd);
event_idx = virXMLPropString(cur, event_idx);
+} else if (xmlStrEqual(cur-name, BAD_CAST iotune)) {
+char *io_throttle = NULL;
+io_throttle = virXMLPropString(cur, bps);
+if (io_throttle) {
+def-blkiothrottle.bps = strtoull(io_throttle, NULL, 10);
+VIR_FREE(io_throttle);
+}
+
+io_throttle = virXMLPropString(cur, bps_rd);
+if (io_throttle) {
+def-blkiothrottle.bps_rd = strtoull(io_throttle, NULL,
10);
+VIR_FREE(io_throttle);
+}
+
+io_throttle = virXMLPropString(cur, bps_wr);
+if (io_throttle) {
+def-blkiothrottle.bps_wr = strtoull(io_throttle, NULL,
10);
+VIR_FREE(io_throttle);
+}
+io_throttle = virXMLPropString(cur, iops);
+if (io_throttle) {
+def-blkiothrottle.iops = strtoull(io_throttle, NULL, 10);
+VIR_FREE(io_throttle);
+}
+
+io_throttle = virXMLPropString(cur, iops_rd);
+if (io_throttle) {
+def-blkiothrottle.iops_rd = strtoull(io_throttle, NULL,
10);
+VIR_FREE(io_throttle);
+}
+
+io_throttle = virXMLPropString(cur, iops_wr);
+if (io_throttle) {
+def-blkiothrottle.iops_wr = strtoull(io_throttle, NULL,
10);
+}
With so many attributes (bps, bps_rd, etc), I wonder if this schema should be
Re: [libvirt] [PATCH v3] pci address conflict when virtio disk with drive type
于 2011年10月28日 06:28, Eric Blake 写道:
On 10/23/2011 05:31 AM, Xu He Jie wrote:
When using the xml as below:
--
devices
emulator/home/soulxu/data/work-code/qemu-kvm/x86_64-softmmu/qemu-system-x86_64/emulator
disk type='file' device='disk'
driver name='qemu' type='qcow2'/
source file='/home/soulxu/data/VM/images/linux.img'/
target dev='vda' bus='virtio'/
address type='drive' controller='0' bus='0' unit='0'/
/disk
input type='mouse' bus='ps2'/
graphics type='vnc' port='-1' autoport='yes'/
video
model type='cirrus' vram='9216' heads='1'/
address type='pci' domain='0x' bus='0x00' slot='0x02'
function='0x0'/
/video
memballoon model='virtio'
address type='pci' domain='0x' bus='0x00' slot='0x03'
function='0x0'/
/memballoon
/devices
Lack of indentation made this harder to read.
--
Then can't startup qemu, the error message as below:
virsh # start test-vm
error: Failed to start domain test-vm
error: internal error process exited while connecting to monitor:
qemu-system-x86_64: -device
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3: PCI: slot 3
function 0 not available for virtio-balloon-pci, in use by
virtio-blk-pci
qemu-system-x86_64: -device
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3: Device
'virtio-balloon-pci' could not be initialized
So adding check for bus type and address type. Only the address of
pci type support by virtio bus.
Signed-off-by: Xu He Jiex...@linux.vnet.ibm.com
---
src/qemu/qemu_command.c | 10 +++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 30c0be6..7c4bc0a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1321,13 +1321,17 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
qemuDomainPCIAddressSetPtr addrs)
/* Disks (VirtIO only for now */
While we're here, add the closing ).
for (i = 0; i def-ndisks ; i++) {
- if (def-disks[i]-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
- continue;
-
/* Only VirtIO disks use PCI addrs */
if (def-disks[i]-bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
continue;
+ if ((def-disks[i]-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+ (def-disks[i]-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
This is user-visible, so VIR_ERR_CONFIG_UNSUPPORTED is probably better.
+ _(virtio only supports device address type 'PCI' ));
no trailing space in the message
+ goto error;
+ }
Your patch fails 'make check', so it needs another respin that
resolves this ('make check -C tests TESTS=qemuxml2argvtest
VIR_TEST_DEBUG=1'):
58) QEMU XML-2-ARGV disk-ioeventfd ...
Offset 387
Expect [4,drive=drive-virtio-disk0,id=virtio-disk0 -drive
file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,id=drive-ide0-1-0
-device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0
-device
virtio-net-pci,tx=bh,ioeventfd=off,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3
-net user,vlan=0,name=hostnet0 -serial pty -usb -vnc 127.0.0.1:-809
-std-vga -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5]
Actual [5,drive=drive-virtio-disk0,id=virtio-disk0 -drive
file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,id=drive-ide0-1-0
-device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0
-device
virtio-net-pci,tx=bh,ioeventfd=off,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3
-net user,vlan=0,name=hostnet0 -serial pty -usb -vnc 127.0.0.1:-809
-std-vga -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7]
... FAILED
In other words, your patch silently caused pci devices to be
renumbered. If it was a bug in our tests for using the wrong addresses
in the first place, then let's fix our tests; but more likely, this
means your patch was incorrect and would break guest ABI stability if
it were applied.
I checked the code again. My patch was incorrect. if address's type is
'VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI', it should not execute
'qemuDomainPCIAddressSetNextAddr' again.
I will send patch v4 later.
Thanks for your review.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] Update systemtap probing
You're right, that is the root cause of the numeric errors, supporting systemtap should install not only package systemtap,but also systemtap* to get dtrace installed. And also,many Ubuntu versions have a bug called:systemtap process probes requires CONFIG_UTRACE enabled,which means I can't use it without recompile the kernel :'( Thank you so much! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4] pci address conflict when virtio disk with drive type
When using the xml as below:
--
devices
emulator/home/soulxu/data/work-code/qemu-kvm/x86_64-softmmu/qemu-system-x86_64/emulator
disk type='file' device='disk'
driver name='qemu' type='qcow2'/
source file='/home/soulxu/data/VM/images/linux.img'/
target dev='vda' bus='virtio'/
address type='drive' controller='0' bus='0' unit='0'/
/disk
input type='mouse' bus='ps2'/
graphics type='vnc' port='-1' autoport='yes'/
video
model type='cirrus' vram='9216' heads='1'/
address type='pci' domain='0x' bus='0x00' slot='0x02' function='0x0'/
/video
memballoon model='virtio'
address type='pci' domain='0x' bus='0x00' slot='0x03' function='0x0'/
/memballoon
/devices
--
Then can't startup qemu, the error message as below:
virsh # start test-vm
error: Failed to start domain test-vm
error: internal error process exited while connecting to monitor:
qemu-system-x86_64: -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3:
PCI: slot 3 function 0 not available for virtio-balloon-pci, in use by
virtio-blk-pci
qemu-system-x86_64: -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3:
Device 'virtio-balloon-pci' could not be initialized
So adding check for bus type and address type. Only the address of pci type
support by virtio bus.
Signed-off-by: Xu He Jie x...@linux.vnet.ibm.com
---
src/qemu/qemu_command.c | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0c5bfab..07eaece 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1319,15 +1319,21 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
qemuDomainPCIAddressSetPtr addrs)
goto error;
}
-/* Disks (VirtIO only for now */
+/* Disks (VirtIO only for now) */
for (i = 0; i def-ndisks ; i++) {
-if (def-disks[i]-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
-continue;
-
/* Only VirtIO disks use PCI addrs */
if (def-disks[i]-bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
continue;
+if (def-disks[i]-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+continue;
+
+if (def-disks[i]-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _(virtio only support device address type 'PCI'));
+goto error;
+}
+
if (qemuDomainPCIAddressSetNextAddr(addrs, def-disks[i]-info) 0)
goto error;
}
--
1.7.5.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Fix virUUIDGeneratePseudoRandomBytes
It forgets to move a pointer to a buffer for UUID and as a result
fills only the first byte of the buffer.
---
src/util/uuid.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 0df3ebc..823a2b9 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -80,7 +80,7 @@ virUUIDGeneratePseudoRandomBytes(unsigned char *buf,
int buflen)
{
while (buflen 0) {
-*buf = virRandom(256);
+*buf++ = virRandom(256);
buflen--;
}
--
1.7.4.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: plug memory leak
From: Alex Jia a...@redhat.com Deteted by valgrind: ==18462== 1,100 bytes in 1 blocks are definitely lost in loss record 183 of 184 ==18462==at 0x4A05FDE: malloc (vg_replace_malloc.c:236) ==18462==by 0x4A06167: realloc (vg_replace_malloc.c:525) ==18462==by 0x4AADBB: virReallocN (memory.c:161) ==18462==by 0x4A975E: virBufferGrow (buf.c:117) ==18462==by 0x4A9D92: virBufferVasprintf (buf.c:290) ==18462==by 0x4A9EF7: virBufferAsprintf (buf.c:263) ==18462==by 0x429488: qemuBuildControllerDevStr (qemu_command.c:1993) ==18462==by 0x42C4B6: qemuBuildCommandLine (qemu_command.c:3803) ==18462==by 0x41A604: testCompareXMLToArgvHelper (qemuxml2argvtest.c:124) ==18462==by 0x41BB81: virtTestRun (testutils.c:141) ==18462==by 0x416DFF: mymain (qemuxml2argvtest.c:369) ==18462==by 0x41B277: virtTestMain (testutils.c:696) ==18462== ==18462== LEAK SUMMARY: ==18462==definitely lost: 1,100 bytes in 1 blocks ==18462==indirectly lost: 0 bytes in 0 blocks * src/qemu/qemu_command.c (qemuBuildCommandLine): Clean up on success. Signed-off-by: Alex Jia a...@redhat.com --- src/qemu/qemu_command.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0c5bfab..9c435de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3804,6 +3804,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; virCommandAddArg(cmd, devstr); +VIR_FREE(devstr); } } else if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_USB def-controllers[i]-model == -1 -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] Extend RPC protocol to allow FD passing
On 10/28/2011 02:28 AM, Daniel P. Berrange wrote:
+if ((fd = dup(msg-fds[slot])) 0) {
+virReportSystemError(errno,
+ _(Unable to duplicate FD %d),
+ msg-fds[slot]);
+return -1;
+}
Do we want to be using gnulib's dup_cloexec (cloexec.h) or
fcntl(fd, F_DUPFD_CLOEXEC, 0) here, so that our dup doesn't leak
into a third-party child if we are linked into some larger
multithreaded app?
dup3() is the new glibc API for this ? Does gnulib support
that yet ?
Gnulib supports dup3(). But dup3() is not the same as dup(); it is the
same as dup2(). That is:
dup(fd) maps to fcntl(fd, F_DUPFD_CLOEXEC,0) == dup_cloexec(fd)
dup2(fd1, fd2) maps to dup3(fd1, fd2, O_CLOEXEC)
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] Extend RPC protocol to allow FD passing
On Fri, Oct 28, 2011 at 05:49:59AM -0600, Eric Blake wrote:
On 10/28/2011 02:28 AM, Daniel P. Berrange wrote:
+if ((fd = dup(msg-fds[slot])) 0) {
+virReportSystemError(errno,
+ _(Unable to duplicate FD %d),
+ msg-fds[slot]);
+return -1;
+}
Do we want to be using gnulib's dup_cloexec (cloexec.h) or
fcntl(fd, F_DUPFD_CLOEXEC, 0) here, so that our dup doesn't leak
into a third-party child if we are linked into some larger
multithreaded app?
dup3() is the new glibc API for this ? Does gnulib support
that yet ?
Gnulib supports dup3(). But dup3() is not the same as dup(); it is
the same as dup2(). That is:
dup(fd) maps to fcntl(fd, F_DUPFD_CLOEXEC,0) == dup_cloexec(fd)
dup2(fd1, fd2) maps to dup3(fd1, fd2, O_CLOEXEC)
Ah ok, I see what you mean.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/8 v2] Summary on block IO throttle
On 10/28/2011 02:59 AM, Lei Li wrote: Given that the XML is named iotune under disk, we should probably name the virsh command 'blkiotune' or 'disk-iotune', not 'blkiothrottle'. Hi Eric, we usediothrottle first, I changed it since Daniel P. Berrange proposediotune for per-disk element instead ofiothrottle when we discussed at RFC V1. The command 'blkiotune' already exist, supported the cgroups blkio-controller, which handles proportional shares and throughput/iops limits on host block devices, global to the domain, but blkio throttling is specified per-disk and can vary across multiple disks. They are different two mechanism. So how about useiothrottle again? :) For extensibility, I _don't_ want to hardcode 'throttle' into the name; the goal here is that we want this xml element to contain all tuning parameters that are appropriate for a single disk, which could be more than just throttling. So using 'virsh disk-iotune' sounds like the best name for the virsh side of the command. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: avoid leaking uninit data from hotplug to dumpxml
On 10/26/2011 07:42 PM, Eric Blake wrote:
Detected by Coverity. The fix in 2c27dfa didn't catch all bad
instances of memcpy(). Thankfully, on further analysis, all of
the problematic uses are only triggered by old qemu that lacks
-device.
* src/qemu/qemu_hotplug.c (qemuDomainAttachPciDiskDevice)
(qemuDomainAttachNetDevice, qemuDomainAttachHostPciDevice): Init
all fields since monitor only populates some of them.
---
src/qemu/qemu_hotplug.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 037f4aa..06b21c3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -246,7 +246,7 @@ int qemuDomainAttachPciDiskDevice(struct qemud_driver
*driver,
}
}
} else {
-virDomainDevicePCIAddress guestAddr;
+virDomainDevicePCIAddress guestAddr = disk-info.addr.pci;
ret = qemuMonitorAddPCIDisk(priv-mon,
disk-src,
type,
@@ -775,6 +775,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
goto try_remove;
}
} else {
+guestAddr = net-info.addr.pci;
if (qemuMonitorAddPCINetwork(priv-mon, nicstr,
guestAddr) 0) {
qemuDomainObjExitMonitorWithDriver(driver, vm);
@@ -929,7 +930,7 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver
*driver,
configfd, configfd_name);
qemuDomainObjExitMonitorWithDriver(driver, vm);
} else {
-virDomainDevicePCIAddress guestAddr;
+virDomainDevicePCIAddress guestAddr = hostdev-info.addr.pci;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorAddPCIHostDevice(priv-mon,
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Fix virUUIDGeneratePseudoRandomBytes
On 10/28/2011 05:06 AM, Ryota Ozaki wrote:
It forgets to move a pointer to a buffer for UUID and as a result
fills only the first byte of the buffer.
---
src/util/uuid.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
How embarrassing! That bug has been present since commit bce1d26, Feb
2007! The only reason we haven't noticed it is that most systems have a
good /dev/urandom.
ACK and pushed.
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 0df3ebc..823a2b9 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -80,7 +80,7 @@ virUUIDGeneratePseudoRandomBytes(unsigned char *buf,
int buflen)
{
while (buflen 0) {
-*buf = virRandom(256);
+*buf++ = virRandom(256);
buflen--;
}
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/8 v2] Summary on block IO throttle
On Fri, Oct 28, 2011 at 08:09:32AM -0600, Eric Blake wrote: On 10/28/2011 02:59 AM, Lei Li wrote: Given that the XML is named iotune under disk, we should probably name the virsh command 'blkiotune' or 'disk-iotune', not 'blkiothrottle'. Hi Eric, we usediothrottle first, I changed it since Daniel P. Berrange proposediotune for per-disk element instead ofiothrottle when we discussed at RFC V1. The command 'blkiotune' already exist, supported the cgroups blkio-controller, which handles proportional shares and throughput/iops limits on host block devices, global to the domain, but blkio throttling is specified per-disk and can vary across multiple disks. They are different two mechanism. So how about useiothrottle again? :) For extensibility, I _don't_ want to hardcode 'throttle' into the name; the goal here is that we want this xml element to contain all tuning parameters that are appropriate for a single disk, which could be more than just throttling. So using 'virsh disk-iotune' sounds like the best name for the virsh side of the command. I'd prefer 'blkdeviotune', so it is discoverable alongside blkiotune 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/8 v2] Summary on block IO throttle
On Fri, Oct 28, 2011 at 04:59:40PM +0800, Lei Li wrote: On 10/27/2011 11:45 PM, Eric Blake wrote: On 10/27/2011 03:12 AM, Lei Li wrote: 1) Enable the blkio throttling in xml when guest is starting up. Add blkio throttling in xml as follows: disk type='file' device='disk' driver name='qemu' type='raw'/ source file='/var/lib/libvirt/images/kvm-one.img'/ target dev='vda' bus='virtio'/ address type='pci' domain='0x' bus='0x00' slot='0x05' function='0x0'/ iotune bps='n'.../ /disk 2) Enable blkio throttling setting at guest running time. virsh blkiothrottledomain device [--bpsnumber] [--bps_rdnumber] \ [--bps_wrnumber] [--iopsnumber] [--iops_rdnumber] [--iops_wrnumber] 3) The support to get the current block i/o throttling for a device - HMP/QMP. virsh blkiothrottledomain device Given that the XML is named iotune under disk, we should probably name the virsh command 'blkiotune' or 'disk-iotune', not 'blkiothrottle'. Hi Eric, we usediothrottle first, I changed it since Daniel P. Berrange proposediotune for per-disk element instead ofiothrottle when we discussed at RFC V1. The command 'blkiotune' already exist, supported the cgroups blkio-controller, which handles proportional shares and throughput/iops limits on host block devices, global to the domain, but blkio throttling is specified per-disk and can vary across multiple disks. They are different two mechanism. This is a per-device tunable, so just insert 'dev' into the command name. ie: blkdeviotune 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: avoid leaking uninit data from hotplug to dumpxml
On 10/28/2011 08:59 AM, Laine Stump wrote: On 10/26/2011 07:42 PM, Eric Blake wrote: Detected by Coverity. The fix in 2c27dfa didn't catch all bad instances of memcpy(). Thankfully, on further analysis, all of the problematic uses are only triggered by old qemu that lacks -device. * src/qemu/qemu_hotplug.c (qemuDomainAttachPciDiskDevice) (qemuDomainAttachNetDevice, qemuDomainAttachHostPciDevice): Init all fields since monitor only populates some of them. ACK. Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: plug memory leak
On 10/28/2011 05:35 AM, a...@redhat.com wrote: From: Alex Jiaa...@redhat.com Deteted by valgrind: s/Deteted/Detected/ Introduced by c1bc3d89 (unreleased, thankfully). ==18462== LEAK SUMMARY: ==18462==definitely lost: 1,100 bytes in 1 blocks ==18462==indirectly lost: 0 bytes in 0 blocks * src/qemu/qemu_command.c (qemuBuildCommandLine): Clean up on success. Pretty sizeable leak. ACK and pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
On 10/27/2011 02:28 AM, Daniel P. Berrange wrote: On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote: From: Sage Weils...@newdream.net Add a new secret type to store a Ceph authentication key. The name is simply an identifier for easy human reference. The xml looks like this: secret ephemeral='no' private='no' uuid0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f/uuid usage type='ceph' namemycluster_admin/name /usage /secret Signed-off-by: Sage Weils...@newdream.net Signed-off-by: Josh Durginjosh.dur...@dreamhost.com --- docs/schemas/secret.rng | 10 ++ include/libvirt/libvirt.h.in |3 +++ src/conf/secret_conf.c | 23 ++- src/conf/secret_conf.h |1 + src/secret/secret_driver.c |8 5 files changed, 44 insertions(+), 1 deletions(-) ACK I can't find the mail you are replying to, in order to apply it to the libvirt tree; was it accidentally sent off-list? Can anyone point me to a git tree with this patch series ready to be merged, or else resend it? -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
On 10/28/2011 10:33 AM, Eric Blake wrote: On 10/27/2011 02:28 AM, Daniel P. Berrange wrote: On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote: From: Sage Weils...@newdream.net Add a new secret type to store a Ceph authentication key. The name is simply an identifier for easy human reference. The xml looks like this: secret ephemeral='no' private='no' uuid0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f/uuid usage type='ceph' namemycluster_admin/name /usage /secret Signed-off-by: Sage Weils...@newdream.net Signed-off-by: Josh Durginjosh.dur...@dreamhost.com --- docs/schemas/secret.rng | 10 ++ include/libvirt/libvirt.h.in | 3 +++ src/conf/secret_conf.c | 23 ++- src/conf/secret_conf.h | 1 + src/secret/secret_driver.c | 8 5 files changed, 44 insertions(+), 1 deletions(-) ACK I can't find the mail you are replying to, in order to apply it to the libvirt tree; was it accidentally sent off-list? Can anyone point me to a git tree with this patch series ready to be merged, or else resend it? OK, I see an archive of it here: http://thread.gmane.org/gmane.comp.file-systems.ceph.devel/4129/focus=4133 but mail archives tend to corrupt any text with @, so I hope I'm porting it correctly. I'm not sure why the mail showed up on ceph-devel but not libvir-list. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: plug memory leak
Eric, thanks a lot :) Alex - Original Message - From: Eric Blake ebl...@redhat.com To: a...@redhat.com Cc: libvir-list@redhat.com Sent: Saturday, October 29, 2011 12:23:54 AM Subject: Re: [libvirt] [PATCH] qemu: plug memory leak On 10/28/2011 05:35 AM, a...@redhat.com wrote: From: Alex Jiaa...@redhat.com Deteted by valgrind: s/Deteted/Detected/ Introduced by c1bc3d89 (unreleased, thankfully). ==18462== LEAK SUMMARY: ==18462==definitely lost: 1,100 bytes in 1 blocks ==18462==indirectly lost: 0 bytes in 0 blocks * src/qemu/qemu_command.c (qemuBuildCommandLine): Clean up on success. Pretty sizeable leak. ACK and pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
On 10/27/2011 02:28 AM, Daniel P. Berrange wrote: On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote: From: Sage Weils...@newdream.net Add a new secret type to store a Ceph authentication key. The name is simply an identifier for easy human reference. The xml looks like this: secret ephemeral='no' private='no' uuid0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f/uuid usage type='ceph' namemycluster_admin/name /usage /secret Signed-off-by: Sage Weils...@newdream.net Signed-off-by: Josh Durginjosh.dur...@dreamhost.com --- docs/schemas/secret.rng | 10 ++ Missing docs/formatsecret.html.in changes to document this, but I think I managed. include/libvirt/libvirt.h.in |3 +++ src/conf/secret_conf.c | 23 ++- src/conf/secret_conf.h |1 + src/secret/secret_driver.c |8 5 files changed, 44 insertions(+), 1 deletions(-) ACK I'm adding this, and pushing: diff --git i/docs/formatsecret.html.in w/docs/formatsecret.html.in index 63a1f2a..01aff2d 100644 --- i/docs/formatsecret.html.in +++ w/docs/formatsecret.html.in @@ -39,8 +39,8 @@ dd Specifies what this secret is used for. A mandatory codetype/code attribute specifies the usage category, currently -only codevolume/code is defined. Specific usage categories are -described below. +only codevolume/code and codeceph/code are defined. +Specific usage categories are described below. /dd /dl @@ -54,6 +54,18 @@ this secret is associated with. /p +h3Usage type ceph/h3 + +p + This secret is associated with a Ceph RBD (rados block device). + The codelt;usage type='ceph'gt;/code element must contain + a single codename/code element that specifies a usage name + for the secret. The Ceph secret can then be used by UUID or by + this usage name via the codelt;authgt;/code element of + a a href=domain.html#elementsDisksdisk + device/a. span class=sinceSince 0.9.7/span. +/p + h2a name=exampleExample/a/h2 pre -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
On 10/28/2011 10:41 AM, Eric Blake wrote: On 10/27/2011 02:28 AM, Daniel P. Berrange wrote: On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote: From: Sage Weils...@newdream.net Add a new secret type to store a Ceph authentication key. The name is simply an identifier for easy human reference. The xml looks like this: secret ephemeral='no' private='no' uuid0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f/uuid usage type='ceph' namemycluster_admin/name /usage /secret Signed-off-by: Sage Weils...@newdream.net Signed-off-by: Josh Durginjosh.dur...@dreamhost.com --- docs/schemas/secret.rng | 10 ++ Missing docs/formatsecret.html.in changes to document this, but I think I managed. include/libvirt/libvirt.h.in | 3 +++ src/conf/secret_conf.c | 23 ++- src/conf/secret_conf.h | 1 + src/secret/secret_driver.c | 8 5 files changed, 44 insertions(+), 1 deletions(-) ACK I'm adding this, and pushing: Thanks, I'm not sure why the mail didn't go through to the libvirt list. It looks like there's a break missing in the pushed version though: diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index fa80888..a51fc69 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -55,6 +55,7 @@ virSecretDefFree(virSecretDefPtr def) case VIR_SECRET_USAGE_TYPE_CEPH: VIR_FREE(def-usage.ceph); +break; default: VIR_ERROR(_(unexpected secret usage type %d), def-usage_type); I'll send an updated version of the other patches shortly. diff --git i/docs/formatsecret.html.in w/docs/formatsecret.html.in index 63a1f2a..01aff2d 100644 --- i/docs/formatsecret.html.in +++ w/docs/formatsecret.html.in @@ -39,8 +39,8 @@ dd Specifies what this secret is used for. A mandatory codetype/code attribute specifies the usage category, currently - only codevolume/code is defined. Specific usage categories are - described below. + only codevolume/code and codeceph/code are defined. + Specific usage categories are described below. /dd /dl @@ -54,6 +54,18 @@ this secret is associated with. /p + h3Usage type ceph/h3 + + p + This secret is associated with a Ceph RBD (rados block device). + The codelt;usage type='ceph'gt;/code element must contain + a single codename/code element that specifies a usage name + for the secret. The Ceph secret can then be used by UUID or by + this usage name via the codelt;authgt;/code element of + a a href=domain.html#elementsDisksdisk + device/a. span class=sinceSince 0.9.7/span. + /p + h2a name=exampleExample/a/h2 pre -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
On 10/28/2011 12:47 PM, Josh Durgin wrote: Thanks, I'm not sure why the mail didn't go through to the libvirt list. It looks like there's a break missing in the pushed version though: diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index fa80888..a51fc69 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -55,6 +55,7 @@ virSecretDefFree(virSecretDefPtr def) case VIR_SECRET_USAGE_TYPE_CEPH: VIR_FREE(def-usage.ceph); + break; Apologies; that's what I get for manually applying your patch. :( I've pushed a trivial fix for that... -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef
On 10/27/2011 02:33 AM, Daniel P. Berrange wrote:
On Thu, Oct 20, 2011 at 11:01:25AM -0700, Josh Durgin wrote:
Add additional fields to let you specify the how to authenticate with a disk.
The secret to use may be referenced by a usage string or a UUID, i.e.:
auth username='myuser'
secret type='ceph' usage='secretname'/
/auth
or
auth username='myuser'
secret type='ceph' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/
/auth
+++ b/src/Makefile.am
@@ -128,7 +128,8 @@ DOMAIN_CONF_SOURCES =
\
conf/capabilities.c conf/capabilities.h \
conf/domain_conf.c conf/domain_conf.h \
conf/domain_audit.c conf/domain_audit.h \
- conf/domain_nwfilter.c conf/domain_nwfilter.h
+ conf/domain_nwfilter.c conf/domain_nwfilter.h \
+ conf/secret_conf.c
Unless I'm missing something, I don't think your code changes to
domain_conf.c actually introduce any dependancy on secret_conf.c
You include secret_conf.h, but that is only to get access to one
of the enum values. So there's no dep on the secret_conf.c code
and you can just drop this hunk
Actually, the linker now wants to pull in
virSecretUsageTypeTypeFromString (yuck; why do we have that doubled
Type in the name?), so that means more drivers have to add a link
library, to prevent problems like this:
libvirt_lxc-domain_conf.o: In function `virDomainDiskDefParseXML':
/home/remote/eblake/libvirt/src/conf/domain_conf.c:2479: undefined
reference to `virSecretUsageTypeTypeFromString'
+/attribute
+attribute name=usage
+ ref name=genericName/
This says usage='name' uses a genericName, but in secret.rng, you said
element name could use arbitrary text - that is, we have a discrepancy
where the secret could have an arbitrary name which validates for
secret.rng but fails to validate for auth as part of domain.rng. You
probably ought to do a followup patch that consolidates the two .rng
files to use the same definition for what you will accept as a valid
Ceph secret name.
+if (def-auth.username) {
+virBufferAsprintf(buf, auth username='%s'\n,
+ def-auth.username);
+if (def-auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
+virUUIDFormat(def-auth.secret.uuid, uuidstr);
+virBufferAsprintf(buf,
+ secret type='passphrase' uuid='%s'/\n,
This disagrees with your type='ceph' in the commit message (twice). You
would have caught this had you added a test that does round-trip from
XML in and back out somewhere in the series. Could you please do that
as a followup patch?
+ uuidstr);
+}
+if (def-auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
+virBufferAsprintf(buf,
This must use virBufferEscapeString, since the user's usage string may
have arbitrary text.
+ secret type='passphrase' usage='%s'/\n,
+ def-auth.secret.usage);
+}
+virBufferAsprintf(buf, /auth\n);
AddLit is more efficient than Asprintf for a constant string.
+enum virDomainDiskSecretType {
+VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
+VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
+VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
+
+VIR_DOMAIN_DISK_SECRET_TYPE_LAST
+};
+
/* Stores the virtual disk configuration */
typedef struct _virDomainDiskDef virDomainDiskDef;
typedef virDomainDiskDef *virDomainDiskDefPtr;
@@ -281,6 +289,14 @@ struct _virDomainDiskDef {
int protocol;
int nhosts;
virDomainDiskHostDefPtr hosts;
+struct {
+char *username;
+int secretType;
I like to add a comment stating which values are expected in this field
(here, enum virDomainDiskSecretType).
ACK with the Makefile.am hunk dropped
Also missing documentation. Here's what I had to squash in for that,
before pushing. Also, I added Josh to AUTHORS (shoot, I also realized
that I botched Josh's email in 1/4 when hand-applying everything, due to
battling the lost emails, sorry about that).
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index fcffb25..f31b775 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -913,6 +913,16 @@
lt;transient/gt;
lt;address type='drive' controller='0' bus='1' unit='0'/gt;
lt;/diskgt;
+lt;disk type='network'gt;
+ lt;driver name=qemu type=raw/gt;
+ lt;source protocol=rbd name=image_name2gt;
+lt;host name=hostname port=7000/gt;
+ lt;/sourcegt;
+ lt;target dev=hdd bus=ide/gt;
+ lt;auth username='myuser'gt;
+lt;secret type='ceph' usage='mypassid'/gt;
+ lt;/authgt;
+lt;/diskgt;
lt;disk type='block' device='cdrom'gt;
lt;driver name='qemu' type='raw'/gt;
lt;target def='hdc' bus='ide'/gt;
@@ -1160,7
Re: [libvirt] [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef
On 10/28/2011 11:53 AM, Eric Blake wrote:
On 10/27/2011 02:33 AM, Daniel P. Berrange wrote:
On Thu, Oct 20, 2011 at 11:01:25AM -0700, Josh Durgin wrote:
Add additional fields to let you specify the how to authenticate with
a disk.
The secret to use may be referenced by a usage string or a UUID, i.e.:
auth username='myuser'
secret type='ceph' usage='secretname'/
/auth
or
auth username='myuser'
secret type='ceph' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/
/auth
+++ b/src/Makefile.am
@@ -128,7 +128,8 @@ DOMAIN_CONF_SOURCES = \
conf/capabilities.c conf/capabilities.h \
conf/domain_conf.c conf/domain_conf.h \
conf/domain_audit.c conf/domain_audit.h \
- conf/domain_nwfilter.c conf/domain_nwfilter.h
+ conf/domain_nwfilter.c conf/domain_nwfilter.h \
+ conf/secret_conf.c
Unless I'm missing something, I don't think your code changes to
domain_conf.c actually introduce any dependancy on secret_conf.c
You include secret_conf.h, but that is only to get access to one
of the enum values. So there's no dep on the secret_conf.c code
and you can just drop this hunk
Actually, the linker now wants to pull in
virSecretUsageTypeTypeFromString (yuck; why do we have that doubled
Type in the name?), so that means more drivers have to add a link
library, to prevent problems like this:
libvirt_lxc-domain_conf.o: In function `virDomainDiskDefParseXML':
/home/remote/eblake/libvirt/src/conf/domain_conf.c:2479: undefined
reference to `virSecretUsageTypeTypeFromString'
+ /attribute
+ attribute name=usage
+ ref name=genericName/
This says usage='name' uses a genericName, but in secret.rng, you said
element name could use arbitrary text - that is, we have a discrepancy
where the secret could have an arbitrary name which validates for
secret.rng but fails to validate for auth as part of domain.rng. You
probably ought to do a followup patch that consolidates the two .rng
files to use the same definition for what you will accept as a valid
Ceph secret name.
Yeah, I'll fix that.
+ if (def-auth.username) {
+ virBufferAsprintf(buf, auth username='%s'\n,
+ def-auth.username);
+ if (def-auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
+ virUUIDFormat(def-auth.secret.uuid, uuidstr);
+ virBufferAsprintf(buf,
+ secret type='passphrase' uuid='%s'/\n,
This disagrees with your type='ceph' in the commit message (twice). You
would have caught this had you added a test that does round-trip from
XML in and back out somewhere in the series. Could you please do that as
a followup patch?
Oops, sorry about that. The reason I didn't include a test going from
commandline to secret is that we're going to be passing the secret
through the qemu monitor so it won't be exposed on the command line.
+ uuidstr);
+ }
+ if (def-auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
+ virBufferAsprintf(buf,
This must use virBufferEscapeString, since the user's usage string may
have arbitrary text.
+ secret type='passphrase' usage='%s'/\n,
+ def-auth.secret.usage);
+ }
+ virBufferAsprintf(buf, /auth\n);
AddLit is more efficient than Asprintf for a constant string.
+enum virDomainDiskSecretType {
+ VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
+ VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
+ VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
+
+ VIR_DOMAIN_DISK_SECRET_TYPE_LAST
+};
+
/* Stores the virtual disk configuration */
typedef struct _virDomainDiskDef virDomainDiskDef;
typedef virDomainDiskDef *virDomainDiskDefPtr;
@@ -281,6 +289,14 @@ struct _virDomainDiskDef {
int protocol;
int nhosts;
virDomainDiskHostDefPtr hosts;
+ struct {
+ char *username;
+ int secretType;
I like to add a comment stating which values are expected in this field
(here, enum virDomainDiskSecretType).
ACK with the Makefile.am hunk dropped
Also missing documentation. Here's what I had to squash in for that,
before pushing. Also, I added Josh to AUTHORS (shoot, I also realized
that I botched Josh's email in 1/4 when hand-applying everything, due to
battling the lost emails, sorry about that).
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index fcffb25..f31b775 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -913,6 +913,16 @@
lt;transient/gt;
lt;address type='drive' controller='0' bus='1' unit='0'/gt;
lt;/diskgt;
+ lt;disk type='network'gt;
+ lt;driver name=qemu type=raw/gt;
+ lt;source protocol=rbd name=image_name2gt;
+ lt;host name=hostname port=7000/gt;
+ lt;/sourcegt;
+ lt;target dev=hdd bus=ide/gt;
+ lt;auth username='myuser'gt;
+ lt;secret type='ceph' usage='mypassid'/gt;
+ lt;/authgt;
+ lt;/diskgt;
lt;disk type='block' device='cdrom'gt;
lt;driver name='qemu' type='raw'/gt;
lt;target def='hdc' bus='ide'/gt;
@@ -1160,7 +1170,24 @@
drive controller, additional attributes
codecontroller/code, codebus/code,
and codeunit/code are available, each defaulting to 0.
-
+ /dd
+ dtcodeauth/code/dt
+ ddIf present, the codeauth/code element provides the
+ authentication credentials needed to access the source. It
+ includes a mandatory
Re: [libvirt] [PATCH v4] pci address conflict when virtio disk with drive type
On 10/28/2011 03:57 AM, Xu He Jie wrote: Then can't startup qemu, the error message as below: virsh # start test-vm error: Failed to start domain test-vm error: internal error process exited while connecting to monitor: qemu-system-x86_64: -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3: PCI: slot 3 function 0 not available for virtio-balloon-pci, in use by virtio-blk-pci qemu-system-x86_64: -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3: Device 'virtio-balloon-pci' could not be initialized So adding check for bus type and address type. Only the address of pci type support by virtio bus. Signed-off-by: Xu He Jiex...@linux.vnet.ibm.com --- src/qemu/qemu_command.c | 14 ++ 1 files changed, 10 insertions(+), 4 deletions(-) Looks better, and passed testing. ACK and pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4 1/4] Rework value part of name-value pairs
On 10/27/2011 03:07 PM, Stefan Berger wrote:
NWFilters can be provided name-value pairs using the following
XML notiation:
filterref filter='xyz'
parameter name='PORT' value='80'/
parameter name='VAL' value='abc'/
/filterref
The internal representation currently is so that a name is stored as a
string and the value as well. This patch now addresses the value part of it
and introduces a data structure for storing a value either as a simple
value or as an array for later support of lists (provided in python-like
notation ( [a,b,c] ).
This patch adjusts all code that was handling the values in hash tables
and makes it use the new data type.
v4:
- Fixed virNWFilterVarValueDelValue to maintain order of elements
+
+void
+virNWFilterVarValuePrint(virNWFilterVarValuePtr val, virBufferPtr buf)
+{
+unsigned i;
+char *item;
+
+switch (val-valType) {
+case NWFILTER_VALUE_TYPE_SIMPLE:
+virBufferAdd(buf, val-u.simple.value, -1);
+break;
+case NWFILTER_VALUE_TYPE_ARRAY:
+virBufferAddLit(buf, [);
+for (i = 0; i val-u.array.nValues; i++) {
+if (i 0)
+virBufferAddLit(buf, , );
+item = val-u.array.values[i];
+if (item) {
+bool quote = false;
+if (c_isspace(item[0]) || c_isspace(item[strlen(item)- 1 ]))
+quote = true;
+if (quote)
+virBufferEscapeString(buf, %s, ');
+virBufferAdd(buf, val-u.array.values[i], -1);
+if (quote)
+virBufferEscapeString(buf, %s, ');
+}
+}
+virBufferAddLit(buf, ]);
This needs to be fixed, per Daniel's comments here:
https://www.redhat.com/archives/libvir-list/2011-October/msg01105.html
We'll need a v5, but as long as I've started reviewing this series, I'll
keep going...
+val-valType = NWFILTER_VALUE_TYPE_SIMPLE;
+if (copy_value) {
+val-u.simple.value = strdup(value);
+if (!val-u.simple.value) {
+VIR_FREE(val);
+virReportOOMError();
+return NULL;
+}
+} else
+val-u.simple.value = (char *)value;
Style - with if-else, if one branch needs {}, then you should use {} on
both branches.
Casting away const is risky - it does mean that the compiler can't
enforce someone who accidentally calls
virNWFilterVarValueCreateSimple(string_lit, false)
Could we instead split the decision by having two functions, correctly
typed?
virNWFilterVarValueTransferSimple(char *) - reuse
virNWFilterVarValueCopySimple(const char *) - copy
+
+bool
+virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value)
+{
+unsigned int i;
+
+switch (val-valType) {
+case NWFILTER_VALUE_TYPE_SIMPLE:
+return false;
+
+case NWFILTER_VALUE_TYPE_ARRAY:
+for (i = 0; i val-u.array.nValues; i++) {
+if (STREQ(value, val-u.array.values[i])) {
+VIR_FREE(val-u.array.values[i]);
+
+i++;
+for (; i val-u.array.nValues; i++)
+val-u.array.values[i - 1] = val-u.array.values[i];
Would memmove() be more efficient here?
+
+bool
+virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, const char *value,
+bool make_copy)
+{
+char *tmp;
+bool rc = false;
+
+if (make_copy) {
+value = strdup(value);
Now you've locked the just-malloc'd value into being const char*. I
would have gone through an intermediate variable, 'char *', so that you
can call helper functions without having to cast away const.
+if (!value) {
+virReportOOMError();
+return false;
+}
+}
+
+switch (val-valType) {
+case NWFILTER_VALUE_TYPE_SIMPLE:
+/* switch to array */
+tmp = val-u.simple.value;
+if (VIR_ALLOC_N(val-u.array.values, 2) 0) {
+val-u.simple.value = tmp;
+virReportOOMError();
+return false;
+}
+val-valType = NWFILTER_VALUE_TYPE_ARRAY;
+val-u.array.nValues = 2;
+val-u.array.values[0] = tmp;
+val-u.array.values[1] = (char *)value;
If the user didn't pass make_copy, this is casting away const; are we up
for the maintenance cost of making sure that is always safe?
+rc = true;
+break;
+
+case NWFILTER_VALUE_TYPE_ARRAY:
+if (VIR_REALLOC_N(val-u.array.values,
+ val-u.array.nValues + 1) 0) {
VIR_EXPAND_N might be better here; as it is a bit more structured than
VIR_REALLOC_N at guaranteeing new elements start life zero-initialized.
+typedef struct _virNWFilterVarValue virNWFilterVarValue;
+typedef virNWFilterVarValue *virNWFilterVarValuePtr;
+struct _virNWFilterVarValue {
+enum virNWFilterVarValueType valType;
+union {
+struct {
+char *value;
+}
[libvirt] [PATCH 1/1] Use a common xml type for ceph secret usage.
The types used in domaincommon.rng and secret.rng should be the same.
Signed-off-by: Josh Durgin josh.dur...@dreamhost.com
---
docs/schemas/domaincommon.rng | 11 ---
docs/schemas/secret.rng |4 +++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3477351..d053489 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2558,13 +2558,13 @@
attribute name='uuid'
ref name=UUID/
/attribute
-attribute name=usage
- ref name=genericName/
+attribute name='usage'
+ ref name='usageName'/
/attribute
/choice
/element
/define
-
+
!--
Optional hypervisor extensions in their own namespace:
QEmu
@@ -2675,6 +2675,11 @@
param
name=pattern(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)/param
/data
/define
+ define name=usageName
+data type=string
+ param name=pattern[a-zA-Z0-9_\.\+\-]+/param
+/data
+ /define
define name=usbId
data type=string
param name=pattern(0x)?[0-9a-fA-F]{1,4}/param
diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng
index 8e7714b..3abd3c7 100644
--- a/docs/schemas/secret.rng
+++ b/docs/schemas/secret.rng
@@ -4,6 +4,8 @@
ref name='secret'/
/start
+ include href='domaincommon.rng'/
+
define name='secret'
element name='secret'
optional
@@ -60,7 +62,7 @@
valueceph/value
/attribute
element name='name'
- text/
+ ref name='usageName'/
/element
/define
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4 2/4] Create rules for each member of a list
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch extends the NWFilter driver for Linux (ebiptables) to create
rules for each member of a previously introduced list. If for example
an attribute value (internally) looks like this:
IP = [10.0.0.1, 10.0.0.2, 10.0.0.3]
then 3 rules will be generated for a rule accessing the variable 'IP',
one for each member of the list. The effect of this is that this now
allows for filtering for multiple values in one field. This can then be
used to support for filtering/allowing of multiple IP addresses per
interface.
An interator is introduced that extracts each member of a list and
s/interator/iterator/
puts it into a hash table which then is passed to the function creating
a rule. For the above example the iterator would cause 3 loops.
+ebiptablesCreateRuleInstanceIterate(
+ virConnectPtr conn ATTRIBUTE_UNUSED,
+ enum virDomainNetType nettype ATTRIBUTE_UNUSED,
+ virNWFilterDefPtr nwfilter,
+ virNWFilterRuleDefPtr rule,
+ const char *ifname,
+ virNWFilterHashTablePtr vars,
+ virNWFilterRuleInstPtr res)
+{
+int rc = 0;
+virNWFilterVarCombIterPtr vciter;
+
+/* rule-vars holds all the variables names that this rule will access.
+ * iterate over all combinations of the variables' values and instantiate
+ * the filtering rule with each combination.
+ */
+vciter = virNWFilterVarCombIterCreate(vars, rule-vars, rule-nvars);
+if (!vciter)
+return 1;
Shouldn't we go with the more typical convention of -1 on error?
+static int
+virNWFilterVarCombIterAddVariable(virNWFilterVarCombIterEntryPtr cie,
+ virNWFilterHashTablePtr hash,
+ const char *varName)
+{
+virNWFilterVarValuePtr varValue;
+unsigned int cardinality;
+
+varValue = virHashLookup(hash-hashTable, varName);
+if (varValue == NULL) {
+virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+ _(Could not find value for variable '%s'),
+ varName);
+return 1;
+}
Again.
+
+if (VIR_REALLOC_N(cie-varNames, cie-nVarNames + 1) 0) {
+virReportOOMError();
+return 1;
Here too.
+}
+
+cie-varNames[cie-nVarNames] = varName;
+cie-nVarNames++;
+
+return 0;
+}
+
+/*
+ * Create an iterator over the contents of the given variables. All variables
+ * must have entries in the hash table.
+ * The iterator that is created processes all given variables in parallel,
+ * meaning it will access $ITEM1[0] and $ITEM2[0] then $ITEM1[1] and $ITEM2[1]
+ * upto $ITEM1[n] and $ITEM2[n]. For this to work, the cardinality of all
s/upto/up to/
+ * processed lists must be the same.
+ * The notation $ITEM1 and $ITEM2 (in one rule) therefore will always have to
+ * process the items in parallel. This could be an implicit notiation for
s/notiation/notation/
+typedef struct _virNWFilterVarCombIter virNWFilterVarCombIter;
+typedef virNWFilterVarCombIter *virNWFilterVarCombIterPtr;
+struct _virNWFilterVarCombIter {
+virNWFilterHashTablePtr hashTable;
+virNWFilterVarCombIterEntry iter[1];
1-element arrays look odd,
+unsigned int nIter;
+};
especially when they aren't the last element of a struct.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 4/4] qemu/rbd: improve rbd device specification
From: Sage Weil s...@newdream.net
This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.
And auth member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.
The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd. Instead, pass those options
explicitly to qemu. Update the qemu argument parsing and tests
accordingly.
Signed-off-by: Sage Weil s...@newdream.net
Signed-off-by: Josh Durgin josh.dur...@dreamhost.com
---
This fixes the things Daniel mentioned.
src/qemu/qemu_command.c| 284
.../qemuxml2argv-disk-drive-network-rbd-auth.args |6 +
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 +++
.../qemuxml2argv-disk-drive-network-rbd.args |6 +-
tests/qemuxml2argvtest.c | 56
5 files changed, 272 insertions(+), 117 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f5d89b9..48b0762 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
#include domain_audit.h
#include domain_conf.h
#include network/bridge_driver.h
+#include base64.h
#include sys/utsname.h
#include sys/stat.h
@@ -1495,6 +1496,159 @@ qemuSafeSerialParamValue(const char *value)
return 0;
}
+static int qemuBuildRBDString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt)
+{
+int i;
+virSecretPtr sec = NULL;
+char *secret = NULL;
+size_t secret_size;
+
+virBufferAsprintf(opt, rbd:%s, disk-src);
+if (disk-auth.username) {
+virBufferEscape(opt, :, :id=%s, disk-auth.username);
+/* look up secret */
+switch (disk-auth.secretType) {
+case VIR_DOMAIN_DISK_SECRET_TYPE_UUID:
+sec = virSecretLookupByUUID(conn,
+disk-auth.secret.uuid);
+break;
+case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE:
+sec = virSecretLookupByUsage(conn,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ disk-auth.secret.usage);
+break;
+}
+
+if (sec) {
+char *base64;
+
+secret = (char *)conn-secretDriver-getValue(sec, secret_size, 0,
+
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+if (secret == NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(could not get the value of the secret for
username %s),
+disk-auth.username);
+return -1;
+}
+/* qemu/librbd wants it base64 encoded */
+base64_encode_alloc(secret, secret_size, base64);
+virBufferEscape(opt, :, :key=%s:auth_supported=cephx\\;none,
+base64);
+VIR_FREE(base64);
+VIR_FREE(secret);
+virUnrefSecret(sec);
+} else {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(rbd username '%s' specified but secret not
found),
+disk-auth.username);
+return -1;
+}
+}
+
+if (disk-nhosts 0) {
+virBufferStrcat(opt, :mon_host=, NULL);
+for (i = 0; i disk-nhosts; ++i) {
+if (i) {
+virBufferStrcat(opt, \\;, NULL);
+}
+if (disk-hosts[i].port) {
+virBufferAsprintf(opt, %s\\:%s,
+ disk-hosts[i].name,
+ disk-hosts[i].port);
+} else {
+virBufferAsprintf(opt, %s, disk-hosts[i].name);
+}
+}
+}
+
+return 0;
+}
+
+static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
+{
+char *port;
+int ret;
+
+disk-nhosts++;
+ret = VIR_REALLOC_N(disk-hosts, disk-nhosts);
+if (ret 0) {
+virReportOOMError();
+return -1;
+}
+
+port = strstr(hostport, \\:);
+if (port) {
+*port = '\0';
+port += 2;
+disk-hosts[disk-nhosts-1].port = strdup(port);
+} else {
+disk-hosts[disk-nhosts-1].port = strdup(6789);
+}
+disk-hosts[disk-nhosts-1].name = strdup(hostport);
+return 0;
+}
+
+/* disk-src initially has everything after the rbd: prefix */
+static int
Re: [libvirt] [PATCH V4 3/4] Extend NWFilter parameter parser to cope with lists of values
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch modifies the NWFilter parameter parser to support multiple
elements with the same name and to internally build a list of items.
An example of the XML looks like this:
parameter name='TEST' value='10.1.2.3'/
parameter name='TEST' value='10.2.3.4'/
parameter name='TEST' value='10.1.1.1'/
Oh, I see - you fixed parsing to allow multiple, more or less replacing
the part of patch 1/4 that was trying to do [a,b,c] formatting. We
might as well ditch that part of the earlier patch, rather than
introducing it just to pull it back out.
The list of values is then stored in the newly introduced data type
virNWFilterVarValue.
The XML formatter is also adapted to print out all items in alphabetical
order sorted by 'name'.
This patch als fixes a bug in the XML schema on the way.
s/als/also/
-static void
-_formatParameterAttrs(void *payload, const void *name, void *data)
+static int
+virNWFilterFormatParameterNameSorter(const virHashKeyValuePairPtr a,
+ const virHashKeyValuePairPtr b)
{
-struct formatterParam *fp = (struct formatterParam *)data;
-virNWFilterVarValuePtr value = payload;
-
-virBufferAsprintf(fp-buf, %sparameter name='%s' value=',
- fp-indent,
- (const char *)name);
-virNWFilterVarValuePrint(value, fp-buf);
-virBufferAddLit(fp-buf, '/\n);
+return strcmp((const char *)a-key, (const char *)b-key);
}
-
char *
virNWFilterFormatParamAttributes(virNWFilterHashTablePtr table,
const char *indent)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
-struct formatterParam fp = {
-.buf =buf,
-.indent = indent,
-};
+char **keys, *key;
+int i, j, card, numKeys;
+virNWFilterVarValuePtr value;
+
+if (!table)
+return NULL;
+
+keys = (char **)virHashGetKeys(table-hashTable,
+ virNWFilterFormatParameterNameSorter);
+if (!keys)
+return NULL;
+
+numKeys = virHashSize(table-hashTable);
+
+for (i = 0; i numKeys; i++) {
+value = virHashLookup(table-hashTable, keys[i]);
+card = virNWFilterVarValueGetCardinality(value);
+
+for (j = 0; j card; j++) {
+virBufferAsprintf(buf,
+ %sparameter name='%s' value='%s'/\n,
+ indent, keys[i],
+ virNWFilterVarValueGetNthValue(value, j));
Are the parameter values guaranteed to be safe to print, or do you need
virBufferEscapeString()?
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V4 4/4] Add test cases for parsing of list values
On 10/27/2011 03:07 PM, Stefan Berger wrote: This patch adds test cases for parsing of parameters with multiple occurrances of the same name. Signed-off-by: Stefan Bergerstef...@linux.vnet.ibm.com --- tests/nwfilterxml2xmlin/attr-value-test.xml | 23 +++ tests/nwfilterxml2xmlout/attr-value-test.xml | 18 ++ tests/nwfilterxml2xmltest.c |2 ++ 3 files changed, 43 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml === --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml @@ -0,0 +1,23 @@ +filter name='testcase' +uuid83011800-f663-96d6-8841-fd836b4318c6/uuid +filterref filter='clean-traffic' +parameter name='a' value='1.2.3.4'/ +parameter name='a' value='10.1.2.3'/ +parameter name='a' value='10.3.3.3'/ +parameter name='b' value='1.2.3.4'/ It's okay that the key (name) gets sorted when it appears multiple times, but we need to make sure the value doesn't get reordered. Can we mix it up just a bit more to make it obvious that no sorting on value is happening, such as: +parameter name='a' value='1.2.3.4'/ +parameter name='a' value='1.2.3.6'/ +parameter name='a' value='1.2.3.5'/ Also, since we have a separate input from output, it might be worth intentionally putting name='b' out of order on the input, to show how it gets sorted into the output. But definitely a good idea to add tests. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] macvtap: Fix error return value convention/inconsistencies
From: Roopa Prabhu ropra...@cisco.com
- changed some return 1's to return -1
- changed if (rc) error checks to if (rc 0)
- fixed some other minor convention violations
I might have missed some. Can fix in another patch or can respin
Signed-off-by: Roopa Prabhu ropra...@cisco.com
Reported-by: Eric Blake ebl...@redhat.com
Reported-by: Laine Stump la...@laine.org
---
src/util/interface.c |8 +++---
src/util/macvtap.c | 64 +++---
src/util/pci.c |6 ++---
3 files changed, 41 insertions(+), 37 deletions(-)
diff --git a/src/util/interface.c b/src/util/interface.c
index 5d473b7..4ab74b5 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -1244,7 +1244,7 @@ ifaceIsVirtualFunction(const char *ifname)
char *if_sysfs_device_link = NULL;
int ret = -1;
-if (ifaceSysfsFile(if_sysfs_device_link, ifname, device))
+if (ifaceSysfsFile(if_sysfs_device_link, ifname, device) 0)
return ret;
ret = pciDeviceIsVirtualFunction(if_sysfs_device_link);
@@ -1272,10 +1272,10 @@ ifaceGetVirtualFunctionIndex(const char *pfname, const
char *vfname,
char *pf_sysfs_device_link = NULL, *vf_sysfs_device_link = NULL;
int ret = -1;
-if (ifaceSysfsFile(pf_sysfs_device_link, pfname, device))
+if (ifaceSysfsFile(pf_sysfs_device_link, pfname, device) 0)
return ret;
-if (ifaceSysfsFile(vf_sysfs_device_link, vfname, device)) {
+if (ifaceSysfsFile(vf_sysfs_device_link, vfname, device) 0) {
VIR_FREE(pf_sysfs_device_link);
return ret;
}
@@ -1306,7 +1306,7 @@ ifaceGetPhysicalFunction(const char *ifname, char
**pfname)
char *physfn_sysfs_path = NULL;
int ret = -1;
-if (ifaceSysfsDeviceFile(physfn_sysfs_path, ifname, physfn))
+if (ifaceSysfsDeviceFile(physfn_sysfs_path, ifname, physfn) 0)
return ret;
ret = pciDeviceNetName(physfn_sysfs_path, pfname);
diff --git a/src/util/macvtap.c b/src/util/macvtap.c
index f8b9d55..78e30f8 100644
--- a/src/util/macvtap.c
+++ b/src/util/macvtap.c
@@ -210,8 +210,11 @@ configMacvtapTap(int tapfd, int vnet_hdr)
rc_on_fail = -1;
errmsg = _(cannot clean IFF_VNET_HDR flag on macvtap tap);
} else if ((ifreq.ifr_flags IFF_VNET_HDR) == 0 vnet_hdr) {
-if (ioctl(tapfd, TUNGETFEATURES, features) != 0)
-return errno;
+if (ioctl(tapfd, TUNGETFEATURES, features) 0) {
+virReportSystemError(errno, %s,
+ _(cannot get feature flags on macvtap tap));
+return -1;
+ }
if ((features IFF_VNET_HDR)) {
new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
errmsg = _(cannot set IFF_VNET_HDR flag on macvtap tap);
@@ -335,7 +338,7 @@ create_name:
macaddress,
linkdev,
virtPortProfile,
- vmuuid, vmOp) != 0) {
+ vmuuid, vmOp) 0) {
rc = -1;
goto link_del_exit;
}
@@ -352,7 +355,6 @@ create_name:
}
rc = openTap(cr_ifname, 10);
-
if (rc = 0) {
if (configMacvtapTap(rc, vnet_hdr) 0) {
VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
@@ -552,6 +554,8 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf,
}
}
+return rc;
+
err_exit:
if (msg)
macvtapError(VIR_ERR_INTERNAL_ERROR, %s, msg);
@@ -572,7 +576,7 @@ doPortProfileOpSetLink(bool nltarget_kernel,
int32_t vf,
uint8_t op)
{
-int rc = 0;
+int rc = -1;
struct nlmsghdr *resp;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = {
@@ -588,7 +592,7 @@ doPortProfileOpSetLink(bool nltarget_kernel,
nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST);
if (!nl_msg) {
virReportOOMError();
-return -1;
+return rc;
}
if (nlmsg_append(nl_msg, ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) 0)
@@ -690,16 +694,12 @@ doPortProfileOpSetLink(bool nltarget_kernel,
if (!nltarget_kernel) {
pid = getLldpadPid();
-if (pid == 0) {
-rc = -1;
+if (pid == 0)
goto err_exit;
-}
}
-if (nlComm(nl_msg, recvbuf, recvbuflen, pid) 0) {
-rc = -1;
+if (nlComm(nl_msg, recvbuf, recvbuflen, pid) 0)
goto err_exit;
-}
if (recvbuflen NLMSG_LENGTH(0) || recvbuf == NULL)
goto malformed_resp;
@@ -716,7 +716,7 @@ doPortProfileOpSetLink(bool nltarget_kernel,
virReportSystemError(-err-error,
_(error during virtual port configuration of ifindex %d),
ifindex);
-rc = -1;
+goto err_exit;
}
break;
@@ -727,6 +727,8 @@ doPortProfileOpSetLink(bool nltarget_kernel,
goto malformed_resp;
}
+rc = 0;
+
err_exit:
nlmsg_free(nl_msg);
Re: [libvirt] [PATCH 1/1] Use a common xml type for ceph secret usage.
On 10/28/2011 03:19 PM, Josh Durgin wrote:
The types used in domaincommon.rng and secret.rng should be the same.
Signed-off-by: Josh Durginjosh.dur...@dreamhost.com
---
docs/schemas/domaincommon.rng | 11 ---
docs/schemas/secret.rng |4 +++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3477351..d053489 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2558,13 +2558,13 @@
attribute name='uuid'
ref name=UUID/
/attribute
-attribute name=usage
-ref name=genericName/
+attribute name='usage'
+ref name='usageName'/
genericName is probably fine instead of inventing usageName; but I'll
graduate it out of domaincommon.rng into basictypes.rng.
/attribute
/choice
/element
/define
-
+
Spurious whitespace change.
+++ b/docs/schemas/secret.rng
@@ -4,6 +4,8 @@
ref name='secret'/
/start
+include href='domaincommon.rng'/
domaincommon.rng is rather heavyweight; basictypes.rng is better.
+
define name='secret'
element name='secret'
optional
@@ -60,7 +62,7 @@
valueceph/value
/attribute
element name='name'
-text/
+ref name='usageName'/
/element
/define
At any rate, once we include common types, we can stop repeating the
UUID definition too.
Not quite like your original patch, but I'll go ahead and squash this
in, then push in your name since it matches up with your intent.
diff --git i/docs/schemas/basictypes.rng w/docs/schemas/basictypes.rng
index b3267f5..3b4b952 100644
--- i/docs/schemas/basictypes.rng
+++ w/docs/schemas/basictypes.rng
@@ -97,6 +97,12 @@
/choice
/define
+ define name=genericName
+data type=string
+ param name=pattern[a-zA-Z0-9_\+\-]+/param
+/data
+ /define
+
define name=dnsName
data type=string
param name=pattern[a-zA-Z0-9\.\-]+/param
diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index d053489..b6f858e 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -2559,12 +2559,12 @@
ref name=UUID/
/attribute
attribute name='usage'
- ref name='usageName'/
+ ref name='genericName'/
/attribute
/choice
/element
/define
-
+
!--
Optional hypervisor extensions in their own namespace:
QEmu
@@ -2660,11 +2660,6 @@
param name=pattern[A-Za-z0-9_\.\+\-]+/param
/data
/define
- define name=genericName
-data type=string
- param name=pattern[a-zA-Z0-9_\+\-]+/param
-/data
- /define
define name=bridgeMode
data type=string
param name=pattern(vepa|bridge|private|passthrough)/param
@@ -2675,11 +2670,6 @@
param
name=pattern(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)/param
/data
/define
- define name=usageName
-data type=string
- param name=pattern[a-zA-Z0-9_\.\+\-]+/param
-/data
- /define
define name=usbId
data type=string
param name=pattern(0x)?[0-9a-fA-F]{1,4}/param
diff --git i/docs/schemas/secret.rng w/docs/schemas/secret.rng
index 3abd3c7..e49bd5a 100644
--- i/docs/schemas/secret.rng
+++ w/docs/schemas/secret.rng
@@ -1,10 +1,11 @@
+?xml version=1.0?
!-- A Relax NG schema for the libvirt secret properties XML format --
grammar xmlns=http://relaxng.org/ns/structure/1.0;
start
ref name='secret'/
/start
- include href='domaincommon.rng'/
+ include href='basictypes.rng'/
define name='secret'
element name='secret'
@@ -62,25 +63,8 @@
valueceph/value
/attribute
element name='name'
- ref name='usageName'/
+ ref name='genericName'/
/element
/define
- define name=UUID
-choice
- data type=string
-param name=pattern[a-fA-F0-9]{32}/param
- /data
- data type=string
-param
name=pattern[a-fA-F0-9]{8}\-([a-fA-F0-9]{4}\-){3}[a-fA-F0-9]{12}/param
- /data
-/choice
- /define
-
- define name=absFilePath
-data type=string
- param name=pattern/[a-zA-Z0-9_\.\+\-amp;/%]+/param
-/data
- /define
-
/grammar
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] macvtap: Fix error return value convention/inconsistencies
On 10/28/2011 04:04 PM, Roopa Prabhu wrote:
From: Roopa Prabhuropra...@cisco.com
- changed some return 1's to return -1
- changed if (rc) error checks to if (rc 0)
- fixed some other minor convention violations
I might have missed some. Can fix in another patch or can respin
Signed-off-by: Roopa Prabhuropra...@cisco.com
Reported-by: Eric Blakeebl...@redhat.com
Reported-by: Laine Stumpla...@laine.org
Looks mostly good. I'll squash in some fixes as I finish auditing your
changes...
--- a/src/util/macvtap.c
+++ b/src/util/macvtap.c
@@ -210,8 +210,11 @@ configMacvtapTap(int tapfd, int vnet_hdr)
rc_on_fail = -1;
errmsg = _(cannot clean IFF_VNET_HDR flag on macvtap tap);
} else if ((ifreq.ifr_flags IFF_VNET_HDR) == 0 vnet_hdr) {
-if (ioctl(tapfd, TUNGETFEATURES,features) != 0)
-return errno;
+if (ioctl(tapfd, TUNGETFEATURES,features) 0) {
+virReportSystemError(errno, %s,
+ _(cannot get feature flags on macvtap tap));
+return -1;
+ }
No TABs.
if ((features IFF_VNET_HDR)) {
new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
errmsg = _(cannot set IFF_VNET_HDR flag on macvtap tap);
@@ -335,7 +338,7 @@ create_name:
macaddress,
linkdev,
virtPortProfile,
- vmuuid, vmOp) != 0) {
+ vmuuid, vmOp) 0) {
Needed some touchups before vpAssociatePortProfileId had a safe return
value. I also adjusted global callers, such as in src/qemu.
@@ -552,6 +554,8 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf,
}
}
+return rc;
+
Spurious addition.
@@ -963,10 +964,13 @@ getPhysfnDev(const char *linkdev,
*physfndev = strdup(linkdev);
if (!*physfndev) {
virReportOOMError();
-rc = -1;
-}
+goto err_exit;
+ }
+ rc = 0;
More TABs.
@@ -1011,7 +1015,7 @@ doPortProfileOp8021Qbh(const char *ifname,
case PREASSOCIATE_RR:
case ASSOCIATE:
rc = virGetHostUUID(hostuuid);
-if (rc)
+if (rc 0)
Won't work unless we also fix virGetHostUUID. Let's save that one for
later, since it touches 7 or so files.
@@ -1971,7 +1971,7 @@ pciGetVirtualFunctionIndex(const char
*pf_sysfs_device_link,
}
for (i = 0; i num_virt_fns; i++) {
- if (pciConfigAddressEqual(vf_bdf, virt_fns[i])) {
+ if (pciConfigAddressEqual(vf_bdf, virt_fns[i]) == 1) {
Spurious change.
Here's what I added before pushing:
diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
index decb0f2..838a31f 100644
--- i/src/qemu/qemu_migration.c
+++ w/src/qemu/qemu_migration.c
@@ -2527,7 +2527,7 @@
qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) {
virDomainNetGetActualDirectDev(net),
virDomainNetGetActualDirectVirtPortProfile(net),
def-uuid,
- VIR_VM_OP_MIGRATE_IN_FINISH) != 0)
+ VIR_VM_OP_MIGRATE_IN_FINISH) 0)
goto err_exit;
}
last_good_net = i;
diff --git i/src/util/interface.c w/src/util/interface.c
index 4ab74b5..12bf7bc 100644
--- i/src/util/interface.c
+++ w/src/util/interface.c
@@ -1016,7 +1016,7 @@ ifaceMacvtapLinkDump(bool nltarget_kernel
ATTRIBUTE_UNUSED,
* Get the nth parent interface of the given interface. 0 is the interface
* itself.
*
- * Return 0 on success, != 0 otherwise
+ * Return 0 on success, 0 otherwise
*/
#if defined(__linux__) WITH_MACVTAP
int
@@ -1037,7 +1037,7 @@ ifaceGetNthParent(int ifindex, const char *ifname,
unsigned int nthParent,
while (!end i = nthParent) {
rc = ifaceMacvtapLinkDump(true, ifname, ifindex, tb, recvbuf,
NULL);
-if (rc)
+if (rc 0)
break;
if (tb[IFLA_IFNAME]) {
diff --git i/src/util/macvtap.c w/src/util/macvtap.c
index 329cbf1..54dc670 100644
--- i/src/util/macvtap.c
+++ w/src/util/macvtap.c
@@ -214,7 +214,7 @@ configMacvtapTap(int tapfd, int vnet_hdr)
virReportSystemError(errno, %s,
_(cannot get feature flags on macvtap tap));
return -1;
- }
+}
if ((features IFF_VNET_HDR)) {
new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
errmsg = _(cannot set IFF_VNET_HDR flag on macvtap tap);
@@ -295,7 +295,7 @@ openMacvtapTap(const char *tgifname,
* emulate their switch in firmware.
*/
if (mode == VIR_MACVTAP_MODE_PASSTHRU) {
-if (ifaceReplaceMacAddress(macaddress, linkdev, stateDir) != 0) {
+if (ifaceReplaceMacAddress(macaddress, linkdev, stateDir) 0) {
return -1;
}
}
@@ -473,7 +473,7 @@ getLldpadPid(void) {
* status: pointer to a uint16 where the status
Re: [libvirt] [PATCH] qemu: Restore the original states of PCI device when restarting daemon
On 10/20/2011 04:45 AM, Osier Yang wrote:
This patch is to solve the problem by introducing internal XML
(won't be dumped to user, only dumped to status XML). The XML is:
origstates
unbind/
remove_slot/
reprobe/
/origstates
Which element will this be added under?
+++ b/src/conf/domain_conf.c
@@ -59,8 +59,12 @@ verify(VIR_DOMAIN_VIRT_LAST= 32);
/* Private flags used internally by virDomainSaveStatus and
* virDomainLoadStatus. */
typedef enum {
- VIR_DOMAIN_XML_INTERNAL_STATUS = (116), /* dump internal domain status
information */
- VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = (117), /* dump/parseactual
element */
+ /* dump internal domain status information */
+ VIR_DOMAIN_XML_INTERNAL_STATUS = (116),
+ /* dump/parseactual element */
+ VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = (117),
+ /* dump/parse original states of host PCI device */
+ VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (118),
Is this worth a new bit, or does it fit under the already-existing
umbrella of VIR_DOMAIN_XML_INTERNAL_STATUS, since we are treating these
three bits as internal status? But we can change that later, if we need.
+++ b/src/conf/domain_conf.h
@@ -153,6 +153,31 @@ struct _virDomainDeviceInfo {
} master;
};
+typedef struct _virDomainHostdevOrigStates virDomainHostdevOrigStates;
+typedef virDomainHostdevOrigStates *virDomainHostdevOrigStatesPtr;
+struct _virDomainHostdevOrigStates {
+union {
Don't you need some sort of discrimination field outside of the union
which tells you which part of the union is valid? Typically we use a
discriminator such as 'int type; /* enum vir... */'. But until we have
something to distinguish from, I guess this works.
+struct {
+/* Does the device need to ubind from stub when
s/ubind/unbind/
ACK with spelling nit fixed.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] RFC decoupling VM NIC provisioning from VM NIC connection to backend networks
Hi, In its current implementation Libvirt makes sure that the network interfaces that it passes/provision to a VM (for example to qemu[-kvm]) are already connected to its backend (interfaces/networks) by the time the VM starts its boot process. In a non virtualized setup it would be like booting a machine with the Ethernet cable already plugged into a router/switch port. While in a non virtualized setup you can boot a machine first (with no physical connection to a router/switch) and later connect its NIC/s to the switch/router, when you boot a VM via Libvirt it is not possible to decouple the two actions (VM boot, cable plug/unplug). An example of case where the capability of decoupling the two actions mentioned above is a requirement in Quantum/NetStack which is the network service leveraged by OpenStack. The modular design of OpenStack allows you to: - provision VMs with NIC/s - create networks - create ports on networks - plug/unplug a VM NIC into/from a given port on a network (at runtime) Note that this runtime plug/unplug requirement has nothing to do with hot plug/unplug of NICs. The idea is more that of decoupling the provisioning of a VM from the connection of the VM to the network/s. This would make it possible to change (at run-time too) the networks the NIC/s of a given VM are connected to. For example, when a VM boots, its interfaces should be in link down state if the network admin has not connected the VM NIC/s to any network yet. Even though libvirt already provides a way to change the link state of an a VM NIC, link state and physical connection are two different things and should be manageable independently. Ideally the configuration syntax should be interface type and hypervisor type agnostic. Let's take QEMU[-kvm] as an example - when Libvirt starts a QEMU VM, it passes to QEMU a number of file descriptors that map to host backend interfaces (for example macvtap interfaces). In order to introduce this runtime plug/unplug capability, we need a mechanism that permits to delay the binding between the host macvtap interfaces and the guest taps (because you cannot know the fd of the macvtap interfaces before you create them). This means you need a mechanism that allows you to change such fd/s at runtime: - you can close/reset an fd (ie, when you disconnect a VM NIC from its network) - you can open/set an fd (ie, when you connect a VM NIC to a network) This could probably be a libvirt command that translates to a QEMU monitor command. Can the runtime plug/unplug capability described above be achieved (cleanly) with another mechanism? Is anybody working on implementing something similar? [For more information on OpenStack/NetStack/Quantum and the above requirements please refer to the network model used therein: http://docs.openstack.org/incubation/openstack-network/admin/content/Wha tIsQuantum.html (information on network, port, and attachment abstractions) http://www.slideshare.net/danwent/quantum-diablo-summary (slides 7 8)] Thanks, ~Sumit Naiksatam. (On behalf of OpenStack/Quantum team) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] Allow multiple consoles per virtual guest
On 10/20/2011 08:47 AM, Daniel P. Berrange wrote: From: Daniel P. Berrangeberra...@redhat.com While Xen only has a single paravirt console, UML, and QEMU both support multiple paravirt consoles. The LXC driver can also be trivially made to support multiple consoles. This patch extends the XML to allow multiple console elements in the XML. It also makes the UML and QEMU drivers support this config. src/conf/domain_conf.c | 110 ++-- src/conf/domain_conf.h |4 +- I still need to review this series, but a quick question - shouldn't this patch also update docs/formatdomain.html.in, under name=elementCharConsole, to mention that multiple consoles are now supported in particular hypervisors? -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] V2 Modify generic ethernet interface so it will work when sVirt is enabled with qemu
On Thu, Oct 27, 2011 at 10:25 PM, Eric Blake ebl...@redhat.com wrote: On 10/24/2011 05:44 PM, Tyler Coumbes wrote: This patch makes the changes to the generic ethernet interface for QEMU. Allowing it to be used with sVirt enabled. src/qemu/qemu_command.c | 79 -- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 15 + src/qemu/qemu_process.c | 13 4 files changed, 107 insertions(+), 4 deletions(-) I haven't reviewed this one closely yet, but it would help if you added a test in tests/qemuxml2argvtest.c and a new pair of files in tests/qemuxml2argvdata/* that exercise the code to prove the new command line options look sane. Two questions about creating a test for this code. How do I get the actual args so I can compare them to my expected args to see what isn't matching up? VIR_TEST_DEBUG=2 shows me what tests fail but not any detail about why. I think it is because one of the parameters to qemu is a file descriptor which could change for each run. Is there some type of wildcard I can use like fd=%. I don't see any other tests using file descriptors to compare to and am not finding anything in any documentation I can find. Thanks, Tyler -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
