Re: [Xen-devel] [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Mar 08, 2013 at 12:36:16PM +0000, Jan Beulich wrote:
> >>> On 08.03.13 at 13:28, Daniel Kiper <daniel.kiper@xxxxxxxxxx> wrote:
> > On Fri, Mar 08, 2013 at 11:52:21AM +0000, David Vrabel wrote:
> >> On 08/03/13 10:50, Daniel Kiper wrote:
> >> > On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel wrote:
> >> >> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> >>
> >> >> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
> >> >> kexec hypercall.  These new sub-ops allow a priviledged guest to
> >> >> provide the image data to be loaded into Xen memory or the crash
> >> >> region instead of guests loading the image data themselves and
> >> >> providing the relocation code and metadata.
> >> >>
> >> >> The old interface is provided to guests requesting an interface
> >> >> version prior to 4.3.
> >> >>
> >> >> Signed-off: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> >
> >> > [...]
> >> >
> >> >> diff --git a/xen/include/public/kexec.h b/xen/include/public/kexec.h
> >> >> index 61a8d7d..5259446 100644
> >> >> --- a/xen/include/public/kexec.h
> >> >> +++ b/xen/include/public/kexec.h
> >> >> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
> >> >>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
> >> >>   * image == relocation information for kexec (ignored for unload) [in]
> >> >>   */
> >> >> -#define KEXEC_CMD_kexec_load            1
> >> >> -#define KEXEC_CMD_kexec_unload          2
> >> >> -typedef struct xen_kexec_load {
> >> >> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300 
> >> >> */
> >> >> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300 
> >> >> */
> >> >> +typedef struct xen_kexec_load_v1 {
> >> >>      int type;
> >> >>      xen_kexec_image_t image;
> >> >> -} xen_kexec_load_t;
> >> >> +} xen_kexec_load_v1_t;
> >> >
> >> > I think that this is not good idea to redefine meaning of constants,
> >> > types, structures, etc. IMO it is comparable to redefining meaning
> >> > of words in any laguage (e.g. English). It will be very confusing
> >> > and may easily lead to stupid bugs. I think that old interface should
> >> > stay as is (with its bad behavior). New interface should be introduced
> >> > with "_v2" suffix, e.g. KEXEC_CMD_kexec_load_v2, ...
> >> > This would not confuse our descendants.
> >>
> >> This is something that was requested (by Ian C) as the Xen way of doing it.
> >
> > Yes, I remember but still do not agree with that idea in general.
> > Maybe discussion on kexec interface is good point to change
> > that Xen community behavior? Ian?
>
> Together with all consumers being expected to properly make
> use of __XEN_INTERFACE_VERSION__ (or else they get the
> lowest possible definitions), I don't think there's a big issue here
> as long as all old definitions retain their meaning when said
> symbol is set low enough.

It is good for dropping or adding some functionalities. However, I still
do not agree that it is sufficient solution in this case. Here we are
redefining interface. I know that was done many times but it does not
mean this is good. Additionally, compilter does not care. It reads everything.
However, for me (I suppose at least) it is difficult read code which have
same "words" with meaning depending only on placement. I would like to read
sources without thinking where I am. That is all.

Daniel

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