Re: [PATCH v2 4/4] tools/xl: Add pcid daemon to xl

[Anthony PERARD <anthony.perard@citrix.com>]

On Wed, Jul 13, 2022 at 06:03:11PM +0300, dmitry.semenets@gmail.com wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Add pcid daemon (based on vchan-node2) implements pcid protocol. Protocol is
> OS independed and should work on ane supported OS.
> 
> Add essential functionality to handle pcid protocol:
> - define required constants
> - prepare for handling remote requests
> - prepare and send an error packet
> 
> pcid server used if domain has passthrough PCI controller and we wants
> assign some device to other domain.
> pcid server should be launched in domain owns the PCI controller and process
> request from other domains.
> 
> Message exchange imnplementation based on JSON via libvchan. Supported
> messages:
> - make_assignable
> - revert_assignable
> - is_device_assigned
> - resource_list
> - reset_device
> - write_bdf
> 
> Signed-off-by: Dmytro Semenets <dmytro_semenets@epam.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Hi Dmitry,

I don't think libxl is the right place to implement a daemon, and xl
isn't the place to start a new daemon either. Could you look into
creating a new binary which implements only "pcid"?

I understand that making use of the facilities in libxl makes writing
this daemon a bit easier, but still I don't think the code belong to
libxl. Maybe some of libxl's code could be moved to libxlu (utils) if
that help, but I'm not sure.

Also, this patch is way to big, makes too many changes, including
changes to existing libxl's API which we try to avoid. (Adding new
functions is fine, changing the prototype of existing one is what we try
to avoid.)

As for the protocol, it might be better to have the description
somewhere in "docs/" instead of within a C header. I'm not sure which
sub-directory as we have protocols in different one, like in "misc" or
"design" or "features", but "misc" might be the one. Also, the document
should say somewhere that the protocol is based on JSON, as that's
missing in pcid.h.

Thanks,

-- 
Anthony PERARD