Re: [PATCH v2] tools/xl: fix channel configuration setting

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

On 28.02.25 18:34, Anthony PERARD wrote:
> On Thu, Feb 27, 2025 at 04:10:25PM +0100, Jürgen Groß wrote:
> > On 26.02.25 16:55, Anthony PERARD wrote:
> > > On Tue, Feb 25, 2025 at 08:30:33AM +0100, Juergen Gross wrote:
> > > > Channels work differently than other device types: their devid should
> > > > be -1 initially in order to distinguish them from the primary console
> > > > which has the devid of 0.
> > > >
> > > > So when parsing the channel configuration, use
> > > > ARRAY_EXTEND_INIT_NODEVID() in order to avoid overwriting the devid
> > > > set by libxl_device_channel_init().
> > > >
> > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > This might want a:
> > >       Fixes: 3a6679634766 ("libxl: set channel devid when not provided by 
> > > application")
> > >
> > > Before that, the devid set by `xl` was probably ignored. I think before,
> > > the console devid would be taken from libxl__init_console_from_channel()
> > > parametters, so the first devnum would be `0+1`, so never 0.
> > >
> > > Do you agree?
> > I'm not sure I do. The use of ARRAY_EXTEND_INIT() in xl_parse.c predates
> > commit 3a6679634766, so there is certainly more than one potential Fixes:
> > candidate.
> Of course, this was my first candidate as well, but that's part of the
> original introduction of channel to the source code, so it didn't really
> make sense to that the feature was commited with devid=0, surely it
> would already been an issue at the time. This is why I start looking
> in libxl on how devid was used.
>
> > So at least for the xl case commit 3a6679634766 isn't relevant, and my patch
> > is fixing the xl case only.
> Last time I check, `xl` is using libxl, so any change to libxl can
> impact xl.
Of course, but with xl devid was never -1, so 3a6679634766 shouldn't make
a difference.

> Before 3a6679634766, channel#0 would be connected to console#1 because
> libxl__init_console_from_channel() was called with dev_num=`channel#+1`,
> and $dev_num was used to set console.devid.
>
> After 3a6679634766, we have console.devid=channel.devid=dev_num if
> channel.devid is -1, or console.devid=channel.devid.
>
> Without 3a6679634766, having more than one channel with devid=-1 would
> result in failing to start due to having more than one channel with the
> same devid.
Do you have any prove that more than one channel ever worked?

> So the only good candidate is:
> Fixes: 3a6679634766 ("libxl: set channel devid when not provided by 
> application")
In the end I don't feel strong with this. If you really want, I can add
this Fixes tag.

> Also, I'm not sure channels are going to work properly if there's more
> than one console, lots of code seems to assume
> console.devid=channel.devid+1, and there's probably other issues.
I believe channels are not tested a lot.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature