5:24 a.m.
On Fri, 2018-07-27 at 13:19 -0400, John Ferlan wrote:
You shouldn't drop libvir-list when you reply...
Sorry, I
dropped it by mistake.
On 07/27/2018 06:35 AM, skobyda(a)redhat.com wrote:
> On Mon, 2018-07-23 at 17:32 -0400, John Ferlan wrote:
> >
> > On 07/12/2018 09:10 AM, Simon Kobyda wrote:
> > > XML shmem name will not include character '/', and will not be
> > > equal to strings
> > > "." or "..", as shmem name is used in a path.
> >
> > Validate that the provided XML shmem name is not directory
> > specific
> > "."
> > or ".." names as well as ensuring that there is no path separator
> > '/'
> > in
> > the name.
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> > > ---
> > >
> > > Changes in V2
> > > - Added error reports
> > > - Error situation will happen only if shmem name is
> > > equal to
> > > "." or "..", however their occurence in a name
> > > compromised of
> > > more
> > > characters is allowed.
> > >
> > > src/conf/domain_conf.c | 22 ++++++++++++++++++++++
> > > 1 file changed, 22 insertions(+)
> > >
> >
> > I believe this actually belongs in
> > virDomainDeviceDefValidateInternal
> > for case VIR_DOMAIN_DEVICE_SHMEM.
> > Also, should the docs/schemas/domaincommon.rng be modified?
> > Currently
> > it
> > has:
> >
> > <define name="shmem">
> > <element name="shmem">
> > <attribute name="name">
> > <data type="string">
> > <param name="pattern">[^/]*</param>
> > </data>
> >
>
> Is it a good idea? To create such regular expression, I believe it
> would make it unreadable for another person, ergo useless. I don't
> want
> it to end up as IPv6. I've benn told that actual parser can be, and
> sometimes is stricter than scheme.
>
Well this is what I was hoping others would be able to chime in on.
Hence why you ask on list. I don't disagree that being stricter in
Parse
is fine. Doing regexes is like reading a foreign language to me at
times. Some just don't make sense.
Yeah, that's why I don't want to try to insert complicated regex in
docs/schemas/domaincommon.rng.
I think that regex is indicating - any character except '/', but I'm
not
sure. If it is, it's ironic that we have to check for '/'
specifically
since it shouldn't be passing the xml validation test - OK at least
if
one was editing via virsh.
John
You are right about what that regex does. But from what I heard, that
functionality is rather new to libvirt, so users have option to bypass
that verification. So I would like to also leave it in code in case
users try to bypass that regex verification.
Simon.
> > Consider how other names are limited in their scope. The
> > basictypes.rng
> > has a number of examples.
> >
> > Naturally, the problem with changing it is that someone somewhere
> > will
> > complain, but libvirt used to accept this other format. Right now
> > I
> > would think the scope a bit too broad.
>
> But then we cannot fix most of the bugs, since there could always
> be
> some user which relies on bug behavior.
> >
> > If we are to limit the name we should also document in
> > docs/formatdomain.html.in that the shmem name is "limited" in
> > name to
> > avoid the '/' character, ".", and "..".
> >
> > BTW: My regex isn't that good, but it would seem '/' is an
> > invalid
> > character by XML standards even though the code never checked for
> > it.
> > Using virt-xml-validate <file> <schema> would "validate"
whether
> > someone
> > provides valid XML.
> >
> >
> > John
> >
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index 7ab2953d83..6b34c17de4 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> > > @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const
> > > virDomainDef *def)
> > > static int
> > > virDomainDefValidateInternal(const virDomainDef *def)
> > > {
> > > + size_t i;
> > > +
> > > if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
> > > return -1;
> > >
> > > @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const
> > > virDomainDef *def)
> > > return -1;
> > > }
> > >
> > > + for (i = 0; i < def->nshmems; i++) {
> > > + if (strchr(def->shmems[i]->name, '/')) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("shmem name cannot include '/'
> > > character"));
> > > + return -1;
> > > + }
> > > +
> > > + if (STREQ(def->shmems[i]->name, ".")) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("shmem name cannot be equal to
> > > '.'"));
> > > + return -1;
> > > + }
> > > +
> > > + if (STREQ(def->shmems[i]->name, "..")) {
> > > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > > + _("shmem name cannot be equal to
> > > '..'"));
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > if (virDomainDefLifecycleActionValidate(def) < 0)
> > > return -1;
> > >
> > >