Re: [libvirt] [PATCH] qemu: Support chardevs with ARM virt machines
On Wed, 2017-06-07 at 23:13 +0200, Christoffer Dall wrote: > The function to check if -chardev is supported by QEMU was written a > long time ago, where adding chardevs did not make sense on the fixed ARM > platforms. Since then, we now have a general purpose virt platform, > which should support plugging in any device over PCIe which is supported > in a similar fashion on x86. > > Signed-off-by: Christoffer Dall > --- > src/qemu/qemu_capabilities.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 7f22492..1348af7 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -5507,6 +5507,11 @@ virQEMUCapsSupportsChardev(const virDomainDef *def, > if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != >VIR_ARCH_AARCH64)) > return true; > > +/* The virt machine has a PCIe bus and allows plugging in the same type > of > + * devices as x86 systems do on a PCIe bus. */ > +if (qemuDomainIsVirt(def)) > +return true; > + > /* This may not be true for all ARM machine types, but at least > * the only supported non-virtio serial devices of vexpress and versatile > * don't have the -chardev property wired up. */ We have two bugs tracking this issue: https://bugs.linaro.org/show_bug.cgi?id=2777 https://bugzilla.redhat.com/show_bug.cgi?id=1435681 You mention in [1] that applying this patch and using a recent QEMU fixes the problem for you, however I can't say the same: I still get -device isa-serial,chardev=charserial0,id=serial0: No 'ISA' bus found for device 'isa-serial' Would you mind sharing your guest XML and the resulting QEMU command line? [1] https://bugs.linaro.org/show_bug.cgi?id=2777#c36 -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Remove duplicated code in qemuBuildSerialChrDeviceStr()
The call to qemuBuildDeviceAddressStr() happens no matter
what, so we can move it outside of the switch. We can also
move the call to virBufferAsprintf() closer to it to avoid
having formatting - error checking - more formatting.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_command.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..9bb0163 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10296,10 +10296,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
goto error;
}
} else {
-virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
-
virDomainChrSerialTargetTypeToString(serial->targetType),
- serial->info.alias, serial->info.alias);
-
switch (serial->targetType) {
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) {
@@ -10314,9 +10310,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("usb-serial requires address of usb type"));
goto error;
}
-
-if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
-goto error;
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
@@ -10326,9 +10319,6 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("isa-serial requires address of isa type"));
goto error;
}
-
-if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
-goto error;
break;
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
@@ -10344,11 +10334,15 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
_("pci-serial requires address of pci type"));
goto error;
}
-
-if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps)
< 0)
-goto error;
break;
}
+
+virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s",
+
virDomainChrSerialTargetTypeToString(serial->targetType),
+ serial->info.alias, serial->info.alias);
+
+if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0)
+goto error;
}
if (virBufferCheckError(&cmd) < 0)
--
2.7.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 2/2] Resctrl: Add uitls functions to operate sysfs resctrl
On Wednesday, 21 June 2017 at 10:44 PM, Martin Kletzander wrote: > You don't need to pass the whole structure of all the data. Can't the > qemu function just do something like: > > virResctrlLock() > foreach cachetune: > region = virResctrlGetFreeRegion(size, type) > foreach cachetune.vcpu: > virResctrlSetRegion(vcpu.pid, region) > Sure, good suggestion, I will try to read what other tune does. > > Or like with some other tuning, you can have a function that determines > the region when given vcpu and just call it for all vcpus. You can > save the regions in the status XML, so that not only users can see it, > but you can also reference them from that aforementioned function. Or > you could have saved pairs of id: region, but I think that's not needed. > > That reminds me, unless referred to from somewhere, the cachetune > doesn't even need the id. > > But basically, you don't need to pass the whole cachetune or any other > structure. The code is very messy, check your pointers and don't > compare references to NULLs. Read the diffs after yourself. I know it > works for you, but the code needs to be readable as well. Forgive me I am not a qualified c programmer for years :(, I will try to refine them. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/2] Resctrl: Add new xml element to support cache tune
hi Martin It’s really nice of you to help reviewing the mass code. Thanks. I don’t find a better way to split patch. On Wednesday, 21 June 2017 at 9:53 PM, Martin Kletzander wrote: > On Mon, Jun 12, 2017 at 05:48:40PM +0800, Eli Qiao wrote: > > This patch adds new xml element to support cache tune as: > > > > > > ... > > > vcpus='1,2'/> > > > > > The cache_id automatically implies level and type. Either have one or > the other. I know we talked about this already (maybe multiple times), > but without any clear outcome. For me the sensible thing is to have > level and type as that doesn't need to be changed when moving between > hosts, and if it cannot be migrated, then it's properly checked. > Think about this case, if the VM has numa setting, the VM has multiple vcpu running across sockets, if we don’t specify cache_id (cache id stand for on which Socket/Cache), how can we know on which Socket we allocation for the VM? I can image there’s 2 cases: 1. if we don’t specify vcpus, and our host have 2 or more Socket, we have this xml define We allocate 2816 KiB cache on all of the Socket/Cache. 2. if we specify vcpus We need to make sure we vcpu 1, 2 are mapped to Socket/Cache 0 and 3,4 on Socket/Cache 1. So that vcpus running on Socket/Cache 0 has 2816 KiB cache allocated and vcpus running on Socket/Cache 1 has 5632 KiB cache allocated. Does it make sense? … > > > > virDomainCputune cputune; > > > > + virDomainCachetune cachetune; > > + > > > > > It is part of cputune in the XML, why not here? Oh yes, I will rethink how to simple the domain cache tune. > > > virDomainNumaPtr numa; > > virDomainResourceDefPtr resource; > > virDomainIdMapDef idmap; > > -- > > 1.9.1 > > > > -- > > libvir-list mailing list > > libvir-list@redhat.com (mailto:libvir-list@redhat.com) > > https://www.redhat.com/mailman/listinfo/libvir-list > > > > > -- > libvir-list mailing list > libvir-list@redhat.com (mailto:libvir-list@redhat.com) > https://www.redhat.com/mailman/listinfo/libvir-list > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-sandbox PATCH 0/2] virt-sandbox-image: unbreak start from library
On Wed, Jun 07, 2017 at 08:02:03AM +0200, Guido Günther wrote: > This is basically a V2 of "Drop library/ from template name and image path" > with Dan's comment implemented. Ping? -- Guido > > Guido Günther (2): > Drop library/ from image path > Sanitize domain name > > libvirt-sandbox/image/cli.py| 8 ++-- > libvirt-sandbox/image/sources/docker.py | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > -- > 2.11.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH v1 1/4] numa: describe siblings distances within cells
On Mon, 19 Jun 2017 12:26:20 -0600
Jim Fehlig wrote:
> On 06/12/2017 12:54 PM, Wim Ten Have wrote:
> > From: Wim ten Have
> >
> > Add libvirtd NUMA cell domain administration functionality to
> > describe underlying cell id sibling distances in full fashion
> > when configuring HVM guests.
>
> Thanks for the detailed cover letter :-). Some of that information (e.g. ACPI
> spec, SLIT table, qemu support) would be useful in this commit message.
I'll follow up on that under v2.
> > [below is an example of a 4 node setup]
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
>
> Others on the list are better XML designers, but the following seems to
> achieve
> the same and is a bit more compact
>
>
>
>
>
>
>
>
> CC danpb, who often has good ideas on schema design.
Will give Daniel and (others) time prior submitting v2. Otherwise
follow your suggestion above for its compact and clear annotation.
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Changes under this commit concern all those that require adding
> > the valid data structures, virDomainNuma* functional routines and the
> > XML doc schema enhancements to enforce appropriate administration.
>
> One item you forgot was docs/formatdomain.html.in. Changes in schema should
> always be described by accompanying changes in documentation.
Oops ... noted. I'll go slow on coming back with v2 giving you and
other time to further comment. Thanks for reviewing!
- Wim.
> Regards,
> Jim
>
> >
> > These changes alter the docs/schemas/cputypes.rng enforcing
> > domain administration to follow the syntax below per numa cell id.
> >
> > These changes also alter docs/schemas/basictypes.rng to add
> > "numaDistanceValue" which is an "unsignedInt" with a minimum value
> > of 10 as 0-9 are reserved values and can not be used as System
> > Locality Distance Information Table data.
> >
> > Signed-off-by: Wim ten Have
> > ---
> > docs/schemas/basictypes.rng | 8 ++
> > docs/schemas/cputypes.rng | 18 +++
> > src/conf/cpu_conf.c | 2 +-
> > src/conf/numa_conf.c| 260
> > +++-
> > src/conf/numa_conf.h| 25 -
> > src/libvirt_private.syms| 6 +
> > 6 files changed, 313 insertions(+), 6 deletions(-)
> >
> > diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> > index 1ea667c..a335b5d 100644
> > --- a/docs/schemas/basictypes.rng
> > +++ b/docs/schemas/basictypes.rng
> > @@ -77,6 +77,14 @@
> >
> >
> >
> > +
> > +
> > +
> > +10
> > +
> > +
> > +
> > +
> >
> >
> >
> > diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
> > index 3eef16a..c45b6df 100644
> > --- a/docs/schemas/cputypes.rng
> > +++ b/docs/schemas/cputypes.rng
> > @@ -129,6 +129,24 @@
> >
> >
> >
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> >
> >
> >
> > diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> > index da40e9b..5d8f7be3 100644
> > --- a/src/conf/cpu_conf.c
> > +++ b/src/conf/cpu_conf.c
> > @@ -643,7 +643,7 @@ virCPUDefFormatBufFull(virBufferPtr buf,
> > if (virCPUDefFormatBuf(&childrenBuf, def, updateCPU) < 0)
> > goto cleanup;
> >
> > -if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0)
> > +if (virDomainNumaDefCPUFormatXML(&childrenBuf, numa) < 0)
> > goto cleanup;
> >
> > /* Put it all together */
> > diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> > index bfd3703..1914810 100644
> > --- a/src/conf/numa_conf.c
> > +++ b/src/conf/numa_conf.c
> > @@ -48,6 +48,8 @@ VIR_ENUM_IMPL(virDomainMemoryAccess,
> > VIR_DOMAIN_MEMORY_ACCESS_LAST,
> >"shared",
> >"private")
> >
> > +typedef struct _virDomainNumaDistance virDomainNumaDistance;
> > +typedef virDomainNumaDistance *virDomainNumaDistancePtr;
> >
> > typedef struct _virDomainNumaNode virDomainNumaNode;
> > typedef virDomainNumaNode *virDomainNumaNodePtr;
> > @@ -66,6 +68,12 @@ struct _virDomainNuma {
> > virBitmapPtr nodeset; /* host memory nodes where this guest node
> > resides */
> > virDomainNumatuneMemMode mode; /* memory mode selection */
> > virDomainMemoryAccess memAccess; /* shared memory access
> > configuration */
> > +
> > +struct _virDomainNumaDistance {
> > + unsig
[libvirt] [PATCH v3 2/2] util: fix locale problem with virStrToDouble().
This commit fixes a locale problem with locales that use comma as a mantissa separator. Example: 12.34 en_US = 12,34 pt_BR. Since strtod() is a non-safe function, virStrToDouble() will have problems to parse double numbers from kernel settings and other double numbers from static files (XMLs, JSONs, etc). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1457634 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1457481 Signed-off-by: Julio Faracco --- src/util/virstring.c | 18 +- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 9c12f7b..1bd6777 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -534,7 +534,13 @@ virLocaleOnceInit(void) VIR_ONCE_GLOBAL_INIT(virLocale); #endif - +/** + * virStrToDouble + * + * converts string with C locale (thread-safe) to double. + * + * Returns -1 on error or returns 0 on success. + */ int virStrToDouble(char const *s, char **end_ptr, @@ -545,7 +551,17 @@ virStrToDouble(char const *s, int err; errno = 0; +#if HAVE_NEWLOCALE +locale_t old_loc; +if (virLocaleInitialize() < 0) +return -1; + +old_loc = uselocale(virLocale); +#endif val = strtod(s, &p); /* exempt from syntax-check */ +#if HAVE_NEWLOCALE +uselocale(old_loc); +#endif err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/2] Adding locale support for virStrToDouble().
The commits add locale support for virStrToDouble() due to differences between the mantissa separator in different languages. For example, kernel always uses dot to separate mantissa. An user who is using pt_BR locale (for example) uses comma as a separator. So, this user will have problems to parse a kernel settings using strtod() function. One of commits move the virDoubleToStr() to virstring.* to share locale global variables. Joining the two functions makes more sense. Julio Faracco (2): util: moving virDoubleToStr() from virutil to virstring. util: fix locale problem with virStrToDouble(). src/util/virstring.c | 81 src/util/virstring.h | 3 ++ src/util/virutil.c | 63 src/util/virutil.h | 3 -- 4 files changed, 84 insertions(+), 66 deletions(-) -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/2] util: moving virDoubleToStr() from virutil to virstring.
The function virDoubleToStr() is defined in virutil.* and virStrToDouble() is
defined in virstring.*. Joining both functions into the same file makes more
sense.
Signed-off-by: Julio Faracco
---
src/util/virstring.c | 65
src/util/virstring.h | 3 +++
src/util/virutil.c | 63 --
src/util/virutil.h | 3 ---
4 files changed, 68 insertions(+), 66 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 089b539..9c12f7b 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -28,6 +28,7 @@
#include "base64.h"
#include "c-ctype.h"
#include "virstring.h"
+#include "virthread.h"
#include "viralloc.h"
#include "virbuffer.h"
#include "virerror.h"
@@ -516,6 +517,24 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
return 0;
}
+/* In case thread-safe locales are available */
+#if HAVE_NEWLOCALE
+
+static locale_t virLocale;
+
+static int
+virLocaleOnceInit(void)
+{
+virLocale = newlocale(LC_ALL_MASK, "C", (locale_t)0);
+if (!virLocale)
+return -1;
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virLocale);
+#endif
+
+
int
virStrToDouble(char const *s,
char **end_ptr,
@@ -536,6 +555,52 @@ virStrToDouble(char const *s,
return 0;
}
+/**
+ * virDoubleToStr
+ *
+ * converts double to string with C locale (thread-safe).
+ *
+ * Returns -1 on error, size of the string otherwise.
+ */
+int
+virDoubleToStr(char **strp, double number)
+{
+int ret = -1;
+
+#if HAVE_NEWLOCALE
+
+locale_t old_loc;
+
+if (virLocaleInitialize() < 0)
+goto error;
+
+old_loc = uselocale(virLocale);
+ret = virAsprintf(strp, "%lf", number);
+uselocale(old_loc);
+
+#else
+
+char *radix, *tmp;
+struct lconv *lc;
+
+if ((ret = virAsprintf(strp, "%lf", number) < 0))
+goto error;
+
+lc = localeconv();
+radix = lc->decimal_point;
+tmp = strstr(*strp, radix);
+if (tmp) {
+*tmp = '.';
+if (strlen(radix) > 1)
+memmove(tmp + 1, tmp + strlen(radix), strlen(*strp) - (tmp -
*strp));
+}
+
+#endif /* HAVE_NEWLOCALE */
+ error:
+return ret;
+}
+
+
int
virVasprintfInternal(bool report,
int domcode,
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 0038a40..5eaaaea 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -109,6 +109,9 @@ int virStrToDouble(char const *s,
double *result)
ATTRIBUTE_RETURN_CHECK;
+int virDoubleToStr(char **strp, double number)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
void virSkipSpaces(const char **str) ATTRIBUTE_NONNULL(1);
void virSkipSpacesAndBackslash(const char **str) ATTRIBUTE_NONNULL(1);
void virTrimSpaces(char *str, char **endp) ATTRIBUTE_NONNULL(1);
diff --git a/src/util/virutil.c b/src/util/virutil.c
index aba7c6d..d7e01d4 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -75,7 +75,6 @@
#include "virlog.h"
#include "virbuffer.h"
#include "viralloc.h"
-#include "virthread.h"
#include "verify.h"
#include "virfile.h"
#include "vircommand.h"
@@ -437,68 +436,6 @@ int virEnumFromString(const char *const*types,
return -1;
}
-/* In case thread-safe locales are available */
-#if HAVE_NEWLOCALE
-
-static locale_t virLocale;
-
-static int
-virLocaleOnceInit(void)
-{
-virLocale = newlocale(LC_ALL_MASK, "C", (locale_t)0);
-if (!virLocale)
-return -1;
-return 0;
-}
-
-VIR_ONCE_GLOBAL_INIT(virLocale)
-#endif
-
-/**
- * virDoubleToStr
- *
- * converts double to string with C locale (thread-safe).
- *
- * Returns -1 on error, size of the string otherwise.
- */
-int
-virDoubleToStr(char **strp, double number)
-{
-int ret = -1;
-
-#if HAVE_NEWLOCALE
-
-locale_t old_loc;
-
-if (virLocaleInitialize() < 0)
-goto error;
-
-old_loc = uselocale(virLocale);
-ret = virAsprintf(strp, "%lf", number);
-uselocale(old_loc);
-
-#else
-
-char *radix, *tmp;
-struct lconv *lc;
-
-if ((ret = virAsprintf(strp, "%lf", number) < 0))
-goto error;
-
-lc = localeconv();
-radix = lc->decimal_point;
-tmp = strstr(*strp, radix);
-if (tmp) {
-*tmp = '.';
-if (strlen(radix) > 1)
-memmove(tmp + 1, tmp + strlen(radix), strlen(*strp) - (tmp -
*strp));
-}
-
-#endif /* HAVE_NEWLOCALE */
- error:
-return ret;
-}
-
/**
* Format @val as a base-10 decimal number, in the
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 86e9051..4938255 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -63,9 +63,6 @@ int virParseNumber(const char **str);
int virParseVersionString(const char *str, unsigned long *version,
bool allowMissing);
-int virDoubleToStr(char **strp, double number)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
-
char *virFormatIntDecimal(char *buf, size_t buflen, int val)
ATTRIBUTE_NONNULL(
Re: [libvirt] [PATCH 2/3] lease time support per host in dnsmasq
On 06/20/2017 02:00 PM, ar...@gnome.org wrote:
> From: Alberto Ruiz
>
> ---
> src/network/bridge_driver.c | 56
> ++---
> src/util/virdnsmasq.c | 52 +++--
> src/util/virdnsmasq.h | 1 +
> 3 files changed, 57 insertions(+), 52 deletions(-)
As far as I can see, this doesn't set a different lease time for each
static host entry (which is what the title implies), but just sets the
single specified leasetime for *all* static host entries.
Aside from that, I'm not sure what is the value of setting a leastime on
a static host entry. An explanation of that in the commit log would be
helpful in determining whether or not there's even a point to doing this.
Also, I forgot to say it wrt to the previous patch, but you need to add
something to docs/formatnetwork.html.in to document the new config knob.
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index e51e469c8..1cffd4dcf 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -861,30 +861,6 @@ networkKillDaemon(pid_t pid, const char *daemonName,
> const char *networkName)
> return ret;
> }
>
> -/* the following does not build a file, it builds a list
> - * which is later saved into a file
> - */
> -
> -static int
> -networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
> - virNetworkIPDefPtr ipdef)
> -{
> -size_t i;
> -bool ipv6 = false;
> -
> -if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> -ipv6 = true;
> -for (i = 0; i < ipdef->nhosts; i++) {
> -virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
> -if (VIR_SOCKET_ADDR_VALID(&host->ip))
> -if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip,
> - host->name, host->id, ipv6) < 0)
> -return -1;
> -}
> -
> -return 0;
> -}
> -
> static int
> networkBuildDnsmasqHostsList(dnsmasqContext *dctx,
> virNetworkDNSDefPtr dnsdef)
> @@ -940,6 +916,38 @@ networkDnsmasqConfLeaseValueToString (int64_t leasetime)
> return result;
> }
>
> +/* the following does not build a file, it builds a list
> + * which is later saved into a file
> + */
> +
> +static int
> +networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
> + virNetworkIPDefPtr ipdef)
> +{
> +int ret = -1;
> +size_t i;
> +bool ipv6 = false;
> +char *leasetime = networkDnsmasqConfLeaseValueToString(ipdef->leasetime);
> +
> +if (!leasetime)
> +goto cleanup;
> +
> +if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> +ipv6 = true;
> +for (i = 0; i < ipdef->nhosts; i++) {
> +virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
> +if (VIR_SOCKET_ADDR_VALID(&host->ip))
> +if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip,
> + host->name, host->id, leasetime, ipv6) <
> 0)
> +goto cleanup;
> +}
> +
> +ret = 0;
> +cleanup:
> +VIR_FREE(leasetime);
> +return ret;
> +}
> +
> int
> networkDnsmasqConfContents(virNetworkObjPtr network,
> const char *pidfile,
> diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
> index 1b78c1fad..92f834fe7 100644
> --- a/src/util/virdnsmasq.c
> +++ b/src/util/virdnsmasq.c
> @@ -308,52 +308,47 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile,
> virSocketAddr *ip,
> const char *name,
> const char *id,
> + const char *leasetime,
> bool ipv6)
> {
> +int ret = -1;
> char *ipstr = NULL;
> +virBuffer hostbuf = VIR_BUFFER_INITIALIZER;
> +
> if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0)
> goto error;
>
> if (!(ipstr = virSocketAddrFormat(ip)))
> -return -1;
> +goto error;
>
> /* the first test determines if it is a dhcpv6 host */
> if (ipv6) {
> -if (name && id) {
> -if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host,
> -"id:%s,%s,[%s]", id, name, ipstr) < 0)
> -goto error;
> -} else if (name && !id) {
> -if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host,
> -"%s,[%s]", name, ipstr) < 0)
> -goto error;
> -} else if (!name && id) {
> -if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host,
> -"id:%s,[%s]", id, ipstr) < 0)
> -goto error;
> -}
> +if (name && id)
> +virBufferAsprintf(&hostbuf, "id:%s,%s,[%s]", id, name, ipstr);
> +else if (name && !id)
> +virBufferAsprintf(&hostbuf, "%s,[%s]", name, ipstr);
> +else if (!name && id)
> +virBufferAsprintf(&hostbuf, "id:%s,[%s]", id, ipstr);
>
Re: [libvirt] [PATCH 1/3] leasetime support for globally
On 06/21/2017 03:27 AM, Peter Krempa wrote: > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote: >> From: Alberto Ruiz > > Missing commit message. And when composing the commit message, it's useful to include links to the associated BZ. https://bugzilla.redhat.com/show_bug.cgi?id=913446 Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often). As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned). I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for. > >> >> --- >> docs/schemas/basictypes.rng | 16 + >> docs/schemas/network.rng | 8 +++ >> src/conf/network_conf.c | 78 >> ++- >> src/conf/network_conf.h | 3 +- >> src/network/bridge_driver.c | 49 +- >> tests/networkxml2confdata/leasetime-days.conf | 17 + >> tests/networkxml2confdata/leasetime-days.xml | 18 ++ >> tests/networkxml2confdata/leasetime-hours.conf| 17 + >> tests/networkxml2confdata/leasetime-hours.xml | 18 ++ >> tests/networkxml2confdata/leasetime-infinite.conf | 17 + >> tests/networkxml2confdata/leasetime-infinite.xml | 18 ++ >> tests/networkxml2confdata/leasetime-minutes.conf | 17 + >> tests/networkxml2confdata/leasetime-minutes.xml | 18 ++ >> tests/networkxml2confdata/leasetime-seconds.conf | 17 + >> tests/networkxml2confdata/leasetime-seconds.xml | 18 ++ >> tests/networkxml2confdata/leasetime.conf | 17 + >> tests/networkxml2confdata/leasetime.xml | 18 ++ >> tests/networkxml2conftest.c | 7 ++ >> 18 files changed, 368 insertions(+), 3 deletions(-) >> create mode 100644 tests/networkxml2confdata/leasetime-days.conf >> create mode 100644 tests/networkxml2confdata/leasetime-days.xml >> create mode 100644 tests/networkxml2confdata/leasetime-hours.conf >> create mode 100644 tests/networkxml2confdata/leasetime-hours.xml >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf >> create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf >> create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf >> create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml >> create mode 100644 tests/networkxml2confdata/leasetime.conf >> create mode 100644 tests/networkxml2confdata/leasetime.xml >> >> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng >> index 1b4f980e7..8a76c235a 100644 >> --- a/docs/schemas/basictypes.rng >> +++ b/docs/schemas/basictypes.rng >> @@ -518,4 +518,20 @@ >> >> >> >> + >> + >> + seconds >> + minutes >> + hours >> + days >> + >> + Maybe call this "timeUnit" in case some other attribute in the future needs it? >> + >> + >> + >> + -1 >> + 4294967295 >> + >> + >> + >> >> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng >> index 1a18e64b2..4b8056ab6 100644 >> --- a/docs/schemas/network.rng >> +++ b/docs/schemas/network.rng >> @@ -340,6 +340,14 @@ >> >> >> + >> + >> + >> + > name="leaseTimeUnit"/> > > This does not follow the XML style used everywhere else. > >> + >> + >> + >> + >> >> >> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index aa397768c..6f051493f 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -30,6 +30,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "virerror.h" >> #include "datatypes.h" >> @@ -1031,12 +1033,82 @@ virNetworkDHCPHostDefParseXML(const char >> *networkName, >>
Re: [libvirt] [PATCH] tests: virstoragetest: fix --without-yajl
On Wed, Jun 21, 2017 at 09:03:06AM -0400, Cole Robinson wrote: > Recently added JSON tests should be skipped if compiled --without-yajl > > https://bugzilla.redhat.com/show_bug.cgi?id=1463435 > Signed-off-by: Cole Robinson ACK Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] qemu: Implement qemuDomainManagedSaveDefineXML
On 06/21/2017 01:02 PM, Peter Krempa wrote:
On Wed, Jun 21, 2017 at 05:07:49 +0530, Kothapally Madhu Pavan wrote:
This commit adds qemu driver implementation to edit xml
configuration of managed save state file of a domain.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_driver.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c9b3ef3..7ce6464 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6837,6 +6837,43 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom,
unsigned int flags)
return ret;
}
+static int
+qemuDomainManagedSaveDefineXML(virDomainPtr dom, const char *dxml,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virConnectPtr conn = dom->conn;
+virDomainObjPtr vm;
+char *path = NULL;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return -1;
+
+path = qemuDomainManagedSavePath(driver, vm);
+if (!path)
+goto error;
+
+virDomainObjEndAPI(&vm);
+
+if (conn->driver->domainSaveImageDefineXML) {
+int ret;
+ret = qemuDomainSaveImageDefineXML(conn, path, dxml, flags);
+
+VIR_FREE(path);
+
+if (ret < 0)
+goto error;
+return ret;
+}
This implementation is horrible. Please call the qemu implementation
directly or extract useful parts rather than going through the driver
pointers.
I was trying to follow the pattern of other APIs and I didn't realize
the pointers are not necessary. Thanks for the comments. Fixing that in
the next version.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/7] qemu: Implement qemuDomainManagedSaveGetXMLDesc
On 06/21/2017 01:05 PM, Peter Krempa wrote:
On Wed, Jun 21, 2017 at 05:07:48 +0530, Kothapally Madhu Pavan wrote:
This commit adds qemu driver implementaion to get xml description
for managed save state domain.
Signed-off-by: Kothapally Madhu Pavan
---
src/qemu/qemu_driver.c | 40
1 file changed, 40 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e91663c..c9b3ef3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6798,6 +6798,45 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const
char *path,
return ret;
}
+static char *
+qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+char *path = NULL;
+char *ret = NULL;
+virDomainDefPtr def = NULL;
+int fd = -1;
+virQEMUSaveDataPtr data = NULL;
+
+/* We only take subset of virDomainDefFormat flags. */
+virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return ret;
+
+path = qemuDomainManagedSavePath(driver, vm);
+
+if (!path)
+goto cleanup;
+
+fd = qemuDomainSaveImageOpen(driver, path, &def, &data,
+ false, NULL, false, false);
+if (fd < 0)
This will report a horrible error message in case when the VM is not
manage-saved. You need to check whether the file exists and report a
better one.
(error message will be reported by qemuOpenFileAs, thus will be
Failed to open file '%s')
Thanks for the comments. Fixing it in the next version.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 2/2] Resctrl: Add uitls functions to operate sysfs resctrl
On Wed, Jun 21, 2017 at 02:07:15PM +0800, Eli Qiao wrote: On Tuesday, 20 June 2017 at 8:39 PM, Martin Kletzander wrote: On Mon, Jun 12, 2017 at 05:48:41PM +0800, Eli Qiao wrote: > This patch adds 3 major private interface. > > virResctrlGetFreeCache: return free cache, default cache substract cache > allocated. > virResctrlSetCachetunes: set cache banks which defined in a domain. > virResctrlRemoveCachetunes: remove cache allocation group from the > host. > > There's some existed issue when do syntax-check as I reference the cache > tune and cachebank definition (from conf/domain_conf.h) in > util/virresctrl.h. > Yes, util/ cannot depend on conf/ in libvirt due to various reasons. All the data you want to use in util/ need to be defined there. If that corresponds to some XML, the parsers and formatters must be in conf/. In rare cases, there might be need for two data structures, one in util/ and one in conf/. I don't think that's needed in this case. I can move the virDomainCacheBank definition to util/virresctrl.h but what about the virCapsHostPtr, we need the host cache information and resctrl information it’s defined in src/conf/capabilities.h” Do we need to define another copy in virresctrl.h ? You don't need to pass the whole structure of all the data. Can't the qemu function just do something like: virResctrlLock() foreach cachetune: region = virResctrlGetFreeRegion(size, type) foreach cachetune.vcpu: virResctrlSetRegion(vcpu.pid, region) Or like with some other tuning, you can have a function that determines the region when given vcpu and just call it for all vcpus. You can save the regions in the status XML, so that not only users can see it, but you can also reference them from that aforementioned function. Or you could have saved pairs of id: region, but I think that's not needed. That reminds me, unless referred to from somewhere, the cachetune doesn't even need the id. But basically, you don't need to pass the whole cachetune or any other structure. The code is very messy, check your pointers and don't compare references to NULLs. Read the diffs after yourself. I know it works for you, but the code needs to be readable as well. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Do not skip virCPUUpdateLive if priv->origCPU is set
On Wed, Jun 21, 2017 at 04:00:44PM +0200, Jiri Denemark wrote: > Even though we got both the original CPU (used for starting a domain) > and the updated version (the CPU really provided by QEMU) during > incoming migration, restore, or snapshot revert, we still need to update > the CPU according to the data we got from the freshly started QEMU. > Otherwise we don't know whether the CPU we got from QEMU matches the one > before migration. We just need to keep the original CPU in > priv->origCPU. > > Messed up by me in v3.4.0-58-g8e34f4781. > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_process.c | 12 > 1 file changed, 4 insertions(+), 8 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Do not skip virCPUUpdateLive if priv->origCPU is set
Even though we got both the original CPU (used for starting a domain)
and the updated version (the CPU really provided by QEMU) during
incoming migration, restore, or snapshot revert, we still need to update
the CPU according to the data we got from the freshly started QEMU.
Otherwise we don't know whether the CPU we got from QEMU matches the one
before migration. We just need to keep the original CPU in
priv->origCPU.
Messed up by me in v3.4.0-58-g8e34f4781.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_process.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 63119396b..d669dfb32 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3989,20 +3989,16 @@ qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver,
if (qemuProcessVerifyCPUFeatures(def, cpu) < 0)
goto cleanup;
-/* Don't update the CPU if we already did so when starting a domain
- * during migration, restore or snapshot revert. */
-if (priv->origCPU) {
-ret = 0;
-goto cleanup;
-}
-
if (!(orig = virCPUDefCopy(def->cpu)))
goto cleanup;
if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, cpu, disabled)) <
0) {
goto cleanup;
} else if (rc == 0) {
-if (!virCPUDefIsEqual(def->cpu, orig, false))
+/* Store the original CPU in priv if QEMU changed it and we didn't
+ * get the original CPU via migration, restore, or snapshot revert.
+ */
+if (!priv->origCPU && !virCPUDefIsEqual(def->cpu, orig, false))
VIR_STEAL_PTR(priv->origCPU, orig);
def->cpu->check = VIR_CPU_CHECK_FULL;
--
2.13.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 2/2] Resctrl: Add uitls functions to operate sysfs resctrl
On Tue, Jun 20, 2017 at 03:38:15PM +0800, Eli Qiao wrote: hello, ping BTW this ping sent from gmail is both plaintext and html at the same time. Not only is it treating Makefile.am as domain name, that's not that big of a deal, but because you didn't trim it down, you made 36K patch into 91K message (quite a bit more than the five bytes needed for just "ping\n". signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/2] Resctrl: Add new xml element to support cache tune
On Mon, Jun 12, 2017 at 05:48:40PM +0800, Eli Qiao wrote:
This patch adds new xml element to support cache tune as:
...
The cache_id automatically implies level and type. Either have one or
the other. I know we talked about this already (maybe multiple times),
but without any clear outcome. For me the sensible thing is to have
level and type as that doesn't need to be changed when moving between
hosts, and if it cannot be migrated, then it's properly checked.
...
id: any non-minus number
cache_id: reference of the host's cache banks id, it's from capabilities
level: cache level
type: cache bank type
size: should be multiples of the min_size of the bank on host.
must be multiple of granularity, must be greater than or equal to minimum.
vcpus: cache allocation on vcpu set, if empty, will apply the allocation
on all vcpus
Signed-off-by: Eli Qiao
---
docs/schemas/domaincommon.rng | 54 +
src/conf/domain_conf.c| 131 ++
src/conf/domain_conf.h| 21 +++
3 files changed, 206 insertions(+)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4950ddc..11dbb55 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -834,6 +834,35 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
@@ -5686,6 +5715,31 @@
-1
+
+
+ [0-9]+
+
+
+
+
+ [0-9]+
+
+
These are useless ^^
+
+
+ (3)
Either don't restrict it at all or do the following:
3
+
+
+
+
+ (both|code|data)
+
+
both
code
data
+
+
+ KiB
+
+
This doesn't have to be in KiB, otherwise the unit doesn't make sense.
For all the units I'm sure there already is a define in the sources
somewhere.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5c32f93..93984bc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16250,6 +16250,104 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
return ret;
}
+/* Parse the XML definition for cachetune
+ * and a cachetune has the form
+ *
+ */
+static int
+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
+ int n,
+ xmlNodePtr* nodes)
+{
+char* tmp = NULL;
+size_t i;
+int type = -1;
+virDomainCacheBankPtr bank = NULL;
+
+if (VIR_ALLOC_N(bank, n) < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+if (!(tmp = virXMLPropString(nodes[i], "id"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing id in cache
tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].id)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache id '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "cache_id"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache id in cache
tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].cache_id)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache host id '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "level"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache level in cache
tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].level)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache level '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+
+if (!(tmp = virXMLPropString(nodes[i], "size"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing size in cache
tune"));
+goto cleanup;
+}
+if (virStrToLong_ull(tmp, NULL, 10, &(bank[i].size)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache size '%s'"), tmp);
Your error messages until now were almost consistent
+goto cleanup;
+}
+/* convert to B */
+bank[i].size *= 1024;
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "type"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("
[libvirt] [PATCH] tests: virstoragetest: fix --without-yajl
Recently added JSON tests should be skipped if compiled --without-yajl
https://bugzilla.redhat.com/show_bug.cgi?id=1463435
Signed-off-by: Cole Robinson
---
tests/virstoragetest.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 6c1287380..f34408395 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1359,6 +1359,8 @@ mymain(void)
"\n"
" \n"
"\n");
+
+#ifdef WITH_YAJL
TEST_BACKING_PARSE("json:", NULL);
TEST_BACKING_PARSE("json:asdgsdfg", NULL);
TEST_BACKING_PARSE("json:{}", NULL);
@@ -1573,6 +1575,7 @@ mymain(void)
"\n"
" \n"
"\n");
+#endif /* WITH_YAJL */
cleanup:
/* Final cleanup */
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] leasetime support for globally
Hello Peter,
Thanks for the comments, I will update the patch with your feedback, and
sorry I thought I rebased it before I sent it, will fix that too.
2017-06-21 8:27 GMT+01:00 Peter Krempa :
> On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote:
> > From: Alberto Ruiz
>
> Missing commit message.
>
> >
> > ---
> > docs/schemas/basictypes.rng | 16 +
> > docs/schemas/network.rng | 8 +++
> > src/conf/network_conf.c | 78
> ++-
> > src/conf/network_conf.h | 3 +-
> > src/network/bridge_driver.c | 49 +-
> > tests/networkxml2confdata/leasetime-days.conf | 17 +
> > tests/networkxml2confdata/leasetime-days.xml | 18 ++
> > tests/networkxml2confdata/leasetime-hours.conf| 17 +
> > tests/networkxml2confdata/leasetime-hours.xml | 18 ++
> > tests/networkxml2confdata/leasetime-infinite.conf | 17 +
> > tests/networkxml2confdata/leasetime-infinite.xml | 18 ++
> > tests/networkxml2confdata/leasetime-minutes.conf | 17 +
> > tests/networkxml2confdata/leasetime-minutes.xml | 18 ++
> > tests/networkxml2confdata/leasetime-seconds.conf | 17 +
> > tests/networkxml2confdata/leasetime-seconds.xml | 18 ++
> > tests/networkxml2confdata/leasetime.conf | 17 +
> > tests/networkxml2confdata/leasetime.xml | 18 ++
> > tests/networkxml2conftest.c | 7 ++
> > 18 files changed, 368 insertions(+), 3 deletions(-)
> > create mode 100644 tests/networkxml2confdata/leasetime-days.conf
> > create mode 100644 tests/networkxml2confdata/leasetime-days.xml
> > create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
> > create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
> > create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
> > create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
> > create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
> > create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
> > create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
> > create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
> > create mode 100644 tests/networkxml2confdata/leasetime.conf
> > create mode 100644 tests/networkxml2confdata/leasetime.xml
> >
> > diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> > index 1b4f980e7..8a76c235a 100644
> > --- a/docs/schemas/basictypes.rng
> > +++ b/docs/schemas/basictypes.rng
> > @@ -518,4 +518,20 @@
> >
> >
> >
> > +
> > +
> > + seconds
> > + minutes
> > + hours
> > + days
> > +
> > +
> > +
> > +
> > +
> > + -1
> > + 4294967295
> > +
> > +
> > +
> >
> > diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> > index 1a18e64b2..4b8056ab6 100644
> > --- a/docs/schemas/network.rng
> > +++ b/docs/schemas/network.rng
> > @@ -340,6 +340,14 @@
> >
> >
> > +
> > +
> > +
> > + name="leaseTimeUnit"/>
>
> This does not follow the XML style used everywhere else.
>
> > +
> > +
> > +
> > +
> >
> >
> > name="ipAddr"/>
> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > index aa397768c..6f051493f 100644
> > --- a/src/conf/network_conf.c
> > +++ b/src/conf/network_conf.c
> > @@ -30,6 +30,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> >
> > #include "virerror.h"
> > #include "datatypes.h"
> > @@ -1031,12 +1033,82 @@ virNetworkDHCPHostDefParseXML(const char
> *networkName,
> > return ret;
> > }
> >
> > +static int64_t
> > +virNetworkDHCPDefGetLeaseTime (xmlNodePtr node,
> > + xmlXPathContextPtr ctxt,
> > + bool *error)
>
> We usually return error from the function and if necessary return the
> value through pointer in arguments (backwards as you did).
>
> > +{
> > +int64_t multiplier, result = -1;
> > +char *leaseString, *leaseUnit;
> > +xmlNodePtr save;
> > +
> > +*error = 0;
> > +
> > +save = ctxt->node;
> > +ctxt->node = node;
> > +
> > +leaseString = virXPathString ("string(./leasetime/text())", ctxt);
> > +leaseUnit = virXPathString ("string(./leasetime/@unit)", ctxt);
> > +
> > +/* If value is not present we set the value to -2 */
> > +if (leaseString == NULL) {
> > +result = -2;
> > +goto cleanup;
> > +}
> > +
> > +if (leaseUnit == NULL || strcmp (leaseUnit, "seconds") == 0)
> > +multiplier = 1;
> > +else if (strcmp (leaseUnit, "minutes") == 0)
> > +multiplier = 60;
> > +else if (s
Re: [libvirt] [PATCH 0/3] Workaround mdev uevent race affecting node device driver
On Tue, Jun 20, 2017 at 01:59:59PM -0400, John Ferlan wrote: > > > On 06/20/2017 11:03 AM, Erik Skultety wrote: > > This series resolves https://bugzilla.redhat.com/show_bug.cgi?id=1463285 > > > > Erik Skultety (3): > > util: Report an error when virFileResolveLinkHelper's lstat fails > > util: Introduce virFileWaitForAccess > > nodedev: Work around the uevent race by hooking up > > virFileWaitForAccess > > > > src/libvirt_private.syms | 1 + > > src/node_device/node_device_udev.c | 48 > > +- > > src/util/virfile.c | 40 ++- > > src/util/virfile.h | 2 ++ > > 4 files changed, 89 insertions(+), 2 deletions(-) > > > > -- > > 2.13.1 > > > > -- > > libvir-list mailing list > > libvir-list@redhat.com > > https://www.redhat.com/mailman/listinfo/libvir-list > > > > FWIW: This seemed a bit familiar to something for NPIV as well. Although > for NPIV the files exist, it's just that they have bogus data. See: > > https://www.redhat.com/archives/libvir-list/2016-June/msg02213.html So I read the reply as well and though the argument about leaving kernel bugs to kernel to fix is right, this may take and indefinite time to actually get the fix and having an open kernel BZ is about it in terms what we can do about it. So, in order to make things work in the meantime, we have to work around things - discouraging? yes, ugly? absolutely, but unfortunately necessary. > > The referenced bz: > > https://bugzilla.redhat.com/show_bug.cgi?id=1319544 > > The settle code is used in a number of place in libvirt, search on > virWaitForDevices Thanks for the hint, I was looking for exactly something like this, but that function would not work in this case, because it would indeed wait for /dev/vfio/XY to get created, but for nodedev purposes we don't list those devices and mdevs are only matter of sysfs which is out of scope of udev. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessBuildDestroyHugepagesPath: Don't warn on destroying non-existent path
On Wed, Jun 21, 2017 at 11:43:55AM +0200, Michal Privoznik wrote:
This function is called unconditionally from qemuProcessStop to
make sure we leave no dangling dirs behind. However, whenever the
directory we want to rmdir() is not there (e.g. because it hasn't
been created in the first place because domain doesn't use
hugepages at all), we produce a warning like this:
2017-06-20 15:58:23.615+: 32638: warning :
qemuProcessBuildDestroyHugepagesPath:3363 : Unable to remove
hugepage path: /dev/hugepages/libvirt/qemu/1-instance-0001
(errno=2)
Fix this by not producing the warning on ENOENT.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_process.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
trivial;
Reviewed-by: Martin Kletzander
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fa9990e5d..3b7f20be4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3358,7 +3358,8 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr
driver,
goto cleanup;
}
} else {
-if (rmdir(hugepagePath) < 0)
+if (rmdir(hugepagePath) < 0 &&
+errno != ENOENT)
VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
hugepagePath, errno);
}
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [BUG] duplicate suspend/resume lifecycle events with QEMU
On Tue, Jun 13, 2017 at 10:30:59AM +0200, Philipp Hahn wrote:
Hello,
Am 13.06.2017 um 06:13 schrieb Philipp Hahn:
I'm using the libvirt event mechanism and noticed, that several events
are reported twice:
...
$ virsh event --loop --event lifecycle
event 'lifecycle' for domain installer: Started Booted
event 'lifecycle' for domain installer: Suspended Paused
event 'lifecycle' for domain installer: Resumed Unpaused
event 'lifecycle' for domain installer: Resumed Unpaused
event 'lifecycle' for domain installer: Suspended Paused
event 'lifecycle' for domain installer: Suspended Paused
event 'lifecycle' for domain installer: Stopped Destroyed
...
Looking at src/qemu/qemu_monitor_json.c:qemuMonitorJSONIOProcessEvent:
That was not the cause. Using gdb shows the real duplicator:
Breakpoint 9, virDomainEventLifecycleNew (id=10, name=0x7fffd825e700
"win8", uuid=0x7fffd825e128 "o8r\204\344\372F\263\214oGF\325@=9",
type=4, detail=0) at ../../../src/conf/domain_event.c:657
657 }
$13 = {parent = {parent = {parent = {u = {dummy_align1 = 7700611138,
dummy_align2 = 0x1cafe0042, s = {magic = 3405643842, refs = 1}}, klass =
0x558304e0}, eventID = 0, meta = {id = 10, name = 0x558304a0 "win8",
uuid = "o8r\204\344\372F\263\214oGF\325@=9", key =
0x55831270 "6f387284-e4fa-46b3-8c6f-4746d5403d39"}, remoteID = -1,
dispatch = 0x777d85c0 }, dummy =
false}, type = 4, detail = 0}
(gdb) bt
#0 virDomainEventLifecycleNew (id=10, name=0x7fffd825e700 "win8", uuid=0x7fffd825e128
"o8r\204\344\372F\263\214oGF\325@=9", type=4, detail=0) at
../../../src/conf/domain_event.c:657
#1 0x7fffe52fefef in qemuProcessHandleResume (mon=0x0, vm=0x7fffd825ec80,
opaque=0x7fffd8101b90) at ../../../src/qemu/qemu_process.c:773
#2 0x7fffe531c9c2 in qemuMonitorEmitResume (mon=0x7fffdea0) at
../../../src/qemu/qemu_monitor.c:1330
#3 0x7fffe532fd0b in qemuMonitorJSONIOProcessEvent (obj=,
mon=) at ../../../src/qemu/qemu_monitor_json.c:178
#4 qemuMonitorJSONIOProcessLine (mon=0x0, line=0x7fffe55d50e0
"}\242\071\345\377\177", msg=0xe) at ../../../src/qemu/qemu_monitor_json.c:207
#5 0x7fffe532fedb in qemuMonitorJSONIOProcess (mon=0x0, data=0x7774f9b0
"H\203\354\bH\215=\245\315G", len=119, msg=0x0) at
../../../src/qemu/qemu_monitor_json.c:249
#6 0x7fffe531b293 in qemuMonitorIOProcess (mon=) at
../../../src/qemu/qemu_monitor.c:432
#7 qemuMonitorIO (watch=18, fd=-134433088, events=1, opaque=0x7fffdea0) at
../../../src/qemu/qemu_monitor.c:686
#8 0x77735446 in virEventPollDispatchHandles (fds=,
nfds=) at ../../../src/util/vireventpoll.c:508
#9 virEventPollRunOnce () at ../../../src/util/vireventpoll.c:657
#10 0x77733b71 in virEventRunDefaultImpl () at
../../../src/util/virevent.c:314
#11 0x7788aced in virNetDaemonRun (dmn=0x55807d60) at
../../../src/rpc/virnetdaemon.c:818
#12 0x5556e486 in main (argc=-1, argv=0x0) at
../../../daemon/libvirtd.c:1547
(gdb) c
This is the QEMU monitor event handler
Continuing.
[Switching to Thread 0x7fffeee05700 (LWP 11193)]
Breakpoint 9, virDomainEventLifecycleNew (id=10, name=0x7fffd825e700 "win8",
uuid=0x7fffd825e128 "o8r\204\344\372F\263\214oGF\325@=9", type=4, detail=0) at
../../../src/conf/domain_event.c:657
657 }
$14 = {parent = {parent = {parent = {u = {dummy_align1 = 7700611138, dummy_align2 =
0x1cafe0042, s = {magic = 3405643842, refs = 1}}, klass = 0x558304e0}, eventID = 0,
meta = {id = 10, name = 0x7fffbc000a30 "win8",
uuid = "o8r\204\344\372F\263\214oGF\325@=9", key = 0x7fffbc000b70
"6f387284-e4fa-46b3-8c6f-4746d5403d39"}, remoteID = -1, dispatch = 0x777d85c0
}, dummy = false}, type = 4, detail = 0}
(gdb) bt
#0 virDomainEventLifecycleNew (id=10, name=0x7fffd825e700 "win8", uuid=0x7fffd825e128
"o8r\204\344\372F\263\214oGF\325@=9", type=4, detail=0) at
../../../src/conf/domain_event.c:657
#1 0x7fffe53579dc in qemuDomainResume (dom=0x7fffbc000930) at
../../../src/qemu/qemu_driver.c:1965
#2 0x77818acf in virDomainResume (domain=0x7fffbc000930) at
../../../src/libvirt-domain.c:679
#3 0x55595a3e in remoteDispatchDomainResume (server=, msg=, args=, rerr=, client=) at
../../../daemon/remote_dispatch.h:9132
#4 remoteDispatchDomainResumeHelper (server=0x55806d30,
client=0x55832970, msg=0x55837910, rerr=0x7fffeee04cc0,
args=0x7fffbc000930, ret=0x7fffbc000fc0) at
../../../daemon/remote_dispatch.h:9108
#5 0x77890939 in virNetServerProgramDispatchCall (msg=,
client=, server=, prog=) at
../../../src/rpc/virnetserverprogram.c:437
#6 virNetServerProgramDispatch (prog=0x5581be70, server=0x7774f9b0
, server@entry=0x55806d30, client=0x55832970,
msg=0x55837910) at ../../../src/rpc/virnetserverprogram.c:307
#7 0x555a7378 in virNetServerProcessMsg (msg=, prog=, client=, srv=0x55806d30) at ../../../src/rpc/virnetserver.c:148
#8 virNetServerHandleJob (jobOpaque=, opaque=0x55806d30) at
../../../s
Re: [libvirt] [PATCH] Revert "qemu: Check duplicate WWNs also for hotplugged disks"
On Wed, Jun 21, 2017 at 11:55:02AM +0200, Peter Krempa wrote:
> Similarly to commit 5da28cc3069b573f54f0bcaf8eb75476bcfdc6e9 this check
> actually does not make sense since duplicate WWNs are used e.g. when
> multipathing disks.
>
> This reverts commit 780fe4e4baf7e2f10f65ba1a34f9274fc547cad2.
> ---
> src/conf/domain_conf.c | 37 -
> 1 file changed, 37 deletions(-)
Is there something we can add to the hotplug test suite to illustrate
and/or validate this valid usage scenario.
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0409c62ef..fdf85d5dd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -25510,34 +25510,6 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr
> def ATTRIBUTE_UNUSED,
> return 0;
> }
>
> -
> -/**
> - * virDomainDefGetDiskByWWN:
> - * @def: domain definition
> - * @wwn: wwn of a disk to find
> - *
> - * Returns a disk definition pointer corresponding to the given WWN
> identifier
> - * or NULL either if @wwn was NULL or if disk with given WWN is not present
> in
> - * the domain definition.
> - */
> -static virDomainDiskDefPtr
> -virDomainDefGetDiskByWWN(virDomainDefPtr def,
> - const char *wwn)
> -{
> -size_t i;
> -
> -if (!wwn)
> -return NULL;
> -
> -for (i = 0; i < def->ndisks; i++) {
> -if (STREQ_NULLABLE(def->disks[i]->wwn, wwn))
> -return def->disks[i];
> -}
> -
> -return NULL;
> -}
> -
> -
> int
> virDomainDefCompatibleDevice(virDomainDefPtr def,
> virDomainDeviceDefPtr dev,
> @@ -25581,15 +25553,6 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
> }
> }
>
> -if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> -if (!!virDomainDefGetDiskByWWN(def, dev->data.disk->wwn)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Domain already has a disk with wwn '%s'"),
> - dev->data.disk->wwn);
> -return -1;
> -}
> -}
> -
> return 0;
> }
>
> --
> 2.12.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Revert "qemu: Check duplicate WWNs also for hotplugged disks"
Similarly to commit 5da28cc3069b573f54f0bcaf8eb75476bcfdc6e9 this check
actually does not make sense since duplicate WWNs are used e.g. when
multipathing disks.
This reverts commit 780fe4e4baf7e2f10f65ba1a34f9274fc547cad2.
---
src/conf/domain_conf.c | 37 -
1 file changed, 37 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0409c62ef..fdf85d5dd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25510,34 +25510,6 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
}
-
-/**
- * virDomainDefGetDiskByWWN:
- * @def: domain definition
- * @wwn: wwn of a disk to find
- *
- * Returns a disk definition pointer corresponding to the given WWN identifier
- * or NULL either if @wwn was NULL or if disk with given WWN is not present in
- * the domain definition.
- */
-static virDomainDiskDefPtr
-virDomainDefGetDiskByWWN(virDomainDefPtr def,
- const char *wwn)
-{
-size_t i;
-
-if (!wwn)
-return NULL;
-
-for (i = 0; i < def->ndisks; i++) {
-if (STREQ_NULLABLE(def->disks[i]->wwn, wwn))
-return def->disks[i];
-}
-
-return NULL;
-}
-
-
int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
@@ -25581,15 +25553,6 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
}
}
-if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
-if (!!virDomainDefGetDiskByWWN(def, dev->data.disk->wwn)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Domain already has a disk with wwn '%s'"),
- dev->data.disk->wwn);
-return -1;
-}
-}
-
return 0;
}
--
2.12.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemuProcessBuildDestroyHugepagesPath: Don't warn on destroying non-existent path
This function is called unconditionally from qemuProcessStop to
make sure we leave no dangling dirs behind. However, whenever the
directory we want to rmdir() is not there (e.g. because it hasn't
been created in the first place because domain doesn't use
hugepages at all), we produce a warning like this:
2017-06-20 15:58:23.615+: 32638: warning :
qemuProcessBuildDestroyHugepagesPath:3363 : Unable to remove
hugepage path: /dev/hugepages/libvirt/qemu/1-instance-0001
(errno=2)
Fix this by not producing the warning on ENOENT.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_process.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fa9990e5d..3b7f20be4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3358,7 +3358,8 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr
driver,
goto cleanup;
}
} else {
-if (rmdir(hugepagePath) < 0)
+if (rmdir(hugepagePath) < 0 &&
+errno != ENOENT)
VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
hugepagePath, errno);
}
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemuMonitorTextAddDrive: Fail on unrecognized disk format
Since qemu commit 3ef6c40ad0b it can fail if trying to hotplug a
disk that is not qcow2 despite us saying it is. We need to error
out in that case.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_monitor_text.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 66c94fbcd..737e8389b 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1963,6 +1963,12 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
goto cleanup;
}
+if (strstr(reply, "Image is not in")) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Incorrect disk format"));
+goto cleanup;
+}
+
ret = 0;
cleanup:
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] nodedev: Work around the uevent race by hooking up virFileWaitForAccess
> > + */
> > +static int
> > +udevWaitForAttrs(const char *sys_path, ...)
> > +{
> > +int ret = -1;
> > +const char *attr = NULL;
> > +char *attr_path = NULL;
> > +va_list args;
> > +
> > +va_start(args, sys_path);
> > +while ((attr = va_arg(args, char *))) {
> > +if (virAsprintf(&attr_path, "%s/%s", sys_path, attr) < 0)
> > +goto cleanup;
> > +
> > +if (virFileWaitForAccess(attr_path, 100, 10) < 0)
>
> So this waits up to 1 second per file in rather long increments (100
> ms) which I don't think is really desired.
>
> The only prior art here which I think is somewhat relevant is the
> waiting code for netdevs, where a 1 ms timeout with 100 retries is used.
>
> Also note that this will delay the event loop since the function is
> called by udevEventHandleCallback which is registered in the event loop.
> This is definitely unaceptable. NACK to this approach
Oh, I was in a rush writing this and missed that one completely, true, no
blocking in the eventloop, naturally. I'll try a different approach and respin.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] util: Introduce virFileWaitForAccess
On Tue, Jun 20, 2017 at 05:22:52PM +0200, Peter Krempa wrote:
> On Tue, Jun 20, 2017 at 17:03:31 +0200, Erik Skultety wrote:
> > Since we have a number of places where we workaround timing issues with
> > devices, attributes (files in general) not being available at the time
> > of processing them by calling usleep in a loop for a fixed number of
> > tries, we could as well have a utility function that would do that.
> > Therefore we won't have to duplicate this ugly workaround even more.
> >
> > This is a prerequisite for
> > https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
>
> This statement is useless. The helper function can be reused elsewhere.
>
> >
> > Signed-off-by: Erik Skultety
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virfile.c | 36
> > src/util/virfile.h | 2 ++
> > 3 files changed, 39 insertions(+)
>
> [...]
>
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index 6bbcc3d15..0b1a91699 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const char
> > *format, ...)
> > VIR_FREE(str);
> > return ret;
> > }
> > +
> > +
> > +/**
> > + * virFileWaitForAccess:
> > + * @path: absolute path to a sysfs attribute (can be a symlink)
> > + * @ms: how long to wait (in milliseconds)
> > + * @tries: how many times should we try to wait for @path to become
> > accessible
> > + *
> > + * Checks the existence of @path. In case the file defined by @path
> > + * doesn't exist, we wait for it to appear in 100ms (for up to @tries
> > times).
> > + *
> > + * Returns 0 on success, -1 on error (ENOENT is fine here).
>
> This description does not make sense. You don't state that this reports
> errors. Also the mention of ENOENT does not make sense.
>
> This function in fact sometimes returns enoent if the file does not
> appear until timeout.
>
> > + */
> > +int
> > +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> > +{
> > +errno = 0;
> > +
> > +/* wait for @path to be accessible in @ms milliseconds, up to @tries */
> > +while (tries-- > 0 && !virFileExists(path)) {
> > +if (errno != ENOENT) {
> > +virReportSystemError(errno, "%s", path);
>
> This does not really explain stuff to users. You might want to add a
> more comprehensive error message or leave error reporting to users.
>
> > +return -1;
> > +} else if (tries == 10) {
>
> This does not make any sense. The while loop counts down and checks if
> tries is more than 10. And this checks that tries is equal to 10.
>
> So if somebody passes 11 as @tries he/she gets this error? If tries is
> set to < 10 you get success even on timeout?
>
> Did you modify the code without testing it?
Damn :/, I actually tested it (after those 3+ modifications I did to it in an
iterative manner) and it worked, but surely didn't hit the issue you describe.
Nevertheless, yep, it's wrong and I need to rework it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] util: Introduce virFileWaitForAccess
On Tue, Jun 20, 2017 at 12:21:48PM -0400, Dawid Zamirski wrote:
> On Tue, 2017-06-20 at 17:45 +0200, Michal Privoznik wrote:
> > On 06/20/2017 05:21 PM, Dawid Zamirski wrote:
> > > On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote:
> > > > Since we have a number of places where we workaround timing
> > > > issues
> > > > with
> > > > devices, attributes (files in general) not being available at the
> > > > time
> > > > of processing them by calling usleep in a loop for a fixed number
> > > > of
> > > > tries, we could as well have a utility function that would do
> > > > that.
> > > > Therefore we won't have to duplicate this ugly workaround even
> > > > more.
> > > >
> > > > This is a prerequisite for
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
> > > >
> > > > Signed-off-by: Erik Skultety
> > > > ---
> > > > src/libvirt_private.syms | 1 +
> > > > src/util/virfile.c | 36
> > > >
> > > > src/util/virfile.h | 2 ++
> > > > 3 files changed, 39 insertions(+)
> > > >
> > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > > index c1e9471c5..53878a30f 100644
> > > > --- a/src/libvirt_private.syms
> > > > +++ b/src/libvirt_private.syms
> > > > @@ -1698,6 +1698,7 @@ virFileStripSuffix;
> > > > virFileTouch;
> > > > virFileUnlock;
> > > > virFileUpdatePerm;
> > > > +virFileWaitForAccess;
> > > > virFileWrapperFdClose;
> > > > virFileWrapperFdFree;
> > > > virFileWrapperFdNew;
> > > > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > > > index 6bbcc3d15..0b1a91699 100644
> > > > --- a/src/util/virfile.c
> > > > +++ b/src/util/virfile.c
> > > > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const
> > > > char *format, ...)
> > > > VIR_FREE(str);
> > > > return ret;
> > > > }
> > > > +
> > > > +
> > > > +/**
> > > > + * virFileWaitForAccess:
> > > > + * @path: absolute path to a sysfs attribute (can be a symlink)
> > > > + * @ms: how long to wait (in milliseconds)
> > > > + * @tries: how many times should we try to wait for @path to
> > > > become
> > > > accessible
> > > > + *
> > > > + * Checks the existence of @path. In case the file defined by
> > > > @path
> > > > + * doesn't exist, we wait for it to appear in 100ms (for up to
> > > > @tries times).
> > > > + *
> > > > + * Returns 0 on success, -1 on error (ENOENT is fine here).
> > > > + */
> > > > +int
> > > > +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> > > > +{
> > > > +errno = 0;
> > > > +
> > > > +/* wait for @path to be accessible in @ms milliseconds, up
> > > > to
> > > > @tries */
> > > > +while (tries-- > 0 && !virFileExists(path)) {
> > > > +if (errno != ENOENT) {
> > > > +virReportSystemError(errno, "%s", path);
> > > > +return -1;
> > > > +} else if (tries == 10) {
> > > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > + _("Failed to access '%s' after %zu
> > > > tries"),
> > > > + path, tries);
> > > > +return -1;
> > > > +} else {
> > > > +VIR_DEBUG("Failed to access '%s', re-try in %zu ms",
> > > > path, ms);
> > > > +usleep(ms * 1000);
> > > > +}
> > > > +}
> > > > +
> > > > +return 0;
> > > > +}
> > >
> > > Just FYI, there's another way to address it by calling udevadm
> > > settle
> > > before and after "touching" a block device, libguestfs is using
> > > this
> > > approach and it works very well:
> > >
> > > https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=ud
> > > ev_s
> > > ettle&type=
> >
> > Does it? udevadm settle waits for all the events to be processed, not
> > just the one that we want. The wait time would be unpredictable IMO.
> >
> > Michal
>
> Well it's a kind of brute-force approach but at least guarantees the
> device will be accessible once it exits. Why would setting a custom
> arbitrary timeout be better which may be too short? Is the predictable
> wait time here more important than being able to open the device at
> all?
>
So, there was this thing about udevadm settle that wasn't clear to me, because
it says it waits for udevd to create all the device nodes - so since udev only
manages "/dev", I assumed it wouldn't work for sysfs attributes, but gave it a
try since I wasn't sure and, no, we can't use udevadm here, because udevd really
doesn't care about sysfs (mdevs only live in sysfs - well, strictly speaking,
a corresponding /dev/vfio/ device IS created for an mdev, but for that we get a
separate event and because for node device purposes we don't report vfio
devices, we ignore such event) hence the explicit, unpredictable, best-effort
timeout here.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Support chardevs with ARM virt machines
On Wed, Jun 7, 2017 at 11:13 PM, Christoffer Dall wrote: > The function to check if -chardev is supported by QEMU was written a > long time ago, where adding chardevs did not make sense on the fixed ARM > platforms. Since then, we now have a general purpose virt platform, > which should support plugging in any device over PCIe which is supported > in a similar fashion on x86. > > Signed-off-by: Christoffer Dall > --- > src/qemu/qemu_capabilities.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 7f22492..1348af7 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -5507,6 +5507,11 @@ virQEMUCapsSupportsChardev(const virDomainDef *def, > if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != > VIR_ARCH_AARCH64)) > return true; > > +/* The virt machine has a PCIe bus and allows plugging in the same type > of > + * devices as x86 systems do on a PCIe bus. */ > +if (qemuDomainIsVirt(def)) > +return true; > + > /* This may not be true for all ARM machine types, but at least > * the only supported non-virtio serial devices of vexpress and versatile > * don't have the -chardev property wired up. */ > -- > 2.9.0 > ping? Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/7] qemu: Implement qemuDomainManagedSaveGetXMLDesc
On Wed, Jun 21, 2017 at 05:07:48 +0530, Kothapally Madhu Pavan wrote:
> This commit adds qemu driver implementaion to get xml description
> for managed save state domain.
>
> Signed-off-by: Kothapally Madhu Pavan
> ---
> src/qemu/qemu_driver.c | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e91663c..c9b3ef3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6798,6 +6798,45 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const
> char *path,
> return ret;
> }
>
> +static char *
> +qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags)
> +{
> +virQEMUDriverPtr driver = dom->conn->privateData;
> +virDomainObjPtr vm;
> +char *path = NULL;
> +char *ret = NULL;
> +virDomainDefPtr def = NULL;
> +int fd = -1;
> +virQEMUSaveDataPtr data = NULL;
> +
> +/* We only take subset of virDomainDefFormat flags. */
> +virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
> +
> +if (!(vm = qemuDomObjFromDomain(dom)))
> +return ret;
> +
> +path = qemuDomainManagedSavePath(driver, vm);
> +
> +if (!path)
> +goto cleanup;
> +
> +fd = qemuDomainSaveImageOpen(driver, path, &def, &data,
> + false, NULL, false, false);
> +if (fd < 0)
This will report a horrible error message in case when the VM is not
manage-saved. You need to check whether the file exists and report a
better one.
(error message will be reported by qemuOpenFileAs, thus will be
Failed to open file '%s')
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Change coalesce settings on hotplug when they are different
Part of the condition was reverted so no value update was propagated through. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1414627 Signed-off-by: Martin Kletzander --- Notes: Pushed as trivial src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5247c065711d..a486fb4fe334 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3283,8 +3283,8 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, if (!!olddev->coalesce != !!newdev->coalesce || (olddev->coalesce && newdev->coalesce && - !memcmp(olddev->coalesce, newdev->coalesce, - sizeof(*olddev->coalesce + memcmp(olddev->coalesce, newdev->coalesce, +sizeof(*olddev->coalesce needCoalesceChange = true; /* FINALLY - actually perform the required actions */ -- 2.13.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] qemu: Implement qemuDomainManagedSaveDefineXML
On Wed, Jun 21, 2017 at 05:07:49 +0530, Kothapally Madhu Pavan wrote:
> This commit adds qemu driver implementation to edit xml
> configuration of managed save state file of a domain.
>
> Signed-off-by: Kothapally Madhu Pavan
> ---
> src/qemu/qemu_driver.c | 38 ++
> 1 file changed, 38 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c9b3ef3..7ce6464 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6837,6 +6837,43 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom,
> unsigned int flags)
> return ret;
> }
>
> +static int
> +qemuDomainManagedSaveDefineXML(virDomainPtr dom, const char *dxml,
> + unsigned int flags)
> +{
> +virQEMUDriverPtr driver = dom->conn->privateData;
> +virConnectPtr conn = dom->conn;
> +virDomainObjPtr vm;
> +char *path = NULL;
> +
> +if (!(vm = qemuDomObjFromDomain(dom)))
> +return -1;
> +
> +path = qemuDomainManagedSavePath(driver, vm);
> +if (!path)
> +goto error;
> +
> +virDomainObjEndAPI(&vm);
> +
> +if (conn->driver->domainSaveImageDefineXML) {
> +int ret;
> +ret = qemuDomainSaveImageDefineXML(conn, path, dxml, flags);
> +
> +VIR_FREE(path);
> +
> +if (ret < 0)
> +goto error;
> +return ret;
> +}
This implementation is horrible. Please call the qemu implementation
directly or extract useful parts rather than going through the driver
pointers.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] leasetime support for globally
On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote:
> From: Alberto Ruiz
Missing commit message.
>
> ---
> docs/schemas/basictypes.rng | 16 +
> docs/schemas/network.rng | 8 +++
> src/conf/network_conf.c | 78
> ++-
> src/conf/network_conf.h | 3 +-
> src/network/bridge_driver.c | 49 +-
> tests/networkxml2confdata/leasetime-days.conf | 17 +
> tests/networkxml2confdata/leasetime-days.xml | 18 ++
> tests/networkxml2confdata/leasetime-hours.conf| 17 +
> tests/networkxml2confdata/leasetime-hours.xml | 18 ++
> tests/networkxml2confdata/leasetime-infinite.conf | 17 +
> tests/networkxml2confdata/leasetime-infinite.xml | 18 ++
> tests/networkxml2confdata/leasetime-minutes.conf | 17 +
> tests/networkxml2confdata/leasetime-minutes.xml | 18 ++
> tests/networkxml2confdata/leasetime-seconds.conf | 17 +
> tests/networkxml2confdata/leasetime-seconds.xml | 18 ++
> tests/networkxml2confdata/leasetime.conf | 17 +
> tests/networkxml2confdata/leasetime.xml | 18 ++
> tests/networkxml2conftest.c | 7 ++
> 18 files changed, 368 insertions(+), 3 deletions(-)
> create mode 100644 tests/networkxml2confdata/leasetime-days.conf
> create mode 100644 tests/networkxml2confdata/leasetime-days.xml
> create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
> create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
> create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
> create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
> create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
> create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
> create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
> create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
> create mode 100644 tests/networkxml2confdata/leasetime.conf
> create mode 100644 tests/networkxml2confdata/leasetime.xml
>
> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> index 1b4f980e7..8a76c235a 100644
> --- a/docs/schemas/basictypes.rng
> +++ b/docs/schemas/basictypes.rng
> @@ -518,4 +518,20 @@
>
>
>
> +
> +
> + seconds
> + minutes
> + hours
> + days
> +
> +
> +
> +
> +
> + -1
> + 4294967295
> +
> +
> +
>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 1a18e64b2..4b8056ab6 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -340,6 +340,14 @@
>
>
> +
> +
> +
> + name="leaseTimeUnit"/>
This does not follow the XML style used everywhere else.
> +
> +
> +
> +
>
>
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index aa397768c..6f051493f 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -30,6 +30,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include "virerror.h"
> #include "datatypes.h"
> @@ -1031,12 +1033,82 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
> return ret;
> }
>
> +static int64_t
> +virNetworkDHCPDefGetLeaseTime (xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + bool *error)
We usually return error from the function and if necessary return the
value through pointer in arguments (backwards as you did).
> +{
> +int64_t multiplier, result = -1;
> +char *leaseString, *leaseUnit;
> +xmlNodePtr save;
> +
> +*error = 0;
> +
> +save = ctxt->node;
> +ctxt->node = node;
> +
> +leaseString = virXPathString ("string(./leasetime/text())", ctxt);
> +leaseUnit = virXPathString ("string(./leasetime/@unit)", ctxt);
> +
> +/* If value is not present we set the value to -2 */
> +if (leaseString == NULL) {
> +result = -2;
> +goto cleanup;
> +}
> +
> +if (leaseUnit == NULL || strcmp (leaseUnit, "seconds") == 0)
> +multiplier = 1;
> +else if (strcmp (leaseUnit, "minutes") == 0)
> +multiplier = 60;
> +else if (strcmp (leaseUnit, "hours") == 0)
> +multiplier = 60 * 60;
> +else if (strcmp (leaseUnit, "days") == 0)
> +multiplier = 60 * 60 * 24;
> +else {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("invalid value for unit parameter in
> element found in network, "
> + "only 'seconds', 'minutes', 'hours' or 'days' are
> valid: %s"),
> + leaseUnit);
> +*error = 1;
>
