- Devel - libvirt List Archives

[Libvir] [patch 2/5] iptables: use calloc() instead of malloc()/memset()
by Mark McLoughlin 21 Mar '07

21 Mar '07

Replace a few instances of malloc() followed by memset(0) with calloc() Signed-off-by: Mark McLoughlin <markmc(a)redhat.com> Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -270,11 +270,9 @@ iptRulesNew(const char *table, { iptRules *rules; - if (!(rules = (iptRules *)malloc(sizeof (iptRules)))) + if (!(rules = (iptRules *)calloc(1, sizeof (iptRules)))) return NULL; - memset (rules, 0, sizeof (iptRules)); - if (!(rules->table = strdup(table))) goto error; @@ -353,11 +351,9 @@ iptablesAddRemoveChain(iptRules *rules, 2 + /* --table foo */ 2; /* --new-chain bar */ - if (!(argv = (char **)malloc(sizeof(char *) * (n+1)))) + if (!(argv = (char **)calloc(n + 1, sizeof(char *)))) goto error; - memset(argv, 0, sizeof(char *) * (n + 1)); - n = 0; if (!(argv[n++] = strdup(IPTABLES_PATH))) @@ -413,14 +409,12 @@ iptablesAddRemoveRule(iptRules *rules, i va_end(args); - if (!(argv = (char **)malloc(sizeof(char *) * (n + 1)))) + if (!(argv = (char **)calloc(n + 1, sizeof(char *)))) goto error; if (!(rule = (char *)malloc(rulelen))) goto error; - memset(argv, 0, sizeof(char *) * (n + 1)); - n = 0; if (!(argv[n++] = strdup(IPTABLES_PATH))) --

1 0

[Libvir] [patch 1/5] iptables: fix invalid free
by Mark McLoughlin 21 Mar '07

21 Mar '07

In iptablesContextNew(), make sure we don't try and free an invalid pointer if one of the iptRulesNew() fails. Signed-off-by: Mark McLoughlin <markmc(a)redhat.com> Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -496,7 +496,7 @@ iptablesContextNew(void) { iptablesContext *ctx; - if (!(ctx = (iptablesContext *) malloc(sizeof (iptablesContext)))) + if (!(ctx = (iptablesContext *) calloc(1, sizeof (iptablesContext)))) return NULL; if (!(ctx->input_filter = iptRulesNew("filter", IPTABLES_PREFIX "INPUT"))) @@ -518,9 +518,12 @@ iptablesContextNew(void) void iptablesContextFree(iptablesContext *ctx) { - iptRulesFree(ctx->input_filter); - iptRulesFree(ctx->forward_filter); - iptRulesFree(ctx->nat_postrouting); + if (ctx->input_filter) + iptRulesFree(ctx->input_filter); + if (ctx->forward_filter) + iptRulesFree(ctx->forward_filter); + if (ctx->nat_postrouting) + iptRulesFree(ctx->nat_postrouting); free(ctx); } --

1 0

[Libvir] [patch 0/5] Misc. fixes to iptables support
by Mark McLoughlin 21 Mar '07

21 Mar '07

Hey, What follows is a few misc patches for qemud's iptables support. The main point is to add reload-on-HUP as suggested by danpb. Comments welcome .. Cheers, Mark. --

1 0

[Libvir] A whole tonne of networking fixes / enhancements
by Daniel P. Berrange 21 Mar '07

21 Mar '07

I've been testing the networking support and found various bugs / missing features that I thought we really need to have in the release - some of them impact the XML so we have to get this right now. - Build up in memory linked-list of network devices & disk devices in same order as they are listed in XML. Currently they are built up in reversed order, which makes the XML not be idempotent, and also means that if you have multiple NICs, what you think is eth0 ends up being eth4 and what you think is eth4 is eth0. This patch fixes the ordering to match XML. - Set the 'vlan' attribute in command line args to QEMU. This ensures that separate network devices are in fact separated. Previously if you had multiple NICs, QEMU connected them all to the same VLAN so any traffic on one NIC got sent to all NICs. Most definitely not what you want in any usual scenario, and created a traffic storm from the resultant network loops ! - Added support for networking of type='bridge'. This gives parity with equivalent Xen networking, eg <interface type='bridge'> <source dev='xenbr0'/> <target dev='vnet3'/> </interface> Will create a tap device called vnet3 and bridge it into xenbr0 - Added support for networking of type='ethernet'. This give parity with equivlent Xen networking, eg <interface type='ethernet'> <script path='/etc/qemu-ifup'/> <target dev='vnet5'/> </interface> Will create a tap device called vnet5 and run 'qemu-ifup' to setup its configuration. Think the various non-bridge Xen networking configs. - Added support for 'client', 'server', 'mcast' networking types. These are QEMU specific types, allowing unprivileged (or privileged) users to create virtual networks without TAP devices. eg two machines, one with <interface type='server'> <source address="127.0.0.1" port="5555"/> </interface> And the other with <interface type='client'> <source address="127.0.0.1" port="5555"/> </interface> Or both using multicast: <interface type='mcast'> <source address="230.0.0.1" port="5558"/> </interface> Both these options allow QEMU instancs on different physical machines to talk to each other. The multicast protocol is also compatible with the UserModeLinux multicast protocol. - Fix the 'type=network' config use the <target dev='vnet3'> element instead of a custom tapifname=vnet3 attribute - this gives consistent way to name tap devices that - most importantly - matches the Xen XML format for specifying vifname, eg <interface type='network'> <source network='default'/> <target dev='vnet2'/> </interface> Will create a tapdevice called vnet2 and connect it to the bridge device associated with the network 'default'. - Removed references to 'vde' - we're not using this explicitly - at some time in the future, we'll perhaps use VDE for doing virtual networking for unprivileged users where bridge devices are not available - Removed references to 'tap' network type - this is basically handled by the 'ethernet' network type to give XML compatability with the same functionality in the Xen backend. - The virtual network configuration currently always adds whole bunch of IPTables rules to the FORWARD/POSTROUTING chain which allow traffic from the virtual network to be masqueraded out through any active physical interface. This may be correct thing todo for the default network, but we also need the ability to create totally isolated networks (no forwarding at all), or directed networks (eg forwarding to an explicit physical device). To deal with this scenario I introduce a new element in the network XML called '<forward>'. If this is not present, then no forwarding rules are added at all. If it is present, but with no attributes then a generic rule allowing forwarding to any interface is added. If it is present and the 'dev' attribute is specific then forwarding is only allowed to that named interface. The default network XML thus now includes <forward/> So that by default we have a virtual network connected to all physical devices. - MAC addreses were not be autogenerated inside libvirt_qemud. If you don't provide a MAC address, QEMU (insanely) uses a hardcoded default. So all NICs end up with an identical MAC. We now always autogenerate a MAC address if not explicitly listed in XML. One final thing to be aware of - the Fedora Core 6 Xen kernel currently has totally fubar TCP checksum offland. So if you try to bridge a Xen guest into the libvirt virtual networking, it'll fail to get a DHCP address from dnsmasq. Even if you fix that by turning off TX checksums in Dom0, you'll get checksum failures for the actual TCP data transmission too. The only solution is to either upgrade the Dom0 kernel to a RHEL-5 vintage, or to also turn off checksumming in the guest. A new FC6 xen kernel is in the works which should hopefully fix this for real. With fixed kernel, I can easily setup virtual networks connecting both Xen PV, Xen FV and QEMU instances together. Anyway, the upshot of all this, is that we can now trivially create really complicated & fun networking layouts across QEMU & Xen, using a mixture of bridging, NAT, isolated LANs, and tunnelled VLANS :-) We really ought to document it a little though Since Mark is off on vacation for a while, I'd appreciate people taking a close look at this / actually giving it a try if you can. It is possible to create a totally isolated network using <network> <name>private</name> <uuid>d237ce44-8efa-452c-b8e6-1ae9cf53aeb1</uuid> <bridge name="virbr0" /> <ip address="192.168.122.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.122.2" end="192.168.122.254" /> </dhcp> </ip> </network> And a QEMU guest with 5 (yes, 5) network cards <domain type='qemu'> <name>QEMUFirewall</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory>219200</memory> <currentMemory>219200</currentMemory> <vcpu>1</vcpu> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> </os> <devices> <emulator>/usr/bin/qemu</emulator> <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <target dev='hda'/> </disk> <interface type='network'> <source network='private'/> <target dev='vnet1'/> </interface> <interface type='bridge'> <source dev='xenbr0'/> <target dev='vnet2'/> </interface> <interface type='ethernet'> <script path='/etc/dan-test-ifup'/> <target dev='vnet3'/> </interface> <interface type='server'> <source address="127.0.0.1" port="5555"/> </interface> <interface type='mcast'> <source address="230.0.0.1" port="5558"/> </interface> <graphics type='vnc' port='-1'/> </devices> </domain> In this XML, only eth1 is connected to the hosts public facing network. The other NICs are all on various private networks. So this QEMU guest is in essence a router/firewall box. eg, you could connected various other guests to the 'private' virtual network, and the only way they could reach the outside world is via this QEMU instance doing routing. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

3 2

[Libvir] Turn on compiler buffer checks
by Daniel P. Berrange 21 Mar '07

21 Mar '07

Since testing the latest Fedora RPMs I've hit a number of buffer overflow issues which were caught by the extra compiler checks Fedora turns on. It would be much better if we caught these before release, so the attached patch modifies the configure script so that the following options are always turned on if the compiler supports them: -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -fasynchronous-unwind-tables Since all our production builds use these flags & the patch only enables them if the compiler has support I don't see any issue with having them turned on by default. In addition I altered the existing configure compiler flag checks so that every compiler flag we turned on is explicitly checked to see if supported by the current compiler rather than doing a crude $CC=gcc heuristic which doesn't take account of differing gcc version numbers. If we wanted super-extra paranoia we could also turn on -fstack-protector-all possibly only with --enable-compiler-warnings=maximum since it adds more non-trivial performance overhead Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

2 1

[Libvir] Fix for buffer overflow in network setup
by Daniel P. Berrange 20 Mar '07

20 Mar '07

There was a buffer overflow in teh code to deal with building command line arguments which was only exposed when the Fedora RPM builds uses the stack protector args to GCC. The attached patch increases the buffer size and uses snprintf() instead of sprintf(). BTW, if anyone is looking for a patch to write - removing all other calls to sprintf() would be a worthy task :-) Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

2 1

[Libvir] [PATCH] Add error message to virsh setmem/setmaxmem
by Masayuki Sunou 20 Mar '07

20 Mar '07

Hi Virsh setmem/setmaxmem doesn't output an error message when an invalid value is set to "bytes". This patch outputs an error message, when an invalid value is set as "bytes" of virsh setmem/setmaxmem. Signed-off-by: Masayuki Sunou <fj1826dm(a)aa.jp.fujitsu.com> Thanks, Masayuki Sunou. Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.66 diff -u -p -r1.66 virsh.c --- src/virsh.c 19 Mar 2007 14:20:30 -0000 1.66 +++ src/virsh.c 19 Mar 2007 23:29:14 -0000 @@ -1461,6 +1461,7 @@ cmdSetmem(vshControl * ctl, vshCmd * cmd bytes = vshCommandOptInt(cmd, "bytes", &bytes); if (bytes <= 0) { virDomainFree(dom); + vshError(ctl, FALSE, _("Invalid value of \"bytes\"")); return FALSE; } @@ -1504,6 +1505,7 @@ cmdSetmaxmem(vshControl * ctl, vshCmd * bytes = vshCommandOptInt(cmd, "bytes", &bytes); if (bytes <= 0) { virDomainFree(dom); + vshError(ctl, FALSE, _("Invalid value of \"bytes\"")); return FALSE; }

2 1

[Libvir] RPM upgrades interaction with default networking
by Daniel P. Berrange 20 Mar '07

20 Mar '07

I've been thinking about the tricky issue of RPM upgrades and how they interact with the new networking stuff and don't think we've currently got quite the optimal setup. The current approach is that 'make install' puts a default.xml file into /etc/libvirt/qemu/networks and also symlinks that file into the autostart directory. So if someone builds from tar.gz and does make install they'll get a default network config installed & turned on out of the box. This is fine for tar.gz scenario. When we build an RPM we also include the default.xml file in the /etc/libvirt/qemu/networks directory, as well as the autostart symlink. So anyone installing the libvirt RPM gets the default network, whether they're doing an upgrade or fresh install. This is reasonable for the new install, or the first time you upgrade to a new neworking-enabled libvirt. If you subsequently delete the default network, or turn off autostarting, then along comes the next libvirt RPM update and autostart gets turned back on, and/or the default network recreated. This will be considered to be rather unpleasant by many people because no matter what they do, they will always be given a virbr0, a dnsmasq process and a bunch of extra iptables rules whether they want them or not. So I think we need to figure out a way to deploy a default network out of the box, but at the same time ensure that if the turn it off / delete it, upgrades won't re-introduce it. Ideally if someone upgrades from FC6 -> FC7 they will also get the default network created (once only). One way we can address this is to put the default.xml into the docs directory /usr/share/doc/libvirt-X.Y.Z (or perhaps stuff is into a dir like /usr/share/libvirt instead) and then have a RPM %post script which copies it into /etc/libvirt/qemu/networks. If we make the %post script conditional on '$1 == 1' then it will only be run for completely new libvirt installs. This doesn't address the upgrade question though - so someone updating from FC6 -> FC7 won't get the default network. Perhaps we shouldn't worry about them ? Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

4 5

[Libvir] [PATCH] Check calls to printf-like functions (and some fixes)
by Richard W.M. Jones 20 Mar '07

20 Mar '07

* Check calls to printf-like functions (and some fixes) * Fix qemudDebug when debug not enabled * Fix TODO so it doesn't supply a semi-colon (and all calls to this macro) * Add vim/emacs local variables in several places AFAICS this doesn't break the Python API build script, which was the objection to Jim Meyerling's previous patch to do much the same (http://www.redhat.com/archives/libvir-list/2006-March/msg00065.html) Rich.

3 6

[Libvir] more printf-style function checking
by Jim Meyering 19 Mar '07

19 Mar '07

I had the first hunk of the following patch laying around, but when I updated/compiled today, it triggered a warning: virsh.c:3162: warning: format '%d' expects type 'int', but argument 6 has type 's ize_t' I've fixed it by using %lu, which is more likely to be the same width as size_t that %d, and by casting strlen's return val to the new, expected type. 2007-03-19 Jim Meyering <jim(a)meyering.net> Avoid printf-style format mismatch. * src/virsh.c (vshError): Use ATTRIBUTE_FORMAT. (_vshStrdup): Use %lu (not %d) for strlen, and cast the strlen return value to "unsigned long" to match the format. Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.65 diff -u -p -r1.65 virsh.c --- src/virsh.c 19 Mar 2007 09:46:13 -0000 1.65 +++ src/virsh.c 19 Mar 2007 14:03:10 -0000 @@ -179,8 +179,8 @@ typedef struct __vshControl { static vshCmdDef commands[]; -static void vshError(vshControl * ctl, int doexit, const char *format, - ...); +static void vshError(vshControl * ctl, int doexit, const char *format, ...) + ATTRIBUTE_FORMAT(printf, 3, 4); static int vshInit(vshControl * ctl); static int vshDeinit(vshControl * ctl); static void vshUsage(vshControl * ctl, const char *cmdname); @@ -3158,8 +3158,8 @@ _vshStrdup(vshControl * ctl, const char if ((x = strdup(s))) return x; - vshError(ctl, TRUE, _("%s: %d: failed to allocate %d bytes"), - filename, line, strlen(s)); + vshError(ctl, TRUE, _("%s: %d: failed to allocate %lu bytes"), + filename, line, (unsigned long)strlen(s)); return NULL; }

2 1