On Thu, Apr 21, 2016 at 3:12 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > On Fri, Apr 15, 2016 at 02:38:22PM +0100, Zeeshan Ali (Khattak) wrote: >> Add API to read and write PCI hostdev nodes. >> --- >> libvirt-gconfig/Makefile.am | 2 + >> .../libvirt-gconfig-domain-hostdev-pci.c | 210 +++++++++++++++++++++ >> .../libvirt-gconfig-domain-hostdev-pci.h | 81 ++++++++ >> libvirt-gconfig/libvirt-gconfig-domain-hostdev.c | 2 +- >> libvirt-gconfig/libvirt-gconfig.h | 1 + >> libvirt-gconfig/libvirt-gconfig.sym | 11 ++ >> 6 files changed, 306 insertions(+), 1 deletion(-) >> create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.c >> create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.h >> >> diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am >> index a7c6c4e..0400343 100644 >> --- a/libvirt-gconfig/Makefile.am >> +++ b/libvirt-gconfig/Makefile.am >> @@ -51,6 +51,7 @@ GCONFIG_HEADER_FILES = \ >> libvirt-gconfig-domain-graphics-spice.h \ >> libvirt-gconfig-domain-graphics-vnc.h \ >> libvirt-gconfig-domain-hostdev.h \ >> + libvirt-gconfig-domain-hostdev-pci.h \ >> libvirt-gconfig-domain-input.h \ >> libvirt-gconfig-domain-interface.h \ >> libvirt-gconfig-domain-interface-bridge.h \ >> @@ -143,6 +144,7 @@ GCONFIG_SOURCE_FILES = \ >> libvirt-gconfig-domain-graphics-spice.c \ >> libvirt-gconfig-domain-graphics-vnc.c \ >> libvirt-gconfig-domain-hostdev.c \ >> + libvirt-gconfig-domain-hostdev-pci.c \ >> libvirt-gconfig-domain-input.c \ >> libvirt-gconfig-domain-interface.c \ >> libvirt-gconfig-domain-interface-bridge.c \ >> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.c b/libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.c >> new file mode 100644 >> index 0000000..04e3da9 >> --- /dev/null >> +++ b/libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.c >> @@ -0,0 +1,210 @@ >> +/* >> + * libvirt-gconfig-domain-hostdev.c: libvirt domain hostdev configuration >> + * >> + * Copyright (C) 2016 Red Hat, Inc. >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * 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. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library. If not, see >> + * <http://www.gnu.org/licenses/>. >> + * >> + * Authors: Zeeshan Ali (Khattak) <zeeshanak@xxxxxxxxx> >> + * Christophe Fergeau <cfergeau@xxxxxxxxxx> >> + */ >> + >> +#include <config.h> >> + >> +#include "libvirt-gconfig/libvirt-gconfig.h" >> +#include "libvirt-gconfig/libvirt-gconfig-private.h" >> + >> +#define GVIR_CONFIG_DOMAIN_HOSTDEV_PCI_GET_PRIVATE(obj) \ >> + (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI, GVirConfigDomainHostdevPciPrivate)) >> + >> +struct _GVirConfigDomainHostdevPciPrivate >> +{ >> + gboolean unused; >> +}; >> + >> +G_DEFINE_TYPE(GVirConfigDomainHostdevPci, gvir_config_domain_hostdev_pci, GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV); >> + >> +static void gvir_config_domain_hostdev_pci_class_init(GVirConfigDomainHostdevPciClass *klass) >> +{ >> + g_type_class_add_private(klass, sizeof(GVirConfigDomainHostdevPciPrivate)); >> +} >> + >> + >> +static void gvir_config_domain_hostdev_pci_init(GVirConfigDomainHostdevPci *hostdev) >> +{ >> + hostdev->priv = GVIR_CONFIG_DOMAIN_HOSTDEV_PCI_GET_PRIVATE(hostdev); >> +} >> + >> +/** >> + * gvir_config_domain_hostdev_pci_new: >> + * >> + * Creates a new #GVirConfigDomainHostdevPci. >> + * >> + * Returns:(transfer full): a new #GVirConfigDomainHostdevPci. The returned >> + * object should be unreffed with g_object_unref() when no longer needed. >> + */ >> +GVirConfigDomainHostdevPci *gvir_config_domain_hostdev_pci_new(void) >> +{ >> + GVirConfigObject *object; >> + >> + object = gvir_config_object_new(GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI, >> + "hostdev", NULL); >> + gvir_config_object_set_attribute(object, "mode", "subsystem", NULL); >> + gvir_config_object_set_attribute(object, "type", "pci", NULL); >> + >> + return GVIR_CONFIG_DOMAIN_HOSTDEV_PCI(object); >> +} >> + >> +/** >> + * gvir_config_domain_hostdev_pci_new_from_xml: >> + * @xml: xml data to create the host device from >> + * @error: return location for a #GError, or NULL >> + * >> + * Creates a new #GVirConfigDomainHostdevPci with a reference count of 1. > > Still this "with a reference count of 1" wording > >> + * The host device object will be created using the XML description stored >> + * in @xml. This is a fragment of libvirt domain XML whose root node is >> + * <hostdev>. >> + * >> + * Returns: a new #GVirConfigDomainHostdevPci, or NULL if @xml failed to >> + * be parsed. >> + */ >> +GVirConfigDomainHostdevPci *gvir_config_domain_hostdev_pci_new_from_xml(const gchar *xml, >> + GError **error) >> +{ >> + GVirConfigObject *object; >> + >> + object = gvir_config_object_new_from_xml(GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI, >> + "hostdev", NULL, xml, error); >> + if (*error != NULL) >> + return NULL; > > This is going to crash with a NULL GError. > >> + >> + if (g_strcmp0(gvir_config_object_get_attribute(object, NULL, "type"), "pci") != 0) { >> + g_object_unref(G_OBJECT(object)); >> + g_return_val_if_reached(NULL); >> + } >> + >> + return GVIR_CONFIG_DOMAIN_HOSTDEV_PCI(object); >> +} >> + >> +void gvir_config_domain_hostdev_pci_set_address(GVirConfigDomainHostdevPci *hostdev, >> + GVirConfigDomainAddressPci *address) >> +{ >> + GVirConfigObject *source; >> + GVirConfigObject *addr_object; >> + xmlNodePtr node; >> + xmlAttrPtr attr; >> + >> + g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(hostdev)); >> + g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_ADDRESS_PCI(address)); >> + addr_object = GVIR_CONFIG_OBJECT(address); >> + node = gvir_config_object_get_xml_node(addr_object); > > This could be: > node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(address)); > and save the 'addr_object' variable. > > >> + g_return_if_fail(node != NULL); >> + >> + source = gvir_config_object_replace_child(GVIR_CONFIG_OBJECT(hostdev), >> + "source"); >> + /* Because of https://bugzilla.redhat.com/show_bug.cgi?id=1327577, we can't >> + * just use GVirConfigDomainAddressPci's node, as is, since it contains >> + * a 'type' attribute. So we create a copy for our use and just delete the >> + * 'type' attribute from it. > > "Since it contains a 'type' attribute, which is not accepted by libvirt" > >> + */ >> + node = xmlCopyNode(node, 1); >> + for (attr = node->properties; attr; attr = attr->next) { >> + if (g_strcmp0 ("type", (char *)attr->name) == 0) { >> + xmlRemoveProp (attr); >> + break; >> + } >> + } >> + gvir_config_object_set_child(source, node); >> + g_object_unref(source); >> +} >> + >> +GVirConfigDomainAddressPci *gvir_config_domain_hostdev_pci_get_address(GVirConfigDomainHostdevPci *hostdev) >> +{ >> + GVirConfigObject *source; >> + GVirConfigObject* address; >> + >> + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(hostdev), NULL); >> + >> + source = gvir_config_object_get_child(GVIR_CONFIG_OBJECT(hostdev), "source"); >> + if (source == NULL) >> + return NULL; >> + >> + address = gvir_config_object_get_child_with_type(source, >> + "address", >> + GVIR_CONFIG_TYPE_DOMAIN_ADDRESS_PCI); >> + g_object_unref(source); >> + return GVIR_CONFIG_DOMAIN_ADDRESS_PCI(address); >> +} >> + >> +void gvir_config_domain_hostdev_pci_set_managed(GVirConfigDomainHostdevPci *hostdev, >> + gboolean managed) >> +{ >> + g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(hostdev)); >> + >> + gvir_config_object_set_attribute_with_type(GVIR_CONFIG_OBJECT(hostdev), >> + "managed", >> + G_TYPE_BOOLEAN, >> + managed, >> + NULL); >> +} >> + >> +gboolean gvir_config_domain_hostdev_pci_get_managed(GVirConfigDomainHostdevPci *hostdev) >> +{ >> + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(hostdev), FALSE); >> + >> + return gvir_config_object_get_attribute_boolean(GVIR_CONFIG_OBJECT(hostdev), >> + NULL, > >> + "managed", >> + FALSE); >> +} >> + >> +void gvir_config_domain_hostdev_pci_set_rom_file(GVirConfigDomainHostdevPci *hostdev, >> + const gchar *file) >> +{ >> + GVirConfigObject *rom; >> + >> + g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(hostdev)); >> + >> + rom = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(hostdev), "rom"); >> + gvir_config_object_set_attribute(rom, >> + "file", file, >> + NULL); >> + g_object_unref(rom); >> +} >> + >> +void gvir_config_domain_hostdev_pci_set_rom_bar(GVirConfigDomainHostdevPci *hostdev, >> + gboolean bar) >> +{ >> + GVirConfigObject *rom; >> + >> + g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(hostdev)); >> + >> + rom = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(hostdev), "rom"); >> + gvir_config_object_set_attribute(rom, >> + "bar", bar? "on" : "off", >> + NULL); >> + g_object_unref(rom); >> +} >> + >> +const gchar *gvir_config_domain_hostdev_pci_get_rom_file(GVirConfigDomainHostdevPci *hostdev) >> +{ >> + return gvir_config_object_get_attribute(GVIR_CONFIG_OBJECT(hostdev), "rom", "file"); > >> +} >> + >> +gboolean gvir_config_domain_hostdev_pci_get_rom_bar(GVirConfigDomainHostdevPci *hostdev) >> +{ >> + return gvir_config_object_get_attribute_boolean(GVIR_CONFIG_OBJECT(hostdev), >> + "rom", "bar", FALSE); > > I'd prefer to handle on/off parsing here rather than moving it to > get_attribute_boolean(). Why? Quick look through libvirt XML docs, shows that on/off is used in other places too. > I don't know whether we should return TRUE or > FALSE as the default value as this depends on the QEMU version :( > "If no rom bar is specified, the qemu default will be used (older > versions of qemu used a default of "off", while newer qemus have a > default of "on")" Maybe we can go with newer behaviour depending on whether or not rom file is set and leave a comment here about this? > Looks good otherwise. > > Christophe > >> +} >> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.h b/libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.h >> new file mode 100644 >> index 0000000..51378aa >> --- /dev/null >> +++ b/libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.h >> @@ -0,0 +1,81 @@ >> +/* >> + * libvirt-gconfig-domain-hostdev.h: libvirt domain hostdev configuration >> + * >> + * Copyright (C) 2016 Red Hat, Inc. >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * 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. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library. If not, see >> + * <http://www.gnu.org/licenses/>. >> + * >> + * Authors: Zeeshan Ali (Khattak) <zeeshanak@xxxxxxxxx> >> + * Christophe Fergeau <cfergeau@xxxxxxxxxx> >> + */ >> + >> +#if !defined(__LIBVIRT_GCONFIG_H__) && !defined(LIBVIRT_GCONFIG_BUILD) >> +#error "Only <libvirt-gconfig/libvirt-gconfig.h> can be included directly." >> +#endif >> + >> +#ifndef __LIBVIRT_GCONFIG_DOMAIN_HOSTDEV_PCI_H__ >> +#define __LIBVIRT_GCONFIG_DOMAIN_HOSTDEV_PCI_H__ >> + >> +G_BEGIN_DECLS >> + >> +#define GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI (gvir_config_domain_hostdev_pci_get_type ()) >> +#define GVIR_CONFIG_DOMAIN_HOSTDEV_PCI(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI, GVirConfigDomainHostdevPci)) >> +#define GVIR_CONFIG_DOMAIN_HOSTDEV_PCI_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI, GVirConfigDomainHostdevPciClass)) >> +#define GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI)) >> +#define GVIR_CONFIG_IS_DOMAIN_HOSTDEV_PCI_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI)) >> +#define GVIR_CONFIG_DOMAIN_HOSTDEV_PCI_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI, GVirConfigDomainHostdevPciClass)) >> + >> +typedef struct _GVirConfigDomainHostdevPci GVirConfigDomainHostdevPci; >> +typedef struct _GVirConfigDomainHostdevPciPrivate GVirConfigDomainHostdevPciPrivate; >> +typedef struct _GVirConfigDomainHostdevPciClass GVirConfigDomainHostdevPciClass; >> + >> +struct _GVirConfigDomainHostdevPci >> +{ >> + GVirConfigDomainHostdev parent; >> + >> + GVirConfigDomainHostdevPciPrivate *priv; >> + >> + /* Do not add fields to this struct */ >> +}; >> + >> +struct _GVirConfigDomainHostdevPciClass >> +{ >> + GVirConfigDomainHostdevClass parent_class; >> + >> + gpointer padding[20]; >> +}; >> + >> +GType gvir_config_domain_hostdev_pci_get_type(void); >> + >> +GVirConfigDomainHostdevPci *gvir_config_domain_hostdev_pci_new(void); >> +GVirConfigDomainHostdevPci *gvir_config_domain_hostdev_pci_new_from_xml(const gchar *xml, >> + GError **error); >> +void gvir_config_domain_hostdev_pci_set_address(GVirConfigDomainHostdevPci *hostdev, >> + GVirConfigDomainAddressPci *address); >> +GVirConfigDomainAddressPci *gvir_config_domain_hostdev_pci_get_address(GVirConfigDomainHostdevPci *hostdev); >> + >> +void gvir_config_domain_hostdev_pci_set_managed(GVirConfigDomainHostdevPci *hostdev, >> + gboolean managed); >> +gboolean gvir_config_domain_hostdev_pci_get_managed(GVirConfigDomainHostdevPci *hostdev); >> +void gvir_config_domain_hostdev_pci_set_rom_file(GVirConfigDomainHostdevPci *hostdev, >> + const gchar *file); >> +const gchar *gvir_config_domain_hostdev_pci_get_rom_file(GVirConfigDomainHostdevPci *hostdev); >> +void gvir_config_domain_hostdev_pci_set_rom_bar(GVirConfigDomainHostdevPci *hostdev, >> + gboolean bar); >> +gboolean gvir_config_domain_hostdev_pci_get_rom_bar(GVirConfigDomainHostdevPci *hostdev); >> + >> +G_END_DECLS >> + >> +#endif /* __LIBVIRT_GCONFIG_DOMAIN_HOSTDEV_PCI_H__ */ >> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-hostdev.c b/libvirt-gconfig/libvirt-gconfig-domain-hostdev.c >> index 205878e..4b8a447 100644 >> --- a/libvirt-gconfig/libvirt-gconfig-domain-hostdev.c >> +++ b/libvirt-gconfig/libvirt-gconfig-domain-hostdev.c >> @@ -62,7 +62,7 @@ gvir_config_domain_hostdev_new_from_tree(GVirConfigXmlDoc *doc, >> if (g_str_equal(type, "usb")) { >> goto unimplemented; >> } else if (g_str_equal(type, "pci")) { >> - goto unimplemented; >> + gtype = GVIR_CONFIG_TYPE_DOMAIN_HOSTDEV_PCI; >> } else if (g_str_equal(type, "scsi")) { >> goto unimplemented; >> } else { >> diff --git a/libvirt-gconfig/libvirt-gconfig.h b/libvirt-gconfig/libvirt-gconfig.h >> index cfa9dd3..6462154 100644 >> --- a/libvirt-gconfig/libvirt-gconfig.h >> +++ b/libvirt-gconfig/libvirt-gconfig.h >> @@ -68,6 +68,7 @@ >> #include <libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.h> >> #include <libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.h> >> #include <libvirt-gconfig/libvirt-gconfig-domain-hostdev.h> >> +#include <libvirt-gconfig/libvirt-gconfig-domain-hostdev-pci.h> >> #include <libvirt-gconfig/libvirt-gconfig-domain-input.h> >> #include <libvirt-gconfig/libvirt-gconfig-domain-interface.h> >> #include <libvirt-gconfig/libvirt-gconfig-domain-interface-bridge.h> >> diff --git a/libvirt-gconfig/libvirt-gconfig.sym b/libvirt-gconfig/libvirt-gconfig.sym >> index 1cfc6eb..f26423f 100644 >> --- a/libvirt-gconfig/libvirt-gconfig.sym >> +++ b/libvirt-gconfig/libvirt-gconfig.sym >> @@ -741,6 +741,17 @@ global: >> gvir_config_domain_hostdev_get_readonly; >> gvir_config_domain_hostdev_get_shareable; >> gvir_config_domain_hostdev_get_type; >> + gvir_config_domain_hostdev_pci_get_address; >> + gvir_config_domain_hostdev_pci_get_managed; >> + gvir_config_domain_hostdev_pci_get_rom_bar; >> + gvir_config_domain_hostdev_pci_get_rom_file; >> + gvir_config_domain_hostdev_pci_get_type; >> + gvir_config_domain_hostdev_pci_new; >> + gvir_config_domain_hostdev_pci_new_from_xml; >> + gvir_config_domain_hostdev_pci_set_address; >> + gvir_config_domain_hostdev_pci_set_managed; >> + gvir_config_domain_hostdev_pci_set_rom_bar; >> + gvir_config_domain_hostdev_pci_set_rom_file; >> gvir_config_domain_hostdev_set_boot_order; >> gvir_config_domain_hostdev_set_readonly; >> gvir_config_domain_hostdev_set_shareable; >> -- >> 2.5.5 >> >> -- >> libvir-list mailing list >> libvir-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/libvir-list -- Regards, Zeeshan Ali (Khattak) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list