Re: [libvirt] [PATCH] Forbid new-line char in name of networks

On Mon, Oct 10, 2016 at 04:53:15 +0200, Michal Privoznik wrote: > On 08.10.2016 23:38, Sławek Kapłoński wrote: > > New line character in name of network is now forbidden because it > > mess virsh output and can be confusing for users. > > > > Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064 > > --- > > src/conf/network_conf.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > > index aa39776..f9c537f 100644 > > --- a/src/conf/network_conf.c > > +++ b/src/conf/network_conf.c > > @@ -2123,6 +2123,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) > > goto error; > > } > > > > + if (strchr(def->name, '\n')) { > > + virReportError( > > + VIR_ERR_XML_ERROR, > > + _("name %s cannot contain new-line character"), def->name); Please note that this is not our expected style of breaking up arguments. > > + goto error; > > + } > > + > > /* Extract network uuid */ > > tmp = virXPathString("string(./uuid[1])", ctxt); > > if (!tmp) { > > > > Okay, this makes sense. We can put reasonable restrictions on object > names. However, I think while we are at this we can fix the same problem > in other areas of the code. For instance, storage pools and volumes > suffer from the same problem. Now, instead of copying this chunk all > over the places, what about doing this more wisely? > > For instance: > > char *c; > ... > /* Parse name from XML */ > > if ((c = strpbrk(def->name, "/\n")) { > /* Insert your favourite character here ^^ */ > virReportError(VIR_ERR_XML_DETAIL, > _("invalid char in name: %c"), *c); > return -1; /* goto cleanup */ > } > > This way, we can just add new character to the string when somebody > files a bug. > Moreover, if we'd wrap those couple of lines into a function, we could > just do: > > if (!virXMLCheckName(def->name, "/\n")) > return -1; > > Let me know if you have any troubles writing this, or if you disagree. The problem with making the parser stricter is that it's prone to create regressions for people that might have used such configuration. As this was previously working (except for the broken virsh interface) we should limit such names only when defining a network/vm/whatever. Peter

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list