Re: [libvirt] [PATCH-v4 2/2] Support for static routes on a virtual bridge

(I had thought there would be just a few tweaks to the documentation that I could squash in and push, but as I reviewed I found more issues, and don't have time to fix them myself. It is *very* close though, and the freeze for 1.0.5 is coming up (within a couple days, I think?), so if you can make the changes below, maybe we can get this into 1.0.5.) On 04/20/2013 03:45 PM, Gene Czarcinski wrote: The first two paragraphs are nearly equivalent, and their first sentences are also redundant with the first line of the comment. I've merged them into a single, more succinct introduction. (btw, note that "gateway" is an attribute, not a subelement): network: static route support for <network> This patch adds the <route> subelement of <network> to define a static route. the address and prefix (or netmask) attribute identify the destination network, and the gateway attribute specifies the next hop address (which must be directly reachable from the containing <network>) which is to receive the packets destined for "adress/(prefix|netmask)". This explanation is fine for this patch, but I'd still (separately) like a more detailed description of what makes the network unusable. Are we missing something when we tear down the network if we fail at this point? If so, we should fix our error exit code to do the right thing. I think that can be done in a separate patch though,. The documentation is meant to be a "snapshot of current behavior", not a historical narrative (with annotations indicating when functionality was introduced). Because of that, I think that "it has been possible to define..." style isn't appropriate. We can just start of with "Static route ....." and toss in a <since> at the end of the paragraph. That will be more consistent with the rest of the document. (This is all very good/useful information, but someday soon we need to move it out of this "reference" type file into a "admin guide" type file.) This paragraph is unnecessary, and I think would just serve to confuse, so it should be removed. s/rill/will/ "must be directly reachable via (on the same subnet as) one of the <ip> addresses defined for this <network>" <gateway> is mandatory. It doesn't make sense to have a route without a gateway (w/o address or prefix is fine though - that makes it a default route). address should be optional unless prefix or netmask is non-0, although I've now noticed that won't be handled properly due to virSocketAddrGetIpPrefix returning -1 when there is no address or prefix or netmask (I'm fixing that before I push that patch, so you can just toss your 1/2 patch, rebase, and assume it's fixed). How about structuring it like this: if (!gateway) { "Missing error" goto cleanup; } if (virSocketAddrPars(...) < 0) { "bad gateway error" goto cleanup; } gateway is required, so AF_UNSPEC is also unacceptable. I think you're going to end up needing the parsed netmask sooner (when you're trying to decide whether or not address is required), so you should probably do this parsing right after you get the netmask string from the xml. } else { You should only set def->prefix if the definition contains "prefix", not if you're deriving it from netmask, or getting the natural netmask based on network class. It will be useful to know the result of virSocketAddrGetIpPrefix during the parse, but only in order to decide whether or not "address" is optional; don't save it in the def. Extra space after "32" (make syntax-check catches this) address should be optional if prefix is 0. Another extra space. Again, you should only set def->prefix if the incoming xml has a prefix attribute; don't automatically set it (only do that when you're creating the commandline to add the route) "address '%s' in <route> definition of network '%s' is not a network address" "if the specified gateway is directly reachable from this bridge" Don't you just mean: if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway) != VIR_SOCKET_ADDR_FAMILY(&def2->address)) { continue; } This VIR_FREE() needs to go down to the end of the function so that it's always called, even on error returns. Remove the extra space before the "/" (don't forget to update the test cases accordingly) That comment just confuses - what is "real address"? (this is probably an artifact of earlier versions of the patch where you re-used <ip> for routes). Now that routes have their own list, that comment is redundant as well (the code self-documents) This is *very* close!