[Libvirt-cim] [PATCH V2 1/5] libxutil: Controller Support
John Ferlan
jferlan at redhat.com
Wed Mar 26 22:31:24 UTC 2014
Precursor to review - I'll merge your changes with what I posted last
week so we can then work off one common set of changes. They're pretty
close anyway since what I did was based on your v1.
My "goal" is to make breaks in the changes such that it's possible to
bisect history and reduce cimtest failures - it's not easy to
accomplish, but I'll try.
...
This patch will be split into two - the two .h files in one and the two
.c files in the other. It's more for easier review since I've added
quite a few new properties.
On 03/25/2014 03:20 AM, Xu Wang wrote:
> Signed-off-by: Xu Wang <gesaint at linux.vnet.ibm.com>
> ---
> libxkutil/device_parsing.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
> libxkutil/device_parsing.h | 9 +++++
> libxkutil/xmlgen.c | 30 +++++++++++++++++++
> src/svpc_types.h | 4 ++-
> 4 files changed, 111 insertions(+), 2 deletions(-)
>
> diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c
> index d2d3859..4240243 100644
> --- a/libxkutil/device_parsing.c
> +++ b/libxkutil/device_parsing.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright IBM Corp. 2007, 2013
> + * Copyright IBM Corp. 2007, 2014
Oh yea... Although I'll make it 2007-2014
> *
> * Authors:
> * Dan Smith <danms at us.ibm.com>
> @@ -49,6 +49,7 @@
> #define GRAPHICS_XPATH (xmlChar *)"/domain/devices/graphics | "\
> "/domain/devices/console"
> #define INPUT_XPATH (xmlChar *)"/domain/devices/input"
> +#define CONTROLLER_XPATH (xmlChar *)"/domain/devices/controller"
>
> #define DEFAULT_BRIDGE "xenbr0"
> #define DEFAULT_NETWORK "default"
> @@ -308,6 +309,15 @@ static void cleanup_input_device(struct input_device *dev)
> free(dev->bus);
> }
>
> +static void cleanup_controller_device(struct controller_device *dev)
> +{
> + if (dev == NULL)
> + return;
> +
> + free(dev->type);
> + free(dev->model);
See .h comments, but while we're at it - may as well add other
properties too (queues, ports, vectors, address) as well as storing the
index which we'll need.
> +}
> +
> void cleanup_virt_device(struct virt_device *dev)
> {
> if (dev == NULL)
> @@ -325,6 +335,8 @@ void cleanup_virt_device(struct virt_device *dev)
> cleanup_input_device(&dev->dev.input);
> else if (dev->type == CIM_RES_TYPE_CONSOLE)
> cleanup_console_device(&dev->dev.console);
> + else if (dev->type == CIM_RES_TYPE_CONTROLLER)
> + cleanup_controller_device(&dev->dev.controller);
>
> free(dev->id);
>
> @@ -1107,6 +1119,49 @@ static int parse_input_device(xmlNode *node, struct virt_device **vdevs)
> return 0;
> }
>
> +static int parse_controller_device(xmlNode *node, struct virt_device **vdevs)
> +{
> + struct virt_device *vdev = NULL;
> + struct controller_device *cdev = NULL;
> + char *index = NULL;
> + int ret;
> +
> + vdev = calloc(1, sizeof(*vdev));
> + if (vdev == NULL)
> + goto err;
> +
> + cdev = &(vdev->dev.controller);
> +
> + cdev->type = get_attr_value(node, "type");
> + cdev->model = get_attr_value(node, "model");
> + if (cdev->model == NULL) {
> + cdev->model = strdup("");
> + }
I disagree here - if the model doesn't exist, don't save it an empty
string, because then we'll write out an empty string which isn't
expected. If not found, then keep model as NULL and handle that later.
> + index = get_attr_value(node, "index");
We must save the index because if it exists, we need to write it out. If
it doesn't exist on input - that's a problem. As pointed out by Boris
in one of his reviews we'll have to handle this specially on the create
from a nonXML description path, see:
http://www.redhat.com/archives/libvirt-cim/2014-March/msg00037.html
search for CONTROLLER_INDEX_NOT_SET
> +
> + if (cdev->type == NULL)
> + goto err;
> +
> + vdev->type = CIM_RES_TYPE_CONTROLLER;
> +
> + ret = asprintf(&vdev->id, "%s:%s", cdev->type, index);
> + if (ret == -1) {
> + CU_DEBUG("Failed to create controller id string");
> + goto err;
> + }
> +
> + *vdevs = vdev;
> + free(index);
> +
> + return 1;
> + err:
> + free(index);
> + cleanup_controller_device(cdev);
> + free(vdev);
> +
> + return 0;
> +}
> +
> static bool resize_devlist(struct virt_device **list, int newsize)
> {
> struct virt_device *_list;
> @@ -1230,6 +1285,11 @@ static int parse_devices(const char *xml, struct virt_device **_list, int type)
> func = &parse_input_device;
> break;
>
> + case CIM_RES_TYPE_CONTROLLER:
> + xpathstr = CONTROLLER_XPATH;
> + func = &parse_controller_device;
> + break;
> +
> default:
> CU_DEBUG("Unrecognized device type. Returning.");
> goto err1;
> @@ -1351,7 +1411,11 @@ struct virt_device *virt_device_dup(struct virt_device *_dev)
> } else if (dev->type == CIM_RES_TYPE_CONSOLE) {
> console_device_dup(&dev->dev.console,
> &_dev->dev.console);
> + } else if (dev->type == CIM_RES_TYPE_CONTROLLER) {
> + DUP_FIELD(dev, _dev, dev.controller.type);
> + DUP_FIELD(dev, _dev, dev.controller.model);
There will be more fields here too
> }
> +
> return dev;
> }
>
> @@ -1731,6 +1795,9 @@ int get_dominfo_from_xml(const char *xml, struct domain **dominfo)
> (*dominfo)->dev_vcpu_ct = parse_devices(xml,
> &(*dominfo)->dev_vcpu,
> CIM_RES_TYPE_PROC);
> + (*dominfo)->dev_controller_ct = parse_devices(xml,
> + &(*dominfo)->dev_controller,
> + CIM_RES_TYPE_CONTROLLER);
>
> return ret;
>
> @@ -1819,6 +1886,7 @@ void cleanup_dominfo(struct domain **dominfo)
> cleanup_virt_devices(&dom->dev_graphics, dom->dev_graphics_ct);
> cleanup_virt_devices(&dom->dev_input, dom->dev_input_ct);
> cleanup_virt_devices(&dom->dev_console, dom->dev_console_ct);
> + cleanup_virt_devices(&dom->dev_controller, dom->dev_controller_ct);
>
> free(dom);
>
> diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h
> index a92e223..cc58970 100644
> --- a/libxkutil/device_parsing.h
> +++ b/libxkutil/device_parsing.h
> @@ -163,6 +163,11 @@ struct input_device {
> char *bus;
> };
>
> +struct controller_device {
> + char *type;
The index should be saved too... I've also converted the type to an int
and added API's to handle converting from type to string and back.
> + char *model;
While we're at it we should grab/store/restore other fields (ports,
vectors, queues, address)
> +};
> +
> struct virt_device {
> uint16_t type;
> union {
> @@ -174,6 +179,7 @@ struct virt_device {
> struct graphics_device graphics;
> struct console_device console;
> struct input_device input;
> + struct controller_device controller;
> } dev;
> char *id;
> };
> @@ -249,6 +255,9 @@ struct domain {
>
> struct virt_device *dev_vcpu;
> int dev_vcpu_ct;
> +
> + struct virt_device *dev_controller;
> + int dev_controller_ct;
> };
>
> struct virt_device *virt_device_dup(struct virt_device *dev);
> diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c
> index 18c4765..529c946 100644
> --- a/libxkutil/xmlgen.c
> +++ b/libxkutil/xmlgen.c
> @@ -798,6 +798,30 @@ static const char *input_xml(xmlNodePtr root, struct domain *dominfo)
> return NULL;
> }
>
> +static const char *controller_xml(xmlNodePtr root, struct domain *dominfo)
> +{
> + int i;
> +
> + for (i = 0; i < dominfo->dev_controller_ct; i++) {
> + xmlNodePtr tmp;
> + struct virt_device *_dev = &dominfo->dev_controller[i];
> + if (_dev->type == CIM_RES_TYPE_UNKNOWN)
> + continue;
> +
> + struct controller_device *dev = &_dev->dev.controller;
> +
> + tmp = xmlNewChild(root, NULL, BAD_CAST "controller", NULL);
> + if (tmp == NULL)
> + return XML_ERROR;
> +
> + xmlNewProp(tmp, BAD_CAST "type", BAD_CAST dev->type);
> + if (!STREQC(dev->model, ""))
> + xmlNewProp(tmp, BAD_CAST "model", BAD_CAST dev->model);
Also need to output other fields here if they exist as well.
> + }
> +
> + return NULL;
> +}
> +
> static char *system_xml(xmlNodePtr root, struct domain *domain)
> {
> xmlNodePtr tmp;
> @@ -1129,6 +1153,11 @@ char *device_to_xml(struct virt_device *_dev)
> dominfo->dev_input_ct = 1;
> dominfo->dev_input = dev;
> break;
> + case CIM_RES_TYPE_CONTROLLER:
> + func = controller_xml;
> + dominfo->dev_controller_ct = 1;
> + dominfo->dev_controller = dev;
> + break;
> default:
> cleanup_virt_devices(&dev, 1);
> goto out;
> @@ -1167,6 +1196,7 @@ char *system_to_xml(struct domain *dominfo)
> &console_xml,
> &graphics_xml,
> &emu_xml,
> + &controller_xml,
> NULL
> };
>
> diff --git a/src/svpc_types.h b/src/svpc_types.h
> index 404e428..7a2b653 100644
> --- a/src/svpc_types.h
> +++ b/src/svpc_types.h
> @@ -36,8 +36,9 @@
> #define CIM_RES_TYPE_IMAGE 32768
> #define CIM_RES_TYPE_CONSOLE 32769
> #define CIM_RES_TYPE_EMU 32770
> +#define CIM_RES_TYPE_CONTROLLER 32771
>
> -#define CIM_RES_TYPE_COUNT 7
> +#define CIM_RES_TYPE_COUNT 8
Avoid doing too early - causes cimtest failures since not all the
plumbing is available. So in the changes you see from me, this will not
be in the first patch - rather it'll be in a later patch.
> const static int cim_res_types[CIM_RES_TYPE_COUNT] =
> {CIM_RES_TYPE_NET,
> CIM_RES_TYPE_DISK,
> @@ -46,6 +47,7 @@ const static int cim_res_types[CIM_RES_TYPE_COUNT] =
> CIM_RES_TYPE_GRAPHICS,
> CIM_RES_TYPE_INPUT,
> CIM_RES_TYPE_CONSOLE,
> + CIM_RES_TYPE_CONTROLLER,
Avoid adding in this patch since alone it causes cimtest failures if
done too early.
> };
>
> #define CIM_VSSD_RECOVERY_NONE 2
>
The bottom of this will get a list of enum controller types and the
string/type conversion routines.
John
More information about the Libvirt-cim
mailing list