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: Wed, 03 Sep 2014 08:38:28 -0400 Message-ID: <54070BC4.3030706@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> <54061094.6050008@terremark.com> <5406E4A50200007800030124@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5406E4A50200007800030124@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/03/14 03:51, Jan Beulich wrote: >>>> On 02.09.14 at 20:46, wrote: >> 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 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. > I can't connect your response to my comment. Reading it a again, I agree. s/reset/rest/ helps a little. Hope this is better: I put these files in the new vmware directory in order to prevent other parts of xen from attempting to use these include files. This is a work around for any possible issue with them. In v2 I had defined in a different file the 2 numbers that are used else where. It now looks like in v3 I can fix this by having all uses contained in this new directory. >> If I have found the right "C standard's naming rule", this file is not >> alone in violating it: > Yes, that's the one. And no, bad examples elsewhere aren't an > excuse to introduce more bad code. Fine with me. I was also attempting to understand how it would need to change. >>>> +#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. > Question is - why not here (to make the set self consistent)? I see no reason not to. So I have added it here. >> 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. > Not sure what makes you think so - as said earlier on, I can see > the value of having the file mostly (if not entirely) identical to > what is coming from VMware. That way, eventual future updates > are going to be easier. All we need to avoid are eventual future > naming clashes. Of course, if the headers are so badly written > that the majority of their defines has a risk of clashing, then of > course it might indeed be preferable to redo them in Xen style. You are the 2nd person to point out issues with these files. So I guess I was jumping the gun in saying to change them and move them. In any case if I was to do the smallest change to make them better that would be a 2nd patch. Thinking more about it, it makes more sense to leave them in the vmware directory. -Don Slutz > Jan >