Re: [PATCH V2 1/2] libxl: virtio: Remove unused frontend nodes

[Anthony PERARD]

On Thu, May 11, 2023 at 01:20:42PM +0530, Viresh Kumar wrote:
> The libxl_virtio file works for only PV devices and the nodes under the

Well, VirtIO devices are a kind of PV devices, yes, like there's Xen PV
devices. So this explanation doesn't really make much sense.

> frontend path are not used by anyone. Remove them.

Instead of describing what isn't used, it might be better to describe
what we try to achieve. Something like:

Only the VirtIO backend will watch xenstore to find out when a new
instance needs to be created for a guest, and read the parameters
from there. VirtIO frontend are only virtio, so they will not do
anything with the xenstore nodes. They can be removed.


> While at it, also add a comment to the file describing what devices this
> file is used for.
> 
> Signed-off-by: Viresh Kumar 
> ---
> V2: New patch.
> 
>  tools/libs/light/libxl_virtio.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> index faada49e184e..eadcb7124c3f 100644
> --- a/tools/libs/light/libxl_virtio.c
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -10,6 +10,9 @@
>   * 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.
> + *
> + * This is used for PV (paravirtualized) devices only and the frontend isn't
> + * aware of the xenstore.

It might be more common to put this kind of comment at the top, that is
just before the "Copyright" line.

Also, same remark as for the patch description, VirtIO are PV devices,
so the comment is unnecessary. What is less obvious is why is there even
xenstore interaction with a VirtIO device?

Maybe a better description for the source file would be:

Setup VirtIO backend. This is intended to interact with a VirtIO
backend that is watching xenstore, and create new VirtIO devices
with the parameter found in xenstore. (VirtIO frontend don't
interact with xenstore.)


Thanks,

-- 
Anthony PERARD

[PATCH V2 1/2] libxl: virtio: Remove unused frontend nodes

[Viresh Kumar]

The libxl_virtio file works for only PV devices and the nodes under the
frontend path are not used by anyone. Remove them.

While at it, also add a comment to the file describing what devices this
file is used for.

Signed-off-by: Viresh Kumar 
---
V2: New patch.

 tools/libs/light/libxl_virtio.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
index faada49e184e..eadcb7124c3f 100644
--- a/tools/libs/light/libxl_virtio.c
+++ b/tools/libs/light/libxl_virtio.c
@@ -10,6 +10,9 @@
  * 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.
+ *
+ * This is used for PV (paravirtualized) devices only and the frontend isn't
+ * aware of the xenstore.
  */
 
 #include "libxl_internal.h"
@@ -49,11 +52,6 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, 
uint32_t domid,
 flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type));
 flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport));
 
-flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq));
-flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, virtio->base));
-flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type));
-flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
-
 return 0;
 }
 
-- 
2.31.1.272.g89b43f80a514