Re: [PATCH] [PATCH RFC v3] Implements Backend Program conventions for vhost-user-scsi

[Stefan Hajnoczi]

On Tue, Apr 05, 2022 at 07:22:38AM -0500, Sakshi Kaushik wrote:

Thanks for the patch! Comments below:

> Signed-off-by: Sakshi Kaushik <sakshikaushik717@gmail.com>
> ---
>  contrib/vhost-user-scsi/vhost-user-scsi.c | 76 +++++++++++++++--------
>  1 file changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c 
> b/contrib/vhost-user-scsi/vhost-user-scsi.c
> index 4f6e3e2a24..74ec44d190 100644
> --- a/contrib/vhost-user-scsi/vhost-user-scsi.c
> +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
> @@ -351,34 +351,58 @@ fail:
>  
>  /** vhost-user-scsi **/
>  
> +int opt_fdnum = -1;
> +char *opt_socket_path;
> +gboolean opt_print_caps;
> +char *iscsi_uri;

These variables can be declared static since they don't need to be
externally visible (they aren't used by any other .c files).

> +    if (opt_print_caps) {
> +        g_print("{\n");
> +        g_print("  \"type\": \"scsi\",\n");

The JSON grammar (https://www.json.org/json-en.html) does not allow
trailing commas in objects. Please remove the comma on this line.

(Some parses may accept the trailing comma but others may reject the
input as invalid JSON.)

> @@ -426,10 +450,12 @@ err:
>      goto out;
>  
>  help:
> -    fprintf(stderr, "Usage: %s [ -u unix_sock_path -i iscsi_uri ] | [ -h 
> ]\n",
> +    fprintf(stderr, "Usage: %s [ -s socket-path -i iscsi_uri -f fd -p 
> print-capabilities ] | [ -h ]\n",
>              argv[0]);
> -    fprintf(stderr, "          -u path to unix socket\n");
> +    fprintf(stderr, "          -s path to unix socket\n");
>      fprintf(stderr, "          -i iscsi uri for lun 0\n");
> +    fprintf(stderr, "          -f fd, file-descriptor\n");
> +    fprintf(stderr, "          -p denotes print-capabilities\n");
>      fprintf(stderr, "          -h print help and quit\n");

The vhost-user backend conventions only document the long options
("--socket-path", "--fd", "--print-capabilities"). It's okay to offer
short options too but please show the long options in the help output
since it's the standard interface and that will make it clear that this
program supports the vhost-user backend program conventions.

Something like:

  -s, --socket-path=SOCKET_PATH     path to unix socket

signature.asc
Description: PGP signature