Re: [Xen-devel] [PATCH v2 2/3] vmport: Add VMware provided include files.

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 09/02/14 03:34, Jan Beulich wrote:
> > > > On 01.09.14 at 17:33, <dslutz@xxxxxxxxxxx> wrote:
> > These 2 files: backdoor_def.h and guest_msg_def.h come from:
> >
> > http://packages.vmware.com/tools/esx/3.5latest/rhel4/SRPMS/index.html
> >   open-vm-tools-kmod-7.4.8-396269.423167.src.rpm
> >    open-vm-tools-kmod-7.4.8.tar.gz
> >     vmhgfs/backdoor_def.h
> >     vmhgfs/guest_msg_def.h
> >
> > and are unchanged.
> Okay, I can the value in keeping them as is even if they're in
> conflict with some of our coding style rules. But ...
>
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/vmport/backdoor_def.h
> > @@ -0,0 +1,167 @@
> > +/* **********************************************************
> > + * Copyright 1998 VMware, Inc.  All rights reserved.
> > + * **********************************************************
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation version 2 and no later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> > + * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > + * for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write to the Free Software Foundation, Inc.,
> > + * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > + */
> > +
> > +/*
> > + * backdoor_def.h --
> > + *
> > + * This contains backdoor defines that can be included from
> > + * an assembly language file.
> > + */
> > +
> > +
> > +
> > +#ifndef _BACKDOOR_DEF_H_
> > +#define _BACKDOOR_DEF_H_
> ... these guards have the (theoretical at this point) risk of clashing
> (apart from violating the C standard's naming rules) and ...
Yes. That is why I had the comment:


/* Note: VMPORT_PORT and VMPORT_MAGIC is also defined as BDOOR_PORT
* and BDOOR_MAGIC in backdoor_def.h Defined in vmport.h also.
*/

because I did not want the reset of xen to be including this file.

If I have found the right "C standard's naming rule", this file is not
alone in violating it:

The following is the C standard's specification for naming the 
identifiers (C11 draft 
<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf>):
*7.1.3 Reserved identifiers*

Each header declares or defines all identifiers listed in its associated 
subclause, and optionally declares or defines identifiers listed in its 
associated future library directions subclause and identifiers which are 
always reserved either for any use or for use as file scope identifiers.
— All identifiers that begin with an underscore and either an uppercase 
letter or another underscore are always reserved for any use.


For example:


xen/arch/x86/boot/video.h:#ifndef __BOOT_VIDEO_H__
xen/arch/arm/vtimer.h:#ifndef __ARCH_ARM_VTIMER_H__
xen/arch/x86/cpu/mcheck/barrier.h:#ifndef _MCHECK_BARRIER_H
tools/xenstore/talloc.h:#ifndef _TALLOC_H_




> > +#define INCLUDE_ALLOW_MODULE
> > +#define INCLUDE_ALLOW_USERLEVEL
> > +#define INCLUDE_ALLOW_VMMEXT
> > +#define INCLUDE_ALLOW_VMCORE
> > +#define INCLUDE_ALLOW_VMKERNEL
> > +#include "includeCheck.h"
> ... the patch is obviously incomplete without this header, the name
> of which is certainly not in line with our way of naming files.
Yes. And an empty file of that name is added in the next patch.


So I am getting the feeling is that it would be better to have a patch
adjusting these include files to better fit in xen, and then move them
out of the new xen/arch/x86/hvm/vmware directory.

In any case I would continue with this patch to provide the history of
where the include file came from.

-Don Slutz
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel