[libvirt] [PATCH 0/2] Two fixes for graphics listen
These area a followup to Luyao Huang's patch to fix incorrect reporting from virsh domdisplay: https://www.redhat.com/archives/libvir-list/2015-February/msg00298.html While his patch works when the remote libvirtd is 0.9.4 or greater, it won't work when talking to older libvirt. Also the code as written isn't what was intended. Patch 1 takes care of that. Patch 2 fixes two theoretical memory leaks (which never happen in real life, but could some day if the functions are used differently). Laine Stump (2): domain: backfill listen address to parent <graphics> listen attribute domain: avoid potential memory leak in virDomainGraphicsListenSet*() src/conf/domain_conf.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) -- 2.1.0
Prior to 0.9.4, libvirt only supported a single listen, and it had to be an IP address: <graphics listen='1.2.3.4' ..../> Starting with 0.9.4, a graphics element could have a <listen> subelement (actually the grammar supports multiples, but all of the drivers only support a single <listen> per <graphics>), and that listen element can be of type='address' or type='network'. For type='address', <listen> also has an attribute called 'address' which contains the IP address for listening: <graphics ....> <listen type='address' address='1.2.3.4' .../> </graphics> type can also be "network", and in that case listen will have a "network" attribute which will contain the name of a libvirt network: <graphics ....> <listen type='network' network='testnet' .../> </graphics> At domain start (or migrate) time, libvirt will attempt to find an IP address associated with that network (e.g. the IP address of the bridge device used by the network, or the physical device listed in <forward dev='physdev'/>) and fill in that address in the status XML: <graphics ....> <listen type='network' network='testnet' address='1.2.3.4' .../> </graphics> In the case that a <graphics> element has a <listen> subelement of type='address', that listen subelement's "address" attribute is backfilled into the parent graphics element's "listen" *attribute* for backward compatibility (so that a management application unaware of the separate <listen> element can still learn the listen address). This backfill should be done with the IP learned from type='network' as well, and that's what this patch does: <graphics listen='1.2.3.4' ....> <listen type='network' network='testnet' address='1.2.3.4' .../> </graphics> This is a continuation of the fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1191016 --- src/conf/domain_conf.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6a57d80..6614fb1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18906,17 +18906,23 @@ virDomainGraphicsDefFormat(virBufferPtr buf, return -1; } - /* find the first listen element of type='address' and duplicate - * its address attribute as the listen attribute of - * <graphics>. This is done to improve backward compatibility. */ + /* find the first listen subelement with a valid address and + * duplicate its address attribute as the listen attribute of + * <graphics>. This is done to improve backward compatibility. + */ for (i = 0; i < def->nListens; i++) { - if (virDomainGraphicsListenGetType(def, i) - == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) { - if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE && - def->listens[i].fromConfig) - continue; - listenAddr = virDomainGraphicsListenGetAddress(def, i); - break; + virDomainGraphicsListenType listenType; + + if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE && + def->listens[i].fromConfig) + continue; + listenType = virDomainGraphicsListenGetType(def, i); + + if (listenType == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS || + (listenType == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK && + !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))) { + if ((listenAddr = virDomainGraphicsListenGetAddress(def, i))) + break; } } -- 2.1.0
virDomainGraphicsListenSetAddress() and virDomainGraphicsListenSetNetwork() both set their respective char* to NULL directly when asked to set it to NULL, which is okay as long as it's already set to NULL. If these functions are ever called to clear a listen object that has a valid string in address or network, it will end up leaking the old value. Currently that doesn't happen, so this is just a preemptive strike. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6614fb1..8600eff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21440,7 +21440,7 @@ virDomainGraphicsListenSetAddress(virDomainGraphicsDefPtr def, listenInfo->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS; if (!address) { - listenInfo->address = NULL; + VIR_FREE(listenInfo->address); return 0; } @@ -21478,7 +21478,7 @@ virDomainGraphicsListenSetNetwork(virDomainGraphicsDefPtr def, listenInfo->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK; if (!network) { - listenInfo->network = NULL; + VIR_FREE(listenInfo->network); return 0; } -- 2.1.0
On Tue, Feb 10, 2015 at 03:18:22PM -0500, Laine Stump wrote:
These area a followup to Luyao Huang's patch to fix incorrect reporting from virsh domdisplay:
https://www.redhat.com/archives/libvir-list/2015-February/msg00298.html
While his patch works when the remote libvirtd is 0.9.4 or greater, it won't work when talking to older libvirt. Also the code as written isn't what was intended. Patch 1 takes care of that. Patch 2 fixes two theoretical memory leaks (which never happen in real life, but could some day if the functions are used differently).
Laine Stump (2): domain: backfill listen address to parent <graphics> listen attribute domain: avoid potential memory leak in virDomainGraphicsListenSet*()
src/conf/domain_conf.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
ACK series Jan
participants (2)
-
Ján Tomko -
Laine Stump