Re: [libvirt] [PATCH v4 4/6] Add GVirConfigDomainHostdevPci

2016-07-08 Thread Christophe Fergeau 
On Wed, Jul 06, 2016 at 10:11:18PM +0100, Zeeshan Ali (Khattak) wrote:
> On Thu, May 12, 2016 at 1:28 PM, Christophe Fergeau  
> wrote:
> > Looks I never answered this one.
> >
> > On Tue, Apr 26, 2016 at 05:04:30PM +0100, Zeeshan Ali (Khattak) wrote:
> >> >> +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.
> >
> > libvirt treats "on"/"off" and "yes"/"no" as different types,
> > virTristateSwitch and virTristateBool.
> 
> Giving them different names don't really make them different. on/off
> and yes/no, both have two states and are essentially booleans.

Yes, but treating them as boolean means it's complicated to do the right
thing when converting them to strings as you cannot guess whether to use
"on"/"off" or "yes"/"no"

> > This patch would treat the 2 as "boolean", and only in the parsing case
> > as it obviously cannot guess what is the right behaviour when converting
> > to string.
> > So I'd rather we don't start to treat both as booleans.
> 
> So you're suggesting that I inline the implementation and not make this 
> generic?

Either that, or introduce an enum type for "on"/"off" and use that ?

Christophe


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 4/6] Add GVirConfigDomainHostdevPci

2016-07-06 Thread Zeeshan Ali (Khattak) 
On Thu, May 12, 2016 at 1:28 PM, Christophe Fergeau  wrote:
> Looks I never answered this one.
>
> On Tue, Apr 26, 2016 at 05:04:30PM +0100, Zeeshan Ali (Khattak) wrote:
>> >> +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.
>
> libvirt treats "on"/"off" and "yes"/"no" as different types,
> virTristateSwitch and virTristateBool.

Giving them different names don't really make them different. on/off
and yes/no, both have two states and are essentially booleans.

> This patch would treat the 2 as "boolean", and only in the parsing case
> as it obviously cannot guess what is the right behaviour when converting
> to string.
> So I'd rather we don't start to treat both as booleans.

So you're suggesting that I inline the implementation and not make this generic?


-- 
Regards,

Zeeshan Ali (Khattak)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 4/6] Add GVirConfigDomainHostdevPci

2016-05-12 Thread Christophe Fergeau 
Looks I never answered this one.

On Tue, Apr 26, 2016 at 05:04:30PM +0100, Zeeshan Ali (Khattak) wrote:
> >> +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.

libvirt treats "on"/"off" and "yes"/"no" as different types,
virTristateSwitch and virTristateBool.
This patch would treat the 2 as "boolean", and only in the parsing case
as it obviously cannot guess what is the right behaviour when converting
to string.
So I'd rather we don't start to treat both as booleans.

> 
> > 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?

I'm not sure these 2 attributes are related this way. We probably can
return the default used for new QEMU versions, and tweak it when needed.

Christophe


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 4/6] Add GVirConfigDomainHostdevPci

2016-04-26 Thread Zeeshan Ali (Khattak) 
On Thu, Apr 21, 2016 at 3:12 PM, Christophe Fergeau  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 000..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
>> + * .
>> + *
>> + * Authors: Zeeshan Ali (Khattak) 
>> + *  Christophe Fergeau 
>> + */
>> +
>> +#include 
>> +
>> +#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);
>> +}
>> +
>> +/**
>>

Re: [libvirt] [PATCH v4 4/6] Add GVirConfigDomainHostdevPci

2016-04-21 Thread Christophe Fergeau 
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 000..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
> + * .
> + *
> + * Authors: Zeeshan Ali (Khattak) 
> + *  Christophe Fergeau 
> + */
> +
> +#include 
> +
> +#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

[libvirt] [PATCH v4 4/6] Add GVirConfigDomainHostdevPci

2016-04-15 Thread Zeeshan Ali (Khattak) 
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 000..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
+ * .
+ *
+ * Authors: Zeeshan Ali (Khattak) 
+ *  Christophe Fergeau 
+ */
+
+#include 
+
+#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.
+ * 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