From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 2/3] vmport: Add VMware provided include files. Date: Tue, 02 Sep 2014 14:46:44 -0400 Message-ID: <54061094.6050008@terremark.com> References: <1409585629-25840-1-git-send-email-dslutz@verizon.com> <1409585629-25840-3-git-send-email-dslutz@verizon.com> <54058F32020000780002FB2A@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <54058F32020000780002FB2A@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Don Slutz Cc: Tim Deegan , Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Andrew Cooper , Ian Jackson , xen-devel@lists.xen.org, Eddie Dong , Aravind Gopalakrishnan , Suravee Suthikulpanit , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 09/02/14 03:34, Jan Beulich wrote: >>>> On 01.09.14 at 17:33, 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 MERCHANTA= BILITY >> + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public Lic= ense >> + * for more details. >> + * >> + * You should have received a copy of the GNU General Public License al= ong >> + * with this program; if not, write to the Free Software Foundation, In= c., >> + * 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 = ): *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. =97 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 >