[virt-tools-list] [vhostmd PATCH 2/3] Optional global sections.

[Jim Fehlig]

On 11/25/19 11:32 AM, Michael Trapp wrote:
> From: d032747 <michael.trapp at sap.com>
> 
> Mark both global sections, disk and virtio, as optional DTD elements.
> Based on that, the configured transport can be either 'disk only' or
> 'virtio only' or 'disk and virtio'.
> But a configuration with disabled disk and disabled virtio is not valid.
> This condition can't be validated in the DTD and therefore it is checked
> in the vhostmd code.

This can be one paragraph IMO, so no need for the "But a configuration..." to 
start on a new line.

> 
> Due to the optional disk transport 'globals/disk/size' must be handled
> as optional element.

Perhaps we can refactor the code to avoid this. More on that below...

> 
> In addition a disabled disk transport should not result in an empty
> disk file. Because the 'initialized' disk file will never get valid
> metrics updates, which might be not that obvious for a client in a vm.
> ---
>   vhostmd.dtd       |  2 +-
>   vhostmd/vhostmd.c | 19 +++++++++++--------
>   2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/vhostmd.dtd b/vhostmd.dtd
> index 888270e..f58a74a 100644
> --- a/vhostmd.dtd
> +++ b/vhostmd.dtd
> @@ -9,7 +9,7 @@ Virtual Host Metrics Daemon (vhostmd). Configuration file DTD
>   -->
>   
>   <!ELEMENT vhostmd (globals,metrics)>
> -<!ELEMENT globals (disk,virtio*,update_period,path,transport+)>
> +<!ELEMENT globals (disk*,virtio*,update_period,path,transport+)>
>   
>   <!ELEMENT disk (name,path,size)>
>   <!ELEMENT name (#PCDATA)>
> diff --git a/vhostmd/vhostmd.c b/vhostmd/vhostmd.c
> index 7e29e6f..1dd739e 100644
> --- a/vhostmd/vhostmd.c
> +++ b/vhostmd/vhostmd.c
> @@ -90,7 +90,7 @@ typedef struct _mdisk_header
>   
>   /* Global variables */
>   static int down = 0;
> -static int mdisk_size = MDISK_SIZE_MIN;
> +static int mdisk_size = MDISK_SIZE_MIN * 256;

Unrelated change, or does the min size really need to be 256x larger? If it 
needs to be larger we should just change MDISK_SIZE_MIN.

>   static int update_period = 5;
>   static char *def_mdisk_path = "/dev/shm/vhostmd0";
>   static char *mdisk_path = NULL;
> @@ -586,10 +586,6 @@ static int parse_config_file(const char *filename)
>      if (vu_xpath_long("string(./globals/disk/size[1])", ctxt, &l) == 0) {
>         mdisk_size = vu_val_by_unit(unit, (int)l);
>      }
> -   else {
> -      vu_log(VHOSTMD_ERR, "Unable to parse metrics disk size");
> -      goto out;
> -   }

Perhaps this function can be refactored to first parse the transports, then 
conditionally parse disk and virtio if specified in transports?

>   
>      if (vu_xpath_long("string(./globals/update_period[1])", ctxt, &l) == 0) {
>         update_period = (int)l;
> @@ -616,6 +612,11 @@ static int parse_config_file(const char *filename)
>            virtio_expiration_time = (int)l;
>      }
>   
> +   if ((transports & (VIRTIO | VBD)) == 0) {
> +      vu_log(VHOSTMD_ERR, "Neither disk nor virtio activated as transport");
> +      goto out;
> +   }
> +

On a Xen host one could conceivably have <transport>xenstore</transport>, but 
that would not be allowed with this change. If no transport at all is specified, 
parse_transports() has

    if (transports == 0)
        transports = VBD;

Regards,
Jim

>      /* Parse requested metrics definitions */
>      if (parse_metrics(xml, ctxt)) {
>         vu_log(VHOSTMD_ERR, "Unable to parse metrics definition "
> @@ -1136,9 +1137,11 @@ int main(int argc, char *argv[])
>         goto out;
>      }
>   
> -   if ((mdisk_fd = metrics_disk_create()) < 0) {
> -      vu_log(VHOSTMD_ERR, "Failed to create metrics disk %s", mdisk_path);
> -      goto out;
> +   if (transports & VBD) {
> +      if ((mdisk_fd = metrics_disk_create()) < 0) {
> +         vu_log(VHOSTMD_ERR, "Failed to create metrics disk %s", mdisk_path);
> +         goto out;
> +      }
>      }
>   
>      /* Drop root privileges if requested.  Note: We do this after
>