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

[Don Slutz <dslutz@verizon.com>]

On 09/03/14 03:51, Jan Beulich wrote:
>>>> On 02.09.14 at 20:46, <dslutz@verizon.com> wrote:
>> On 09/02/14 03:34, Jan Beulich wrote:
>>>>>> On 01.09.14 at 17:33, <dslutz@verizon.com> 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
>