[PATCH wayland v2] Remove protocol/wayland.dtd

[Peter Hutterer]

On Fri, Oct 16, 2015 at 11:42:21AM +0300, Pekka Paalanen wrote:
> On Fri, 16 Oct 2015 12:29:11 +1000
> Peter Hutterer <peter.hutterer at who-t.net> wrote:
> 
> > On Fri, Oct 09, 2015 at 01:16:49PM +0200, Nils Chr. Brause wrote:
> > > Hi,
> > > 
> > > Reviewed-by: Nils Christopher Brause <nilschrbrause at googlemail.com>
> > > 
> > > I ran distcheck and it worked. :)
> > 
> > a bit late, but I would like to register my disagreement with this patch :)
> > 
> > Having the DTD is a much simpler and less bug-prone description of what the
> > protocol should look like. Having the scanner define the protocol means the
> > protocol is whatever the current version of the scanner supports, which is
> > not a good contract.
> 
> Hi Peter,
> 
> can't argue with that. It's just that the DTD was unused since
> 6292fe2af6a45decb7fd39090e74dd87bc4e22b2, Feb 2014.
> 
> > I'd argue for reverting this and fixing any mismatch if there is one. And
> > using the DTD to validate before the scanner even runs.
> 
> We talked in IRC about this, and came to the conclusion that maybe we
> could have wayland-scanner invoke a validity checker against a DTD or
> an XSD.

I played around with that. As a quick basic solution we can hook validation
with libxml2 into the scanner and print a warning if the xml doesn't
validate. That's less than 50 LOC and won't break things since it doesn't
touch the actual parsing. and it won't break existing protocols that use
"creative" tags, all it will do is warn, not fail. See the diff below (after
reverting 06fb8bd37.

it's an ugly solution though, but without scanner tests probably the best
thing we can do.

> If the original objection to a DTD was because it required manually
> writing a lint phase in every project build system using the XML files,
> then having wayland-scanner invoke the check automatically solves that.

the question that remains though is: the dtd must be an external file for
extensions to be validated. Which means we need to either pass the dtd as
argument to the scanner (requires makefile changes everywhere), or we
hardcode the path into wayland-scanner (issues with running the scanner from
within the source tree) or we add it as variable to pkgconfig (requires
makefile changes again). any other solutions to fix this are welcome.
even if all we do is call out to xmllint we still run into that issue.

Cheers,
   Peter

> Then it becomes a question of how to do that and what tools or
> libraries to rely on. Running an external lint program from scanner.c
> could be a start.
>
> Migrating scanner.c from expat to some validating XML parser library
> would be a big enough change that I would like to see some scanner
> tests written before that.
>
> I think this summarises our IRC discussion for the mailing list.


diff --git a/Makefile.am b/Makefile.am
index 9114d98..cd4d6b5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -24,8 +24,8 @@ pkgconfig_DATA =
 
 bin_PROGRAMS = wayland-scanner
 wayland_scanner_SOURCES = src/scanner.c
-wayland_scanner_CFLAGS = $(EXPAT_CFLAGS) $(AM_CFLAGS)
-wayland_scanner_LDADD = $(EXPAT_LIBS) libwayland-util.la
+wayland_scanner_CFLAGS = $(EXPAT_CFLAGS) $(LIBXML_CFLAGS) $(AM_CFLAGS)
+wayland_scanner_LDADD = $(EXPAT_LIBS) $(LIBXML_LIBS) libwayland-util.la
 pkgconfig_DATA += src/wayland-scanner.pc
 
 if USE_HOST_SCANNER
diff --git a/configure.ac b/configure.ac
index ef26929..6630fbb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -105,6 +105,8 @@ PKG_CHECK_MODULES(EXPAT, [expat], [],
 	 AC_SUBST(EXPAT_LIBS)
 	])
 
+PKG_CHECK_MODULES(LIBXML, [libxml-2.0])
+
 AC_PATH_PROG(XSLTPROC, xsltproc)
 AM_CONDITIONAL([HAVE_XSLTPROC], [test "x$XSLTPROC" != "x"])
 
diff --git a/src/scanner.c b/src/scanner.c
index f456aa5..3685f7f 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -34,6 +34,9 @@
 #include <expat.h>
 #include <getopt.h>
 #include <limits.h>
+#include <unistd.h>
+
+#include <libxml/parser.h>
 
 #include "wayland-util.h"
 
@@ -59,6 +62,39 @@ usage(int ret)
 	exit(ret);
 }
 
+static int
+validate(void)
+{
+	int rc = 0;
+	xmlParserCtxtPtr ctx = NULL;
+	xmlDocPtr doc = NULL;
+	xmlDtdPtr dtd = NULL;
+	xmlValidCtxtPtr	dtdctx;
+
+	dtdctx = xmlNewValidCtxt();
+
+	ctx = xmlNewParserCtxt();
+	if (!ctx)
+		return 1;
+
+	dtd = xmlParseDTD(NULL, (const xmlChar*)"protocol/wayland.dtd");
+	if (!dtd)
+		abort();
+
+	doc = xmlCtxtReadFd(ctx, 0, "blah", NULL, 0);
+	if (!doc)
+		rc = 1;
+
+	rc = !xmlValidateDtd(dtdctx, doc, dtd);
+
+	xmlFreeDoc(doc);
+	xmlFreeParserCtxt(ctx);
+	xmlFreeValidCtxt(dtdctx);
+
+	lseek(0, 0, SEEK_SET);
+	return rc;
+}
+
 #define XML_BUFFER_SIZE 4096
 
 struct location {
@@ -1547,6 +1583,16 @@ int main(int argc, char *argv[])
 	ctx.protocol = &protocol;
 	ctx.loc.filename = "<stdin>";
 
+	/* validate the XML */
+	if (validate() != 0) {
+		fprintf(stderr,
+			"*******************************************************\n"
+			"*                                                     *\n"
+			"* WARNING: XML failed validation against the DTD      *\n"
+			"*                                                     *\n"
+			"*******************************************************\n");
+	}
+
 	/* create XML parser */
 	ctx.parser = XML_ParserCreate(NULL);
 	XML_SetUserData(ctx.parser, &ctx);