[PATCH] Cache xkbcomp output for fast start-up v2

[Li, Yan]

On Wed, Apr 01, 2009 at 11:40:02AM +1000, Peter Hutterer wrote:
> On Mon, Mar 30, 2009 at 08:03:25PM +0800, Li, Yan wrote:
<snip>

> this should be ${localstatedir}/cache/xkb instead.

Yes.

> note that when you're doing this change, you also need to remove the part that
> checks for XKM_OUTPUT_DIR to be an absolute directory (in configure.ac)

Why? Is it because that XKB_BASE_DIRECTORY is no longer the
recommended location for storing XKB output?

OK, in my new patch I refused to go on if detected XKBOUTPUT is not an
absolute path.

> >  #include <ctype.h>
> >  #define	NEED_EVENTS 1
> > @@ -58,10 +64,10 @@ THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >  	 * Making the server write to a subdirectory of that directory
> >  	 * requires some work in the general case (install procedure
> >  	 * has to create links to /var or somesuch on many machines),
> > -	 * so we just compile into /usr/tmp for now.
> > +	 * so we just compile into /var/cache/xkb for now.
> >  	 */
> >  #ifndef XKM_OUTPUT_DIR
> > -#define	XKM_OUTPUT_DIR	"compiled/"
> > +#define	XKM_OUTPUT_DIR	"/var/cache/xkb"
> >  #endif
> 
> this part can be removed now. XKM_OUTPUT_DIR is always defined through
> configure.ac.

OK.

<snip>
> > -    char	*buf = NULL, keymap[PATH_MAX], xkm_output_dir[PATH_MAX];
> > +    char *	buf = NULL, xkmfile[PATH_MAX], xkmOutputDir[PATH_MAX];
> 
> any particular reason why you renamed xkm_output_dir to xkmOutputDir?
> it fits better with the rest of the variable naming, but it's not really
> necessary.

OK.  That's gratuitous...

BTW, what's the policy towards inconsistent coding style?  I'd like to
correct some of them I found in ddxLoad.c, such as using tabs for
indention, not using camelCase naming, etc, in a standalone patch
against the tip.  Will this kind of patch be accepted?

<snip>
> > +        ErrorF("[xkb] open xkbKeyMapBuf for writting\n");
> typo: "writing", not "writting".

Oops.

<snip> 
> > +    LogMessage(X_INFO, "[xkb] xkmfile %s ", canonicalXkmfileName);
> > +    if (access(canonicalXkmfileName, R_OK) == 0) {
> > +        /* yes, we can reuse old xkb file */
> > +        LogMessage(X_INFO, "reused\n");
> > +        xfree(canonicalXkmfileName);
> > +        return True;
> > +    }
> > +    LogMessage(X_INFO, "is being compiled\n");
> > +    xfree(canonicalXkmfileName);
> > +
> > +    /* continue to call xkbcomp to compile the keymap */
> >  
> 
> This leaves an awkward log message. 
> (II) [xkb] xkmfile /foo/server-...  (II) is being compiled
> Just print it as one log message, and i'd replace [xkb] with xkb: which is
> more coeherent with the rest of the log. the [xkb] format with square brackets
> is more used for ErrorF.

OK, will correct it.

> >  	else
> > -	    LogMessage(X_ERROR, "Error compiling keymap (%s)\n", keymap);
> > +            LogMessage(X_ERROR, "Error compiling keymap (%s)\n", xkbfile);
> 
> tab/spaces mixup

My new line is correctly using spaces only. The old line used TABs.

> >  #ifdef WIN32
> >          /* remove the temporary file */
> >          unlink(tmpname);
> > @@ -375,7 +466,6 @@ unsigned	missing;
> >  	DebugF("Loaded XKB keymap %s, defined=0x%x\n",fileName,(*xkbRtrn)->defined);
> >      }
> >      fclose(file);
> > -    (void) unlink (fileName);
> >      return (need|want)&(~missing);
> >  }
> >  
> > diff --git a/xkb/xkbfmisc.c b/xkb/xkbfmisc.c
> > index ae752e9..5abf3c7 100644
> > --- a/xkb/xkbfmisc.c
> > +++ b/xkb/xkbfmisc.c
> > @@ -293,15 +293,27 @@ unsigned	wantNames,wantConfig,wantDflts;
> >      multi_section= 1;
> >      if (((complete&XkmKeymapRequired)==XkmKeymapRequired)&&
> >  	((complete&(~XkmKeymapLegal))==0)) {
> > -	fprintf(file,"xkb_keymap \"%s\" {\n",name);
> > +        if (fprintf(file,"xkb_keymap \"%s\" {\n",name) <= 0)
> > +        {
> > +            ErrorF("[xkb] writting XKB Keymap\n");
> > +            return False;
> > +        }
> >      }
> >      else if (((complete&XkmSemanticsRequired)==XkmSemanticsRequired)&&
> >  	((complete&(~XkmSemanticsLegal))==0)) {
> > -	fprintf(file,"xkb_semantics \"%s\" {\n",name);
> > +        if (fprintf(file,"xkb_semantics \"%s\" {\n",name)<=0)
> > +        {
> > +            ErrorF("[xkb] writting XKB Keymap\n");
> > +            return False;
> > +        }
> >      }
> >      else if (((complete&XkmLayoutRequired)==XkmLayoutRequired)&&
> >  	((complete&(~XkmLayoutLegal))==0)) {
> > -	fprintf(file,"xkb_layout \"%s\" {\n",name);
> > +        if (fprintf(file,"xkb_layout \"%s\" {\n",name)<=0)
> > +        {
> > +            ErrorF("[xkb] writting XKB Keymap\n");
> > +            return False;
> > +        }
> >      }
> >      else if (XkmSingleSection(complete&(~XkmVirtualModsMask))) {
> >  	multi_section= 0;
> 
> shouldn't this hunk be in a separate patch? this part has changed in master
> already anyway, so moving it into a separate patch would make it even easier
> to port your patch to master (mind you, it takes less than 5 min now)

OK, removed this part.

I'll send my v3 patch replying this mail.

-- 
Li, Yan