On 11/28/2012 05:55 PM, Laine Stump wrote: > On 11/28/2012 04:19 AM, Martin Kletzander wrote: >> On 11/28/2012 06:08 AM, Laine Stump wrote: >>> This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=879473 >>> >>> The name attribute is required for portgroup elements (yes, the RNG >>> specifies that), and there is code in libvirt that assumes it is >>> non-null. Unfortunately, the portgroup parsing function wasn't >>> checking for lack of portgroup. One adverse result of this was that >>> attempts to update a network by adding a portgroup with no name would >>> cause libvirtd to segfault. For example: >>> >>> virsh net-update default add portgroup "<portgroup default='yes'/>" >>> >>> This patch causes virNetworkPortGroupParseXML to fail if no name is >>> specified, thus avoiding any later problems. >> Looking at the code, I see it's required on more places, yes. And >> according to the documentation the name is needed. >> >>> --- >>> src/conf/network_conf.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >>> index 228951d..6ce2e63 100644 >>> --- a/src/conf/network_conf.c >>> +++ b/src/conf/network_conf.c >>> @@ -1175,6 +1175,12 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, >>> >>> /* grab raw data from XML */ >>> def->name = virXPathString("string(./@name)", ctxt); >>> + if (!def->name) { >>> + virReportError(VIR_ERR_XML_ERROR, "%s", >>> + _("Missing required name attribute in portgroup")); >>> + goto error; >>> + } >>> + >>> isDefault = virXPathString("string(./@default)", ctxt); >>> def->isDefault = isDefault && STRCASEEQ(isDefault, "yes"); >>> >>> >> Just a question; there's a similar check for (!def->name), for networks >> particularly, and that one uses VIR_ERR_NO_NAME (specified as a error >> for missing _domain_ name). Should one of these be changed (in a >> separate patch, of course)? > > According to the comment with the definition of VIR_ERR_NO_NAME, it's > intended only for when the name is missing from a domain. The error > printed out though is "missing name information" (with optional "in %s"). > > So if you look at the comment, it seems that it shouldn't be used for > network (or node device) name, but if you look at the message generated, > it looks like it could be used for any missing name. Personally I don't > see the use of having a separate error code for missing name vs. missing > [anything else]. To me it's just like any other XML error. > That's exactly what I meant, so you assured me that I haven't misunderstood the second appearance of that. Thanks, Martin -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list