On 11/25/19 11:32 AM, Michael Trapp wrote:
> From: d032747 <michael.trapp@xxxxxxx>
>
> 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
>
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
