Re: [PATCH 5/6] tools: add a dedicated header file for barrier definitions

[Jürgen Groß <jgross@xxxxxxxx>]

On 26.11.24 18:28, Anthony PERARD wrote:
> On Wed, Oct 23, 2024 at 03:10:04PM +0200, Juergen Gross wrote:
> > diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
> > new file mode 100644
> > index 0000000000..62036f528b
> > --- /dev/null
> > +++ b/tools/include/xen-barrier.h
> > @@ -0,0 +1,51 @@
> > +/******************************************************************************
> > + * xen-barrier.h
> > + *
> > + * Definition of CPU barriers, part of libxenctrl.
> Does it needs to be part of "libxenctrl" ? :-) Since the goal is to be
> able to use the header without xenctrl.
Yeah, but it was part of libxenctrl until now.

So it needs to be distributed together with xenctrl.h, otherwise we'd
need another distribution unit libxenctrl has to depend on.

> > + *
> > + * Copyright (c) 2003-2004, K A Fraser.
> I'm not sure this copyright line is enough, looking at `git blame`.
>
> Keir introduce xen_barrier macro in 2012, in
> 8d3f757328e1 ("libxc: Update rmb/wmb for x86.")
> Stefano introduced the Arm macro in 2012, in daa314fe1938 ("arm: compile
> libxc"), and Ian in 2013 in ae4b6f29a983 ("tools: libxc: arm64
> support").
> There's been a modification by Andrew in 2020, so Citrix copyright,
> in de16a8fa0db7 ("x86: Use LOCK ADD instead of MFENCE for smp_mb()").
>
> So overall, we probably want:
> Copyright (C) 2003-2012, K A Fraser.
> Copyright (C) 2012-2020 Citrix Systems, Inc.
Hmm, I just copied the Copyright from xenctrl.h, where I took the contents
of the new header from.

I can see a reason for adding the Copyright for Citrix, but IMHO this
_should_ have happened when the changes were applied in 2020. And I'm feeling
a little bit uneasy to add a Copyright for a company I'm not working for. How
do I really know Citrix is wanting that Copyright to be added?

> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation;
> > + * version 2.1 of the License.
> > + *
> > + * This library 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
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef XENBARRIER_H
> > +#define XENBARRIER_H
> With an extra '_' for the '-' in the header filename?
>      XEN_BARRIER_H
Okay.

> Otherwise, the rest of the patch looks fine to me, even without the rest
> of the series.
Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature