August 2009 - Devel - libvirt List Archives

[libvirt] Add support for (qcow*) volume encryption (v3)
by Miloslav Trmač 19 Aug '09

19 Aug '09

Hello, the following patches add full support for qcow/qcow2 volume encryption, assuming a client that supports it. (Main changes since the previous version: * Significantly change the internal API, basing it on virSecretPtr instead of (virConnectPtr, secret_id); details in patch 1 * Make virsh commands more similar to the existing commands * Remove <encryption format='unencrypted'/> * Rename "secret_id" to "uuid" in the XML and API * Use "unsigned char *" for secret value See the specific patch change logs for more details; patches without change logs are unchanged.) New XML tags are defined to represent encryption parameters (currently format and passphrase, more can be added in the future), e.g. <encryption format='qcow'> <secret type='passphrase' uuid='724d95f2-0ed2-6ff9-84d0-0f3d1618428d'/> </encryption> The <encryption> tag can be added to a <volume> node passed to virStorageVolCreateXML() to create an encrypted volume, or to a <disk> node inside a <domain> to specify what encryption parameters to use for a domain. uuid above refers to a separately-managed secret, which was created using virSecretDefineXML() and set using virSecretSetValue(). Other properties of the secret can be managed using an XML representation. Detailed documentation of the formats and features is inside the patches.

4 33

Re: [libvirt] [PATCH 07/20] Secret manipulation step 7: Local driver
by Miloslav Trmac 19 Aug '09

19 Aug '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Sun, Aug 16, 2009 at 10:48:00PM +0200, Miloslav Trmač wrote: > > This implementation stores the secrets in an unencrypted text file, > > for simplicity in implementation and debugging. > > > > (Symmetric encryption, e.g. using gpgme, will not be difficult to add. > > Because the TLS private key used by libvirtd is stored unencrypted, > > encrypting the secrets file does not currently provide much additional > > security.) > > What if we change our mind in some time, would there be any obstacle > to dynamically detecting things are not encrypted and switching to > a crypted file transparently? Two simple ways: 1) use a different file name, e,g. secrets.gpg 2) check the start of the file, currently the file always starts with "id "; I can add a proper magic number if necessary. > > +static int > > +writeBase64Data(virConnectPtr conn, int fd, const char *field, > > + const void *data, size_t size) > > +{ > > + int ret = -1; > > + char *base64 = NULL; > > + > > + if (writeString(conn, fd, field) < 0 || writeString(conn, fd, " > ") < 0) > > + goto cleanup; > > + > > + base64_encode_alloc(data, size, &base64); > > where is base64_encode_alloc coming from ? gnulib - in a later patch I'm afraid, I'll reorder it. > > +static int > > +saveSecrets(virConnectPtr conn, virSecretDriverStatePtr driver) > > +{ > > + const virSecretEntry *secret; > > + char *tmp_path = NULL; > > + int fd = -1, ret = -1; > > + > > + if (virAsprintf(&tmp_path, "%sXXXXXX", driver->filename) < 0) > { > > + virReportOOMError(conn); > > + goto cleanup; > > + } > > + fd = mkstemp (tmp_path); > > Hum even if unencrypted, we should make sure of the mode of the file > beforehand... Good point. > isn't there a safer equivalent (possibly made postable by gnulib ?) Grepping of gnulib does not reveal any. > > + if (fd == -1) { > > + virReportSystemError (conn, errno, _("mkstemp(\"%s\") failed"), > > + tmp_path); > > + goto cleanup; > > + } > > + > > + for (secret = driver->secrets; secret != NULL; > > + secret = secret->next) { > > + if (!secret->ephemeral && writeSecret(conn, fd, secret) < 0) > > + goto cleanup; > > + } > > + close(fd); > > + fd = -1; > > + if (rename(tmp_path, driver->filename) < 0) { > > + virReportSystemError (conn, errno, _("rename(%s, %s) failed"), tmp_path, > > + driver->filename); > > + goto cleanup; > > + } > > Hum, the whole set smells fishy, we are creating a temp file, without > mode checked, then moving that file somewhere else. I would ratehr have > internal APIsdeling with a dump to memory and then a single open, safe > and directly to the driver->filename. "Somewhere else" is in the same directory. The mkstemp() + rename() is used to make sure the secrets file is replaced atomically, without losing any data if a save is interrupted in the middle. > > +static int > > +parseBase64String(virConnectPtr conn, const char *base64, char > **string) > > +{ > > + char *tmp; > > + size_t size; > > + > > + if (!base64_decode_alloc(base64, strlen(base64), &tmp, &size)) > { > > where is base64_decode_alloc coming from ? gnulib > > + if (stat(driver->filename, &st) < 0) { > > + if (errno == ENOENT) > > + return 0; > > + virReportSystemError (conn, errno, _("cannot stat '%s'"), > > + driver->filename); > > + goto cleanup; > > + } > > + if ((size_t)st.st_size != st.st_size || (size_t)(st.st_size + 1) == 0) { > > + virSecretReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", > > + _("secrets file does not fit in memory")); > > + goto cleanup; > > + } > > + > > + fd = open(driver->filename, O_RDONLY); > > + if (fd == -1) { > > + virReportSystemError (conn, errno, _("cannot open '%s'"), > > + driver->filename); > > + goto cleanup; > > + } > > stat()/open() introduces a small race, I think open() and > then fdstat() is a bit cleaner, not a big deal though Technically there is still a race with fstat() - but fstat is a bit better. > > + if (VIR_ALLOC_N(contents, st.st_size + 1) < 0) { > > + virReportOOMError(conn); > > + goto cleanup; > > + } > > + if (saferead(fd, contents, st.st_size) != st.st_size) { > > + virReportSystemError (conn, errno, _("cannot read '%s'"), > > + driver->filename); > > + goto cleanup; > > + } > > + close(fd); > > + fd = -1; > > contents[st.st_size] = 0; > needed here. VIR_ALLOC_N automatically zeroes the memory. I'll fix all other issues you have mentioned. Thanks for the review. Mirek

1 0

Re: [libvirt] [PATCH 05/20] Secret manipulation step 5: RPC client
by Miloslav Trmac 19 Aug '09

19 Aug '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Sun, Aug 16, 2009 at 10:47:58PM +0200, Miloslav Trmač wrote: > > +static virDrvOpenStatus > > +remoteSecretOpen (virConnectPtr conn, > > + virConnectAuthPtr auth, > > + int flags) > > +{ > > + if (inside_daemon) > > + return VIR_DRV_OPEN_DECLINED; > > + > > + if (conn && > > + conn->driver && > > + STREQ (conn->driver->name, "remote")) { > > + struct private_data *priv; > > + > > + /* If we're here, the remote driver is already > > + * in use due to a) a QEMU uri, or b) a remote > > + * URI. So we can re-use existing connection > > + */ > > Hum, is that comment right ? We are certainly using a remote URI in > any case there, isn't it ? The same comment is in in all of the other "subdriver" remote...Open () routines. It's not quite clear what "remote" means where, as far as I can see "remote" means an "explicitly remote" URL, e.g. anything+tls://hostname/..., and "QEMU" means "qemu://hostname/..." (rejected by qemu_driver). Mirek

2 1

Re: [libvirt] [PATCH 01/20] Secret manipulation step 1: Public API
by Miloslav Trmac 19 Aug '09

19 Aug '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Sun, Aug 16, 2009 at 10:47:54PM +0200, Miloslav Trmač wrote: > > Rather than add explicit accessors for attributes of secrets, and > > hard-code the "secrets are related to storage volumes" association > in > > the API, the API uses XML to manipulate the association as well as > > other attributes, similarly to other areas of libvirt. > > > > The user can set attributes of the secret using XML, e.g. > > <secret ephemeral='no' private='yes'> > > <uuid>b8eecf55-798e-4db7-b2dd-025b0cf08a36</uuid> > > <volume>/var/lib/libvirt/images/mail.img</volume> > > <description>LUKS passphrase for our mail server</description> > > </secret> > > If <uuid/> is not specified, it is chosen automatically. > > Should secret always be tied to volumes. The API is generic enough > that we should make sure we can use this later to get priviledged > access to other resources, though right now I don't have a good > example in mind. The secret is not "tied" to a volume - the volume is an optional attribute. A libvirt user can ignore the volume field completely, and manage the secrets separately - but other clients, e.g. virt-manager, can use the volume attribute to locate a secret for a given volume without storing any data outside of libvirt. > > - s/secret_id/uuid/g > > Not sure I catch that, An earlier patch was calling the ID of a secret object "secret_id" everywhere, leading to constructs like <secret secret_id=.../> In an earlier patch review I was asked to change that. > > - use "unsigned char *" for secret value > > fine, assuming 0 terninated. though this may not work with everything > but since we need to import/export to XML, unless uuencoding the value > we won't be able to embed something which is not part of XML character > range (#x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | > [#x10000-#x10FFFF]) unicode code points. No, the value is an arbitrary set of bytes that is never passed through XML. Originally it was (void *value, size_t value_size), the change to (unsigned char *value, size_t value_size) was requested in an earlier review. > > diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in > > new file mode 100644 > > index 0000000..7471bf7 > > --- /dev/null > > +++ b/docs/formatsecret.html.in > > @@ -0,0 +1,52 @@ > > +<html> > > + <body> > > + <h1>Secret XML format</h1> > > + > > + <ul id="toc"></ul> > > it's good to have a document from the start :-) but this lacks a bit > of the intended use for the API, i.e. where this may be required to > use it. Those parts are documented in later patches that add references to secrets to the <volume> and <domain> XML formats. > > +virSecretPtr virSecretDefineXML (virConnectPtr conn, > > + const char *xml); > > Let's add an "unsigned int flags" to virSecretDefineXML() especially > as we don't know yet the scope of future usages. > > > +char * virSecretGetXMLDesc (virSecretPtr secret); > > And also add an "unsigned int flags" to virSecretGetXMLDesc > Will do. > > +int virSecretSetValue (virSecretPtr secret, > > + const unsigned char *value, > > + size_t value_size); > > +unsigned char * virSecretGetValue (virSecretPtr secret, > > + size_t *value_size); > > Ah, so we aren't relying on 0 termination, but in that case the value > need to be uuencoded in the XML, and that should be made clear in the > API description. Actually not having the secret completely in the clear > in the XML dump sounds like a good idea. The values are never transferred in the XML, only using the virSecret[GS]etValue functions. (Storing secrets in XML was a major objection against the first version of my patch e.g. because XML tends to be revealed in various debugging dumps.) Mirek

3 3

[libvirt] OpenVZ : The restriction of domain name should be addressed
by Yuji NISHIDA 19 Aug '09

19 Aug '09

Hi, "The current libvirt OpenVZ driver has a restriction that the domain names must match the OpenVZ container VEID" I really want to have domain name as character, not integer. It could help me very much if any patch against this problem would be released. Is there anyone tackling this problem now?( or going to? ) Otherwise, any hint would help me. Regards. ----- Yuji Nishida nishidy(a)nict.go.jp

5 5

[libvirt] Integrating MAC address based filtering into libvirt
by Gerhard Stenzel 19 Aug '09

19 Aug '09

Hello, I am currently investigating the possibility to implement MAC address based filtering in libvirt and was wondering if there is any related effort going on and what people in general would think about that. Here is a description of my current prototype implementation: I have a small setup of two guests and a network: Guest1: root@stenzel-desktop:/etc/libvirt/qemu# cat build1.xml <domain type='kvm'> <name>build1</name> ... <interface type='network'> <mac address='D0:0F:D0:0F:02:01'/> <source network='mynet'/> <model type='virtio' /> </interface> ... </domain> Guest2: root@stenzel-desktop:/etc/libvirt/qemu# cat build2.xml <domain type='kvm'> <name>build2</name> ... <interface type='network'> <mac address='D0:0F:D0:0F:02:02'/> <source network='mynet'/> <model type='virtio' /> </interface> ... </domain> and the network to which I added a new XML element "filter" with attribute "mac", which switches on the MAC address filtering: root@stenzel-desktop:/etc/libvirt/qemu# cat networks/mynet.xml <network> <name>mynet</name> <uuid>920debe0-c3ef-4395-8241-ee82d4b49c2d</uuid> <bridge name="br%d" stp="off"/> <filter mac="on"/> </network> the "filter" element is evaluated at startup of libvirtd and a generic ebtables rules is generated (all frames are dropped): stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo ebtables -L Bridge table: filter ... Bridge chain: FORWARD, entries: 0, policy: DROP ... When starting up guest1, ... stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo virsh start build1 Domain build1 started ... an ebtables rule is generated to allow its mac address on the its interface: stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo ebtables -L ... Bridge chain: FORWARD, entries: 1, policy: DROP -s d0:f:d0:f:2:1 -i vnet0 -j ACCEPT ... the same happens when starting up the second guest: stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo virsh start build2 Domain build2 started stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo ebtables -L ... Bridge chain: FORWARD, entries: 2, policy: DROP -s d0:f:d0:f:2:2 -i vnet1 -j ACCEPT -s d0:f:d0:f:2:1 -i vnet0 -j ACCEPT ... so the two guests are allowed to communicate. After destroying the two guests, the corresponding ebtables rules are removed: stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo virsh destroy build2 Domain build2 destroyed stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo ebtables -L ... Bridge chain: FORWARD, entries: 1, policy: DROP -s d0:f:d0:f:2:1 -i vnet0 -j ACCEPT ... stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo virsh destroy build1 Domain build1 destroyed stenzel@stenzel-desktop:~/projects/vsn/libvirt$ sudo ebtables -L ... Bridge chain: FORWARD, entries: 0, policy: DROP ... The current prototype implementation is based on the existing iptables wrapper in libvirt. I basically cloned the iptables wrapper to an ebtables wrapper and did some ebtables specific adjustments. There are currenlty four occasions when the ebtables wrapper is called: - when creating the network - when adding a guest to the network - when removing a guest from the network - when destroying the network (currently not implemented) These calls can be augmented to also do for example tagged vlan and protocol filtering. Configuring the filter rules via virsh is also an option. Comments are appreciated. -- Best regards, Gerhard Stenzel, ----------------------------------------------------------------------------------------------------------------------------------- IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Erich Baier Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

2 2

[libvirt] PATCH: Support setting a disk drive serial (number)
by Daniel P. Berrange 18 Aug '09

18 Aug '09

All hard disks (IDE, SCSI, USB, VirtIO) have a concept of a serial number which is intended to uniquely identify them. If you mean this off, QEMU just makes up a serial on the fly. THis is not guarenteed to be stable across guest reboots with hardware changes. It is thus desirable to be able to specify this in libvirt XML. To that end, this patch allows for a <serial> element inside a disk <disk type='file' device='disk'> <source file='/media/lacie-500-disk-2/virtual-machines/scratch5.img'/> <target dev='vda' bus='virtio'/> <serial>dan123virtio</serial> </disk> the contents is free-form. It implemenmts this for the QEMU driver for any disk using -drive style args. Unfortunately this excludes USB disk, even though internally QEMU can set a serial for these, the -usbdevice syntax does not allow it Daniel * docs/schemas/domain.rng: Add <serial> element to disks * src/domain_conf.h, src/domain_conf.c: XML parsing and formatting for disk serial numbers * src/qemu_conf.c: Set serial number when launching guests * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args, tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml: Add serial number to XML test --- docs/schemas/domain.rng | 10 ++++++++++ src/domain_conf.c | 11 +++++++++++ src/domain_conf.h | 1 + src/qemu_conf.c | 2 ++ .../qemuxml2argv-disk-drive-shared.args | 2 +- .../qemuxml2argv-disk-drive-shared.xml | 1 + 6 files changed, 26 insertions(+), 1 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index f857301..16f35e4 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -336,6 +336,11 @@ <empty/> </element> </optional> + <optional> + <element name="serial"> + <ref name="diskSerial"/> + </element> + </optional> </define> <!-- A disk description can be either of type file or block @@ -1135,6 +1140,11 @@ <param name="pattern">[A-Za-z0-9_\.\+\-&:/]+</param> </data> </define> + <define name="diskSerial"> + <data type="string"> + <param name="pattern">[A-Za-z0-9_\.\+\-]+</param> + </data> + </define> <define name="genericName"> <data type="string"> <param name="pattern">[a-zA-Z0-9_\+\-]+</param> diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..654f753 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -284,6 +284,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) if (!def) return; + VIR_FREE(def->serial); VIR_FREE(def->src); VIR_FREE(def->dst); VIR_FREE(def->driverName); @@ -661,6 +662,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *bus = NULL; char *cachetag = NULL; char *devaddr = NULL; + char *serial = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -718,6 +720,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { devaddr = virXMLPropString(cur, "devaddr"); + } else if ((serial == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "serial"))) { + serial = (char *)xmlNodeGetContent(cur); } } cur = cur->next; @@ -836,6 +841,8 @@ virDomainDiskDefParseXML(virConnectPtr conn, driverName = NULL; def->driverType = driverType; driverType = NULL; + def->serial = serial; + serial = NULL; cleanup: VIR_FREE(bus); @@ -847,6 +854,7 @@ cleanup: VIR_FREE(driverName); VIR_FREE(cachetag); VIR_FREE(devaddr); + VIR_FREE(serial); return def; @@ -3519,6 +3527,9 @@ virDomainDiskDefFormat(virConnectPtr conn, virBufferAddLit(buf, " <readonly/>\n"); if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if (def->serial) + virBufferEscapeString(buf, " <serial>%s</serial>\n", + def->serial); if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { virBufferAddLit(buf, " <state"); diff --git a/src/domain_conf.h b/src/domain_conf.h index 44302be..7bad5e7 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -109,6 +109,7 @@ struct _virDomainDiskDef { char *dst; char *driverName; char *driverType; + char *serial; int cachemode; unsigned int readonly : 1; unsigned int shared : 1; diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 082f107..95d41e6 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1748,6 +1748,8 @@ int qemudBuildCommandLine(virConnectPtr conn, if (disk->driverType && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT) virBufferVSprintf(&opt, ",format=%s", disk->driverType); + if (disk->serial) + virBufferVSprintf(&opt, ",serial=%s", disk->serial); if (disk->cachemode) { const char *mode = diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args index 611cd33..07cbe0e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,format=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,format=raw -net none -serial none -parallel none -usb +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,format=qcow2,serial=XYZXYZXYZYXXYZYZYXYZY,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml index b386315..c4e4734 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml @@ -19,6 +19,7 @@ <source dev='/dev/HostVG/QEMUGuest1'/> <target dev='hda' bus='ide'/> <shareable/> + <serial>XYZXYZXYZYXXYZYZYXYZY</serial> </disk> <disk type='block' device='cdrom'> <driver name='qemu' type='raw'/> -- 1.6.2.5 -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

4 7

[libvirt] [PATCH] Don't expose 'vnet%d' to the user
by Mark McLoughlin 18 Aug '09

18 Aug '09

https://bugzilla.redhat.com/517371 Matt Booth points out that if you use a non-existent bridge name when start a guest you get a weird error message: Failed to add tap interface 'vnet%d' to bridge 'virbr0' and dev='vnet%d' appears in the dumpxml output. Fix that by not including 'vnet%d' in the error message and freeing the 'vnet%d' string if adding the tap device to the bridge fails. * src/qemu_conf.c, src/uml_conf.c: fix qemudNetworkIfaceConnect() and umlConnectTapDevice() to not expose 'vnet%d' to the user --- src/qemu_conf.c | 25 +++++++++++++++++-------- src/uml_conf.c | 28 +++++++++++++++++++--------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 082f107..bcdab11 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1038,6 +1038,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, int err; int tapfd = -1; int vnet_hdr = 0; + int template_ifname = 0; if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetworkPtr network = virNetworkLookupByName(conn, @@ -1059,6 +1060,14 @@ qemudNetworkIfaceConnect(virConnectPtr conn, return -1; } + char ebuf[1024]; + if (!driver->brctl && (err = brInit(&driver->brctl))) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot initialize bridge support: %s"), + virStrerror(err, ebuf, sizeof ebuf)); + return -1; + } + if (!net->ifname || STRPREFIX(net->ifname, "vnet") || strchr(net->ifname, '%')) { @@ -1067,14 +1076,8 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virReportOOMError(conn); return -1; } - } - - char ebuf[1024]; - if (!driver->brctl && (err = brInit(&driver->brctl))) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot initialize bridge support: %s"), - virStrerror(err, ebuf, sizeof ebuf)); - return -1; + /* avoid exposing vnet%d in dumpxml or error outputs */ + template_ifname = 1; } if (qemuCmdFlags & QEMUD_CMD_FLAG_VNET_HDR && @@ -1088,12 +1091,18 @@ qemudNetworkIfaceConnect(virConnectPtr conn, qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface to bridge. " "%s is not a bridge device"), brname); + } else if (template_ifname) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to add tap interface to bridge '%s' : %s"), + brname, virStrerror(err, ebuf, sizeof ebuf)); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface '%s' " "to bridge '%s' : %s"), net->ifname, brname, virStrerror(err, ebuf, sizeof ebuf)); } + if (template_ifname) + VIR_FREE(net->ifname); return -1; } diff --git a/src/uml_conf.c b/src/uml_conf.c index 4f756d4..a4c434f 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -104,17 +104,10 @@ umlConnectTapDevice(virConnectPtr conn, virDomainNetDefPtr net, const char *bridge) { + brControl *brctl = NULL; int tapfd = -1; + int template_ifname = 0; int err; - brControl *brctl = NULL; - - if (!net->ifname || - STRPREFIX(net->ifname, "vnet") || - strchr(net->ifname, '%')) { - VIR_FREE(net->ifname); - if (!(net->ifname = strdup("vnet%d"))) - goto no_memory; - } if ((err = brInit(&brctl))) { char ebuf[1024]; @@ -124,6 +117,16 @@ umlConnectTapDevice(virConnectPtr conn, goto error; } + if (!net->ifname || + STRPREFIX(net->ifname, "vnet") || + strchr(net->ifname, '%')) { + VIR_FREE(net->ifname); + if (!(net->ifname = strdup("vnet%d"))) + goto no_memory; + /* avoid exposing vnet%d in dumpxml or error outputs */ + template_ifname = 1; + } + if ((err = brAddTap(brctl, bridge, &net->ifname, BR_TAP_PERSIST, &tapfd))) { if (errno == ENOTSUP) { @@ -131,6 +134,11 @@ umlConnectTapDevice(virConnectPtr conn, umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface to bridge. " "%s is not a bridge device"), bridge); + } else if (template_ifname) { + char ebuf[1024]; + umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to add tap interface to bridge '%s' : %s"), + bridge, virStrerror(err, ebuf, sizeof ebuf)); } else { char ebuf[1024]; umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, @@ -138,6 +146,8 @@ umlConnectTapDevice(virConnectPtr conn, "to bridge '%s' : %s"), net->ifname, bridge, virStrerror(err, ebuf, sizeof ebuf)); } + if (template_ifname) + VIR_FREE(net->ifname); goto error; } close(tapfd); -- 1.6.2.5

3 5

[libvirt] [PATCH 0/8] Various KVM PCI device assignment improvements
by Mark McLoughlin 18 Aug '09

18 Aug '09

Hi, Here's a fairly mixed set of KVM PCI device assignment improvements: - Add hotplug support - Allow PM reset on multi-function devices - Allow Secondary Bus Reset even if it causes other devices to be reset, so long as those other devices are in use by the same VM or are unused - Re-attach devices when the guest shuts down - Properly detect versions of QEMU without -pcidevice support Cheers, Mark.

4 26

[libvirt] [PATCH] Fix up connection reference counting.
by Chris Lalancette 18 Aug '09

18 Aug '09

Currently the reference counting for connections is busted. I first noticed it while trying to use virConnectRef; it would eventually cause a crash in the remote_internal driver, although that was really just a victim. Really, we should only call the close callbacks on the methods when the references drop to 0. To accomplish this, move all of the close callbacks into virUnrefConnect (since there are lots of internal users of that function), and arrange for virConnectClose to call that. V2: Make sure to drop the connection lock before we call the close callbacks, otherwise we could deadlock the daemon Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- src/datatypes.c | 19 +++++++++++++++++++ src/libvirt.c | 14 +------------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 9d556c8..bcb5fd4 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -237,6 +237,25 @@ virUnrefConnect(virConnectPtr conn) conn->refs--; refs = conn->refs; if (refs == 0) { + /* make sure to release the connection lock before we call the + * close() callbacks, otherwise we will deadlock if an error + * is raised by any of the callbacks + */ + virMutexUnlock(&conn->lock); + if (conn->networkDriver) + conn->networkDriver->close (conn); + if (conn->interfaceDriver) + conn->interfaceDriver->close (conn); + if (conn->storageDriver) + conn->storageDriver->close (conn); + if (conn->deviceMonitor) + conn->deviceMonitor->close (conn); + conn->driver->close (conn); + + /* reacquire the connection lock since virReleaseConnect expects + * it to already be held + */ + virMutexLock(&conn->lock); virReleaseConnect(conn); /* Already unlocked mutex */ return (0); diff --git a/src/libvirt.c b/src/libvirt.c index 5aa7f83..472c19b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1231,19 +1231,7 @@ virConnectClose(virConnectPtr conn) return (-1); } - if (conn->networkDriver) - conn->networkDriver->close (conn); - if (conn->interfaceDriver) - conn->interfaceDriver->close (conn); - if (conn->storageDriver) - conn->storageDriver->close (conn); - if (conn->deviceMonitor) - conn->deviceMonitor->close (conn); - conn->driver->close (conn); - - if (virUnrefConnect(conn) < 0) - return (-1); - return (0); + return virUnrefConnect(conn); } /** -- 1.6.0.6

2 2