[libvirt] [PATCH go-xml] support virtualport for interface and add test code
---
domain.go | 29 +
domain_test.go | 4
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/domain.go b/domain.go
index eb7ff9e..ecb8550 100644
--- a/domain.go
+++ b/domain.go
@@ -185,19 +185,24 @@ type DomainInterfaceDriver struct {
Queues uint `xml:"queues,attr,omitempty"`
}
+type DomainInterfaceVirtualport struct {
+ Type string `xml:"type,attr"`
+}
+
type DomainInterface struct {
- XMLName xml.Name `xml:"interface"`
- Typestring `xml:"type,attr"`
- MAC *DomainInterfaceMAC`xml:"mac"`
- Model *DomainInterfaceModel `xml:"model"`
- Source *DomainInterfaceSource `xml:"source"`
- Target *DomainInterfaceTarget `xml:"target"`
- Alias *DomainInterfaceAlias `xml:"alias"`
- Link*DomainInterfaceLink `xml:"link"`
- Boot*DomainDeviceBoot `xml:"boot"`
- Script *DomainInterfaceScript `xml:"script"`
- Driver *DomainInterfaceDriver `xml:"driver"`
- Address *DomainAddress `xml:"address"`
+ XMLName xml.Name`xml:"interface"`
+ Typestring `xml:"type,attr"`
+ MAC *DomainInterfaceMAC `xml:"mac"`
+ Model *DomainInterfaceModel `xml:"model"`
+ Source *DomainInterfaceSource `xml:"source"`
+ Target *DomainInterfaceTarget `xml:"target"`
+ Alias *DomainInterfaceAlias `xml:"alias"`
+ Link*DomainInterfaceLink`xml:"link"`
+ Boot*DomainDeviceBoot `xml:"boot"`
+ Script *DomainInterfaceScript `xml:"script"`
+ Driver *DomainInterfaceDriver `xml:"driver"`
+ Virtualport *DomainInterfaceVirtualport `xml:"virtualport"`
+ Address *DomainAddress `xml:"address"`
}
type DomainChardevSource struct {
diff --git a/domain_test.go b/domain_test.go
index 6b37719..9de725c 100644
--- a/domain_test.go
+++ b/domain_test.go
@@ -565,6 +565,9 @@ var domainTestData = []struct {
Model: &DomainInterfaceModel{
Type: "virtio",
},
+ Virtualport:
&DomainInterfaceVirtualport{
+ Type: "openvswitch",
+ },
},
},
},
@@ -577,6 +580,7 @@ var domainTestData = []struct {
``,
` `,
` `,
+ ` `,
``,
` `,
``,
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On 06/15/2017 10:40 AM, Pavel Hrdina wrote: > On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote: >> >> >> On 06/15/2017 03:11 AM, Pavel Hrdina wrote: >>> On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote: On 05/29/2017 10:31 AM, Pavel Hrdina wrote: > In the case that virtlogd is used as stdio handler we pass to QEMU > only FD to a PIPE connected to virtlogd instead of the file itself. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 > > Signed-off-by: Pavel Hrdina > --- > > Notes: > new in v2 > > src/lxc/lxc_process.c| 6 ++--- > src/qemu/qemu_security.c | 9 +-- > src/security/security_apparmor.c | 7 -- > src/security/security_dac.c | 54 > +++- > src/security/security_driver.h | 6 +++-- > src/security/security_manager.c | 12 ++--- > src/security/security_manager.h | 6 +++-- > src/security/security_nop.c | 6 +++-- > src/security/security_selinux.c | 53 > ++- > src/security/security_stack.c| 12 ++--- > tests/securityselinuxlabeltest.c | 2 +- > 11 files changed, 127 insertions(+), 46 deletions(-) > Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why is (!chr_seclabel) even matter? >>> >>> If you configure the label we shouldn't ignore it in some cases, that's >>> just wrong. If the label is set for the char device we will configure >>> it every time even if it will fail to start the guest, it's a >>> responsibility of the user to configure proper label if it is provided. >>> >> >> When email doesn't convey the question... Ugh... I'm also trying to >> speed learn an area of the code and review at the same time. >> >> If I go back to commit id 'f8b08d0e' where the original implementation >> to add labels for chardevs was done (but has been modified for patch 1 >> to change where the label is stored), I get the impression that a label >> should be added either from something specifically supplied for the >> or to use the per domain one: >> >> "The source element may contain an optional seclabel to override the way >> that labelling is done on the socket path. If this element is not >> present, the security label is inherited from the per-domain setting." >> >> So if I look at the condition "(!chr_seclabel && chardevStdioLogd)" >> added by this patch to decide whether or not to supply the label, I'm >> left with the impression that if for this particular chardev a label >> doesn't exist *and* the configuration option chardevStdioLogd is true, >> then we're going to return happy status *and* not inherit the per-domain >> setting. >> >> So the bug is then that applying the default domain label for a chardev >> configured to use a stdio handle is incorrect? Perhaps I didn't get >> that from reading bz or the patch. > > Yes, that's the bug. If virtlogd is used to handle stdio for char > devices we shouldn't relabel the @path to the default labels, the > @path doesn't have to be accessible by the qemu process, it has to be > accessible by the virtlogd process. > IIUC, whether or not someone set the label for the chardev, for this particular issue/config where the chardev has a , we don't want to set (or restore) the label. I feel like I'm missing something subtle. Maybe a bit more explanation of the adjustment would help me... >>> >>> This is not for the but for the . >>> We don't relabel $path for at all. >>> >> >> hmm.. ah, right... I kept scrolling back and forth in the bz and the >> docs, but missed this in the bz: >> >> 3) Check the virtlogd.log: >> error : virRotatingFileWriterEntryNew:113 : Unable to open file: >> /var/log/libvirt/qemu/log: Permission denied >> >> I guess I got lost in the power of suggestion of reading the docs >> regarding the "optional log file" that can be associated paragraph and >> trying to learn on the fly so that you at least get a review in a >> somewhat timely manner ;-) >> Wouldn't these changes end up selecting "any" chardev if chardevStdioLogd ended up being set regardless of whether they were actually using the log file? >>> >>> I don't know what you mean by this sentence? >>> >> >> Well let's see, chardevStdioLogd is set to true when meeting the two >> conditions a qemu.conf global "cfg->stdioLogd" && a per domain or >> emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND". >> >> So, conceivably chardevStdioLogd could be true for *any* domain as long >> as those conditions are met, right? > > Yes, the two conditions are checked while starting a new domain in > qemuProcessPrepareDomain() and stored in the private date of that > domain. > >> If you have a domain that has chardev's which are not configured to use >> the stdio handler, then the chardevStdioLogd could still be true, right? >
Re: [libvirt] [PATCH v2 7/8] util: Introduce virObjectPoolableDef
On 06/05/2017 08:29 AM, Peter Krempa wrote:
> On Fri, Jun 02, 2017 at 06:17:21 -0400, John Ferlan wrote:
>> Add a new virObjectPoolableHashElement child which will be used to provide
>> the basis for driver def/newDef elements.
>>
>> Each virObjectPoolableDef has:
>>
>> 1. A required @primaryKey to be used to uniquely identity the object
>> by some string value.
>> 2. An optional @secondaryKey to be used as a secondary means of search
>> for the object by some string value.
>> 3. A required @def and @defFreeFunc. The @def will be consumed by the
>> object and when disposed the free function will be called.
>>
>> The _virObjectPoolableDef has an additional @newDef element to store
>> the "next" boot configuration for consumers that support the functionality.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/libvirt_private.syms | 2 ++
>> src/util/virobject.c | 83
>> +++-
>> src/util/virobject.h | 24 ++
>> 3 files changed, 108 insertions(+), 1 deletion(-)
>
> [...]
>
>> @@ -69,10 +72,22 @@ struct _virObjectPoolableHashElement {
>> char *secondaryKey;
>> };
>>
>> +struct _virObjectPoolableDef {
>> +virObjectPoolableHashElement parent;
>> +
>> +/* 'def' is the current config definition.
>> + * 'newDef' is the next boot configuration.
>
> 'boot' does not really apply to anything besides VMs.
>
Right - semantics it's just a "next" possible configuration (domain,
storage, network, and nwfilter have them)...
>> + */
>> +void *def;
>> +void *newDef;
>> +virFreeCallback defFreeFunc;
>> +};
>
> Okay, this is another sign that this abstraction has gone too far. Using
> void pointers here for the definition pointers really does not help
> stuff. You need wrappers to do compile time type safety checks here, so
> I don't really see the value to wrap it into a object with such
> opaqueness.
>
Assume for a moment that the previous patches have the following:
+struct _virObjectLookupKeys {
+virObjectLockable parent;
+
+char *uuid;
+char *name;
+};
+
and everything that goes with it. Essentially, exchange
PoolableHashElement with LookupKeys.
So this presents an interesting quandary. The goal is to have an object
to manage essentially common things between all _vir*Obj structures;
however, those common things are pointers to driver/object specific
definition data. Still in the long run the object isn't *managing* the
data it's merely acting as a storage vessel for common data allowing the
consumer to handle the rest of the details.
If I consider what you wrote it response to patch 5, there would be a
union of sorts:
union {
virNodeDeviceDefPtr nodedev;
virSecretDefPtr secret;
virDomainDefPtr domain;
virNetworkDefPtr network;
virInterfaceDefPtr interface;
virNWFilterDefPtr nwfilter;
virStoragePoolDefPtr pool;
virStorageVolDefPtr volume;
} def;
where def is placed into some new Object:
struct _virObject {
virObjectLookupKeys parent;
virObject???Type deftype;
union {} def; (8 types)
virObject???Type newDeftype;
union {} newDef; (only 4 types)
};
That's all well and good, but the object code doesn't need nor does it
want to manage anything w/r/t the specifics of the @def's.
So the only reason to be able 'type' the @def would seem to be to have
some sort of compile time safety check that (for example) networks
aren't using domain object data.
Even with the type'd @def/@newDef unions, unless there's going to be
APIs in the object for each type of definition, how does one "compile
time" set or get those objects other than using a #define as a wrapper
for at least fetch. How set works in an API I don't have a mental
picture yet beyond passing a @def as a "void *". Perhaps someone else
has some brilliant idea.
I suppose another option is 8 separate klasses for each of the various
types of driver/vir*obj that would consume them. Still that seems
overkill and unnecessary since the original object could store just as
easily. The whole purpose of a single object is to store "something"
that the consumer can use. That something would need a FreeFunc since we
don't know what it is.
I'm still preferential to the current model but perhaps add a @type
parameter to the object and to the various get/set API's for some level
of type checking, but I don't believe that's what's being asked for.
Hopefully by responding again some conversation is generated. While you
consider being generic going to far in one direction, I see typed values
as no change from the current. Of course I have in mind about 8-10 steps
after these patches - so I'm currently blinded by the chosen mechanism.
Tks -
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: implement virStrToDoubleSafe().
Thanks to share the commit SHA, Martin.
My only doubt is: I speak Brazilian portuguese (so pt_BR-Latin
America and we use comma as separator). Should libvirt consider
it as a separator? Or only dot? Because this approach fails to parse
12,34 (12.34 US) for example.
2017-06-15 5:27 GMT-03:00 Martin Kletzander :
> On Wed, Jun 14, 2017 at 01:24:42PM -0300, Julio Faracco wrote:
>>
>> Following the GNU Documentation, functions to convert double/float to
>> string
>> and vice versa, use the locale to set the mantissa separator. Some
>> languages
>> use comma and other use dot as a separator.
>>
>> For example: 1,212.67 (en_US) = 1.112,67 (pt_BR).
>>
>> This can be used to parse values in float/double from XML and other
>> definition
>> files.
>>
>> Signed-off-by: Julio Faracco
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virstring.c | 31 +++
>> src/util/virstring.h | 4
>> 3 files changed, 36 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 044510f..9d791e6 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2659,6 +2659,7 @@ virStringTrimOptionalNewline;
>> virStrncpy;
>> virStrndup;
>> virStrToDouble;
>> +virStrToDoubleSafe;
>> virStrToLong_i;
>> virStrToLong_l;
>> virStrToLong_ll;
>> diff --git a/src/util/virstring.c b/src/util/virstring.c
>> index 089b539..6dad00f 100644
>> --- a/src/util/virstring.c
>> +++ b/src/util/virstring.c
>> @@ -537,6 +537,37 @@ virStrToDouble(char const *s,
>> }
>>
>> int
>> +virStrToDoubleSafe(char const *s,
>> + char **end_ptr,
>> + double *result)
>> +{
>> +char *cur_locale = NULL;
>> +char *saved_locale = NULL;
>> +int ret = -1;
>> +
>> +cur_locale = setlocale (LC_ALL, NULL);
>> +if (!cur_locale)
>> +return ret;
>> +
>> +if (VIR_STRDUP(saved_locale, cur_locale) < 0)
>> +goto cleanup;
>> +
>> +if (!setlocale (LC_ALL, "C"))
>> +goto cleanup;
>> +
>
>
> This is MT-Unsafe. We cannot set this for the whole process, so NACK to
> this approach. You need to use uselocale() as in commit 43bfa23e6f96
> (or just see virDoubleToStr()).
>
> Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Force reading of meta data to get encryption capacity value
https://bugzilla.redhat.com/show_bug.cgi?id=1371892
As it turns out the volume create, build, and refresh path was not peeking
at the meta data, so immediately after a create operation the value displayed
for capacity was still incorrect. However, if a pool refresh was done the
correct value was fetched as a result of a meta data peek.
The reason is it seems historically if the file type is RAW then peeking
at the file just took the physical value for the capacity. However, since
we know if it's an encrypted file, then peeking at the meta data will be
required in order to get a true capacity value.
So check for encryption in the source and if present, use the meta data
in order to fill in the capacity value and set the payload_offset.
Signed-off-by: John Ferlan
---
src/util/virstoragefile.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index e82a7fb..11c3625 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3446,13 +3446,16 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src,
src->format = format;
}
-if (format == VIR_STORAGE_FILE_RAW)
+if (format == VIR_STORAGE_FILE_RAW && !src->encryption) {
src->capacity = src->physical;
-else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf,
- len, format, NULL)))
+} else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf,
+len, format, NULL))) {
src->capacity = meta->capacity ? meta->capacity : src->physical;
-else
+if (src->encryption && meta->encryption)
+src->encryption->payload_offset = meta->encryption->payload_offset;
+} else {
goto cleanup;
+}
if (src->encryption && src->encryption->payload_offset != -1)
src->capacity -= src->encryption->payload_offset * 512;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Creating mediated devices with libvirt
On Thu, 15 Jun 2017 09:33:01 +0100 "Daniel P. Berrange" wrote: > On Thu, Jun 15, 2017 at 12:06:43AM +0200, Erik Skultety wrote: > > Hi all, > > > > so there's been an off-list discussion about finally implementing creation > > of > > mediated devices with libvirt and it's more than desired to get as many > > opinions > > on that as possible, so please do share your ideas. This did come up > > already as > > part of some older threads ([1] for example), so this will be a respin of > > the > > discussions. Long story short, we decided to put device creation off and > > focus > > on the introduction of the framework as such first and build upon that > > later, > > i.e. now. > > > > [1] https://www.redhat.com/archives/libvir-list/2017-February/msg00177.html > > > > > > PART 1: NODEDEV-DRIVER > > > > > > API-wise, device creation through the nodedev driver should be pretty > > straightforward and without any issues, since virNodeDevCreateXML takes an > > XML > > and does support flags. Looking at the current device XML: > > > > > > mdev_0cce8709_0640_46ef_bd14_962c7f73cc6f > > > > /sys/devices/pci:00/.../0cce8709-0640-46ef-bd14-962c7f73cc6f > > pci__03_00_0 > > > > vfio_mdev > > > > > > > > > > UUID > > > > > > > > We can ignore ,, elements, since these are useless > > during creation. We also cannot use since we don't support arbitrary > > names and we also can't rely on users providing a name in correct form > > which we > > would need to further parse in order to get the UUID. > > So since the only thing missing to successfully use create an mdev using > > XML is > > the UUID (if user doesn't want it to be generated automatically), how about > > having a subelement under just like PCIs have > > and > > friends, USBs have & , interfaces have to uniquely > > identify the device even if the name itself is unique. > > Removal of a device should work as well, although we might want to > > consider creating a *Flags version of the API. > > > > = > > PART 2: DOMAIN XML & DEVICE AUTO-CREATION, NO POLICY INVOLVED! > > = > > > > There were some doubts about auto-creation mentioned in [1], although they > > weren't specified further. So hopefully, we'll get further in the discussion > > this time. > > > > From my perspective there are two main reasons/benefits to that: > > > > 1) Convenience > > For apps like virt-manager, user will want to add a host device > > transparently, > > "hey libvirt, I want an mdev assigned to my VM, can you do that". Even for > > higher management apps, like oVirt, even they might not care about the > > parent > > device at all times and considering that they would need to enumerate the > > parents, pick one, create the device XML and pass it to the nodedev driver, > > IMHO > > it would actually be easier and faster to just do it directly through sysfs, > > bypassing libvirt once again > > The convenience only works if the policy we've provided in libvirt actually > matches the policy the application wants. I think it is quite likely that with > cloud the mdevs will be created out of band from the domain startup process. > It is possible the app will just have a fixed set of mdevs pre-created when > the host starts up. Or that the mgmt app wants the domain startup process to > be a two phase setup, where it first allocates the resources needed, and later > then tries to start the guest. This is why I keep saying that putting this > kind > of "convenient" policy in libvirt is a bad idea - it is essentially just > putting > a bit of virt-manager code into libvirt - more advanced apps will need more > flexibility in this area. > > > 2) Future domain migration > > Suppose now that the mdev backing physical devices support state dump and > > reload. Chances are, that the corresponding mdev doesn't even exist or has a > > different UUID on the destination, so libvirt would do its best to handle > > this > > before the domain could be resumed. > > This is not an unusual scenario - there are already many other parts of the > device backend config that need to change prior to migration, especially for > anything related to host devices, so apps already have support for doing > this, which is more flexible & convenient becasue it doesn't tie creation of > the mdevs to running of the migrate command. > > IOW, I'm still against adding any kind of automatic creation policy for > mdevs in libvirt. Just provide the node device API support. I'm not super clear on the extent of what you're against here, is it all forms of device creation or only a placement policy? Are you against any form of having the XML specify the non-instantiated mdev that it wants? We've clearly made an important step with libvirt supportin
Re: [libvirt] [PATCH] maint: update to latest gnulib
On Thu, Jun 15, 2017 at 05:39:00PM +0200, Martin Kletzander wrote:
> On Wed, Jun 14, 2017 at 03:35:39PM +0100, Daniel P. Berrange wrote:
> > On Wed, Jun 14, 2017 at 04:30:49PM +0200, Martin Kletzander wrote:
> > > On Wed, Jun 14, 2017 at 02:32:41PM +0100, Daniel P. Berrange wrote:
> > > > On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
> > > > > On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
> > > > > > This fixes an incompatibility with glibc 2.25.90
> > > > > >
> > > > > > Signed-off-by: Daniel P. Berrange
> > > > > > ---
> > > > > >
> > > > > > Pushed as a broken build fix to get CI back online
> > > > > >
> > > > >
> > > > > After this update the build fails for me with gcc-7.1.0 with the
> > > > > following error:
> > > > >
> > > > > In file included from util/virobject.c:28:0:
> > > > > util/virobject.c: In function 'virClassNew':
> > > > > util/viratomic.h:176:46: error: this condition has identical branches
> > > > > [-Werror=duplicated-branches]
> > > > > (void)(0 ? *(atomic) ^ *(atomic) : 0);
> > > > > \
> > > > > ^
> > > > > util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc'
> > > > > klass->magic = virAtomicIntInc(&magicCounter);
> > > > >^~~
> > > > >
> > > > > Does that mean that gcc does optimize our prefetch trick away
> > > > > (considering I understood what that line is trying to do)? Or should
> > > > > we
> > > > > just turn the warning off for that header file?
> > > >
> > > > Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1
> > > > which gnulib turns on. There's a similar hit with mingw
> > > >
> > > > ../../src/util/vircommand.c: In function 'virCommandWait':
> > > > ../../src/util/vircommand.c:2562:51: error: this condition has
> > > > identical branches [-Werror=duplicated-branches]
> > > > *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status);
> > > > ^
> > > > cc1: all warnings being treated as errors
> > > >
> > > >
> > > > because WEXITSTATUS(x) expands to 'x' on Win32.
> > > >
> > > > We could use a pragma to turn off selectively, but I'm more
> > > > inclined to just disable this new warning flag.
> > > >
> > >
> > > Well, I'm not sure how that affects the line where we actually use it
> > > (with the atomic variables) or whether that line is not needed anymore
> > > (if that was a fix for older compilers or something similar). But I
> > > can send a patch for removing that warning. How about the other
> > > warning we get when we turn off the first one? I just found out. I
> > > think that could be turned off as well, either for some particular
> > > places or for the whole build:
> > >
> > > util/virtime.c: In function 'virTimeStringThenRaw':
> > > util/virtime.c:215:9: error: '%02d' directive output may be truncated
> > > writing between 2 and 11 bytes into a region of size between 5 and 21
> > > [-Werror=format-truncation=]
> > > if (snprintf(buf, VIR_TIME_STRING_BUFLEN,
> > > ^
> > > "%4d-%02d-%02d %02d:%02d:%02d.%03d+",
> > > ~
> > > fields.tm_year, fields.tm_mon, fields.tm_mday,
> > > ~~
> > > fields.tm_hour, fields.tm_min, fields.tm_sec,
> > > ~
> > > (int) (when % 1000)) >= VIR_TIME_STRING_BUFLEN) {
> > >
> > > util/virtime.c:215:9: note: using the range [-2147483648, 2147483647] for
> > > directive argument
> > > In file included from /usr/include/stdio.h:936:0,
> > > from ../gnulib/lib/stdio.h:43,
> > > from util/virtime.c:36:
> > > /usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output
> > > between 29 and 89 bytes into a destination of size 29
> > > return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
> > > ^~~~
> > >__bos (__s), __fmt, __va_arg_pack ());
> > >~
> >
> > I think we might just want to switch to asprintf here instead of trying to
> > optimize into a fixed stack allocated buffer.
> >
>
> I wanted to do that and started rewriting it, but I found out we use
> static buffers in lot of places and lot of them then use snprintf or
> similar. In some cases it doesn't even make much of a sense, e.g.:
>
> snprintf(buf, 3, "%d", var)
>
> even this can fail. I get the reason for the warning, but I don't
> really agree that it's something that is necessary to avoid, so I'm
> inclining to ignoring that warning as well.
In that kind of scenario we should be using IN
Re: [libvirt] [PATCH] Temporarily disable format truncation warnings
On Thu, Jun 15, 2017 at 02:33:34PM +0100, Daniel P. Berrange wrote: GCC 7.1 introduces a new -Wformat-truncation warning flag that reports if it thinks the maximum possible size of the formatted output will exceed the provided fixed buffer. This is enabled automatically by the -Wformat warning flag. There are quite a few places hit by this in libvirt which need rewriting. This is non-trivial work in some places, so temporarily disable the new warning until those fixes can be implemented. Signed-off-by: Daniel P. Berrange --- m4/virt-compile-warnings.m4 | 4 1 file changed, 4 insertions(+) I was missing some mails when I was suggesting the same in another thread. ACK from me, hopefully this will just be temporary. Maybe we should add an idea to bugzilla or wiki to the list of TODOs? diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d7bb172f3..fa0940fc6 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -169,6 +169,10 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ wantwarn="$wantwarn -Wno-format" fi +# -Wformat enables this by default, and we should keep it, +# but need to rewrite various areas of code first +wantwarn="$wantwarn -Wno-format-truncation" + # This should be < 256 really. Currently we're down to 4096, # but using 1024 bytes sized buffers (mostly for virStrerror) # stops us from going down further -- 2.13.1 -- 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] [PATCH] maint: update to latest gnulib
On Wed, Jun 14, 2017 at 03:35:39PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 04:30:49PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 02:32:41PM +0100, Daniel P. Berrange wrote:
> On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
> > On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
> > > This fixes an incompatibility with glibc 2.25.90
> > >
> > > Signed-off-by: Daniel P. Berrange
> > > ---
> > >
> > > Pushed as a broken build fix to get CI back online
> > >
> >
> > After this update the build fails for me with gcc-7.1.0 with the
> > following error:
> >
> > In file included from util/virobject.c:28:0:
> > util/virobject.c: In function 'virClassNew':
> > util/viratomic.h:176:46: error: this condition has identical branches
[-Werror=duplicated-branches]
> > (void)(0 ? *(atomic) ^ *(atomic) : 0); \
> > ^
> > util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc'
> > klass->magic = virAtomicIntInc(&magicCounter);
> >^~~
> >
> > Does that mean that gcc does optimize our prefetch trick away
> > (considering I understood what that line is trying to do)? Or should we
> > just turn the warning off for that header file?
>
> Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1
> which gnulib turns on. There's a similar hit with mingw
>
> ../../src/util/vircommand.c: In function 'virCommandWait':
> ../../src/util/vircommand.c:2562:51: error: this condition has identical
branches [-Werror=duplicated-branches]
> *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status);
> ^
> cc1: all warnings being treated as errors
>
>
> because WEXITSTATUS(x) expands to 'x' on Win32.
>
> We could use a pragma to turn off selectively, but I'm more
> inclined to just disable this new warning flag.
>
Well, I'm not sure how that affects the line where we actually use it
(with the atomic variables) or whether that line is not needed anymore
(if that was a fix for older compilers or something similar). But I
can send a patch for removing that warning. How about the other
warning we get when we turn off the first one? I just found out. I
think that could be turned off as well, either for some particular
places or for the whole build:
util/virtime.c: In function 'virTimeStringThenRaw':
util/virtime.c:215:9: error: '%02d' directive output may be truncated writing
between 2 and 11 bytes into a region of size between 5 and 21
[-Werror=format-truncation=]
if (snprintf(buf, VIR_TIME_STRING_BUFLEN,
^
"%4d-%02d-%02d %02d:%02d:%02d.%03d+",
~
fields.tm_year, fields.tm_mon, fields.tm_mday,
~~
fields.tm_hour, fields.tm_min, fields.tm_sec,
~
(int) (when % 1000)) >= VIR_TIME_STRING_BUFLEN) {
util/virtime.c:215:9: note: using the range [-2147483648, 2147483647] for
directive argument
In file included from /usr/include/stdio.h:936:0,
from ../gnulib/lib/stdio.h:43,
from util/virtime.c:36:
/usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output
between 29 and 89 bytes into a destination of size 29
return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
^~~~
__bos (__s), __fmt, __va_arg_pack ());
~
I think we might just want to switch to asprintf here instead of trying to
optimize into a fixed stack allocated buffer.
I wanted to do that and started rewriting it, but I found out we use
static buffers in lot of places and lot of them then use snprintf or
similar. In some cases it doesn't even make much of a sense, e.g.:
snprintf(buf, 3, "%d", var)
even this can fail. I get the reason for the warning, but I don't
really agree that it's something that is necessary to avoid, so I'm
inclining to ignoring that warning as well.
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 :|
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Thu, Jun 15, 2017 at 04:40:49PM +0200, Pavel Hrdina wrote: > On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote: > > > > > > On 06/15/2017 03:11 AM, Pavel Hrdina wrote: > > > On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote: > > >> > > >> > > >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote: > > >>> In the case that virtlogd is used as stdio handler we pass to QEMU > > >>> only FD to a PIPE connected to virtlogd instead of the file itself. > > >>> > > >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 > > >>> > > >>> Signed-off-by: Pavel Hrdina > > >>> --- > > >>> > > >>> Notes: > > >>> new in v2 > > >>> > > >>> src/lxc/lxc_process.c| 6 ++--- > > >>> src/qemu/qemu_security.c | 9 +-- > > >>> src/security/security_apparmor.c | 7 -- > > >>> src/security/security_dac.c | 54 > > >>> +++- > > >>> src/security/security_driver.h | 6 +++-- > > >>> src/security/security_manager.c | 12 ++--- > > >>> src/security/security_manager.h | 6 +++-- > > >>> src/security/security_nop.c | 6 +++-- > > >>> src/security/security_selinux.c | 53 > > >>> ++- > > >>> src/security/security_stack.c| 12 ++--- > > >>> tests/securityselinuxlabeltest.c | 2 +- > > >>> 11 files changed, 127 insertions(+), 46 deletions(-) > > >>> > > >> > > >> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why > > >> is (!chr_seclabel) even matter? > > > > > > If you configure the label we shouldn't ignore it in some cases, that's > > > just wrong. If the label is set for the char device we will configure > > > it every time even if it will fail to start the guest, it's a > > > responsibility of the user to configure proper label if it is provided. > > > > > > > When email doesn't convey the question... Ugh... I'm also trying to > > speed learn an area of the code and review at the same time. > > > > If I go back to commit id 'f8b08d0e' where the original implementation > > to add labels for chardevs was done (but has been modified for patch 1 > > to change where the label is stored), I get the impression that a label > > should be added either from something specifically supplied for the > > or to use the per domain one: > > > > "The source element may contain an optional seclabel to override the way > > that labelling is done on the socket path. If this element is not > > present, the security label is inherited from the per-domain setting." > > > > So if I look at the condition "(!chr_seclabel && chardevStdioLogd)" > > added by this patch to decide whether or not to supply the label, I'm > > left with the impression that if for this particular chardev a label > > doesn't exist *and* the configuration option chardevStdioLogd is true, > > then we're going to return happy status *and* not inherit the per-domain > > setting. > > > > So the bug is then that applying the default domain label for a chardev > > configured to use a stdio handle is incorrect? Perhaps I didn't get > > that from reading bz or the patch. > > Yes, that's the bug. If virtlogd is used to handle stdio for char > devices we shouldn't relabel the @path to the default labels, the > @path doesn't have to be accessible by the qemu process, it has to be > accessible by the virtlogd process. > > > >> IIUC, whether or not someone set the label for the chardev, for this > > >> particular issue/config where the chardev has a , we > > >> don't want to set (or restore) the label. I feel like I'm missing > > >> something subtle. Maybe a bit more explanation of the adjustment would > > >> help me... > > > > > > This is not for the but for the . > > > We don't relabel $path for at all. > > > > > > > hmm.. ah, right... I kept scrolling back and forth in the bz and the > > docs, but missed this in the bz: > > > > 3) Check the virtlogd.log: > > error : virRotatingFileWriterEntryNew:113 : Unable to open file: > > /var/log/libvirt/qemu/log: Permission denied > > > > I guess I got lost in the power of suggestion of reading the docs > > regarding the "optional log file" that can be associated paragraph and > > trying to learn on the fly so that you at least get a review in a > > somewhat timely manner ;-) > > > > >> Wouldn't these changes end up selecting "any" chardev if > > >> chardevStdioLogd ended up being set regardless of whether they were > > >> actually using the log file? > > > > > > I don't know what you mean by this sentence? > > > > > > > Well let's see, chardevStdioLogd is set to true when meeting the two > > conditions a qemu.conf global "cfg->stdioLogd" && a per domain or > > emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND". > > > > So, conceivably chardevStdioLogd could be true for *any* domain as long > > as those conditions are met, right? > > Yes, the two conditions are checked while starting a new domain in > qemuProcessPrepareDomain() and stored in
Re: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote: > > > On 06/15/2017 03:11 AM, Pavel Hrdina wrote: > > On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote: > >> > >> > >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote: > >>> In the case that virtlogd is used as stdio handler we pass to QEMU > >>> only FD to a PIPE connected to virtlogd instead of the file itself. > >>> > >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 > >>> > >>> Signed-off-by: Pavel Hrdina > >>> --- > >>> > >>> Notes: > >>> new in v2 > >>> > >>> src/lxc/lxc_process.c| 6 ++--- > >>> src/qemu/qemu_security.c | 9 +-- > >>> src/security/security_apparmor.c | 7 -- > >>> src/security/security_dac.c | 54 > >>> +++- > >>> src/security/security_driver.h | 6 +++-- > >>> src/security/security_manager.c | 12 ++--- > >>> src/security/security_manager.h | 6 +++-- > >>> src/security/security_nop.c | 6 +++-- > >>> src/security/security_selinux.c | 53 > >>> ++- > >>> src/security/security_stack.c| 12 ++--- > >>> tests/securityselinuxlabeltest.c | 2 +- > >>> 11 files changed, 127 insertions(+), 46 deletions(-) > >>> > >> > >> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why > >> is (!chr_seclabel) even matter? > > > > If you configure the label we shouldn't ignore it in some cases, that's > > just wrong. If the label is set for the char device we will configure > > it every time even if it will fail to start the guest, it's a > > responsibility of the user to configure proper label if it is provided. > > > > When email doesn't convey the question... Ugh... I'm also trying to > speed learn an area of the code and review at the same time. > > If I go back to commit id 'f8b08d0e' where the original implementation > to add labels for chardevs was done (but has been modified for patch 1 > to change where the label is stored), I get the impression that a label > should be added either from something specifically supplied for the > or to use the per domain one: > > "The source element may contain an optional seclabel to override the way > that labelling is done on the socket path. If this element is not > present, the security label is inherited from the per-domain setting." > > So if I look at the condition "(!chr_seclabel && chardevStdioLogd)" > added by this patch to decide whether or not to supply the label, I'm > left with the impression that if for this particular chardev a label > doesn't exist *and* the configuration option chardevStdioLogd is true, > then we're going to return happy status *and* not inherit the per-domain > setting. > > So the bug is then that applying the default domain label for a chardev > configured to use a stdio handle is incorrect? Perhaps I didn't get > that from reading bz or the patch. Yes, that's the bug. If virtlogd is used to handle stdio for char devices we shouldn't relabel the @path to the default labels, the @path doesn't have to be accessible by the qemu process, it has to be accessible by the virtlogd process. > >> IIUC, whether or not someone set the label for the chardev, for this > >> particular issue/config where the chardev has a , we > >> don't want to set (or restore) the label. I feel like I'm missing > >> something subtle. Maybe a bit more explanation of the adjustment would > >> help me... > > > > This is not for the but for the . > > We don't relabel $path for at all. > > > > hmm.. ah, right... I kept scrolling back and forth in the bz and the > docs, but missed this in the bz: > > 3) Check the virtlogd.log: > error : virRotatingFileWriterEntryNew:113 : Unable to open file: > /var/log/libvirt/qemu/log: Permission denied > > I guess I got lost in the power of suggestion of reading the docs > regarding the "optional log file" that can be associated paragraph and > trying to learn on the fly so that you at least get a review in a > somewhat timely manner ;-) > > >> Wouldn't these changes end up selecting "any" chardev if > >> chardevStdioLogd ended up being set regardless of whether they were > >> actually using the log file? > > > > I don't know what you mean by this sentence? > > > > Well let's see, chardevStdioLogd is set to true when meeting the two > conditions a qemu.conf global "cfg->stdioLogd" && a per domain or > emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND". > > So, conceivably chardevStdioLogd could be true for *any* domain as long > as those conditions are met, right? Yes, the two conditions are checked while starting a new domain in qemuProcessPrepareDomain() and stored in the private date of that domain. > If you have a domain that has chardev's which are not configured to use > the stdio handler, then the chardevStdioLogd could still be true, right? No, if the @chardevStdioLogd is true all char devices for that domain will use virtlogd. The only i
[libvirt] [PATCH] qemu: Remove inactive vm when failed to start it
Libvirt forgets to remove inactive vm when failed to start a defined vm. That may result in residual domain in driver->domains on such condition: during the process of starting a vm, undefine it, and qemu exit because of some exception. As we undefined the vm successfully, the vm->persistent was set to 0, we will always fail to undefine it until restart libvirtd. Signed-off-by: Yi Wang --- src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba1dba5..581f02f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7185,6 +7185,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) endjob: qemuProcessEndJob(driver, vm); +if (ret < 0) +qemuDomainRemoveInactive(driver, vm); cleanup: virDomainObjEndAPI(&vm); -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: add busy polling support
From: Sahid Orentino Ferdjaoui This patch finalizes support of 'poll_us' attribute as an option of the NIC driver-specific element, the commit also takes care of hotplug. That option is supported for all networks which are using a tap device. To be used the backend should be specificly set to use vhost. Signed-off-by: Sahid Orentino Ferdjaoui --- docs/formatdomain.html.in | 12 ++- docs/schemas/domaincommon.rng | 5 + src/qemu/qemu_command.c| 22 + src/qemu/qemu_hotplug.c| 12 +++ .../qemuxml2argv-net-virtio-netdev-pollus-fail.xml | 23 ++ ...xml2argv-net-virtio-netdev-pollus-qemu-fail.xml | 23 ++ .../qemuxml2argv-net-virtio-netdev-pollus.args | 23 ++ .../qemuxml2argv-net-virtio-netdev-pollus.xml | 23 ++ tests/qemuxml2argvtest.c | 9 + 9 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-fail.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-qemu-fail.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 52119f0..7bfeabc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5043,7 +5043,7 @@ qemu-kvm -net nic,model=? /dev/null@@ -5171,6 +5171,16 @@ qemu-kvm -net nic,model=? /dev/null In general you should leave this option alone, unless you are very certain you know what you are doing. + +The optional poll_us attribute configure the +maximum number of microseconds that could be spent on busy +polling. It improves the performance, but requires more +CPU. This option is only available with vhost backend driver. +Since 2.7.0 (QEMU and KVM only) + +In general you should leave this option alone, unless you +are very certain you know what you are doing. + virtio options For virtio interfaces, diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4950ddc..f304385 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2687,6 +2687,11 @@ + + + + + diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c12b2b..412496e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3955,6 +3955,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } if (net->tune.sndbuf_specified) virBufferAsprintf(&buf, "sndbuf=%lu,", net->tune.sndbuf); +if (net->driver.virtio.pollus) +virBufferAsprintf(&buf, "poll-us=%u,", + net->driver.virtio.pollus); } virObjectUnref(cfg); @@ -8451,6 +8454,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, return -1; } +if (net->driver.virtio.pollus > 0) { +if (net->driver.virtio.name != VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) { +virReportError( +VIR_ERR_CONFIG_UNSUPPORTED, +_("Busy polling is only supported with vhost backend driver")); +return -1; +} +/* Nothing besides TAP devices supports busy polling. */ +if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT || + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Busy polling is not supported for: %s"), + virDomainNetTypeToString(actualType)); +return -1; +} +} + /* and only TAP devices support nwfilter rules */ if (net->filter && !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0b8d3d8..33884b7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -995,6 +995,18 @@ qemuDomainAttachNetDevice(virQEMUDrive - +
[libvirt] [PATCH 1/3] qemu: add capability for vhost-net busy polling
From: Sahid Orentino Ferdjaoui
QEMU 2.7.0 adds support of busy polling on vhost-net.
69e87b32680a41d9761191443587c595b6f5fc3f
Signed-off-by: Sahid Orentino Ferdjaoui
---
src/qemu/qemu_capabilities.c | 6 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 +
7 files changed, 12 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c0c39bd..faf6d82 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -376,6 +376,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"intel-iommu.device-iotlb", /* 260 */
"virtio.iommu_platform",
"virtio.ats",
+ "vhost-net.poll_us",
);
@@ -4773,6 +4774,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
if (qemuCaps->version >= 2006000)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT);
+/* Support for busy polling on vhost-net devices ("-netdev
+ * tap,...,poll-us=n") */
+if (qemuCaps->version >= 2007000)
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_VHOST_NET_POLL_US);
+
if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index e57cae9..33f8ed0 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -415,6 +415,7 @@ typedef enum {
QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB, /* intel-iommu.device-iotlb */
QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM, /* virtio-*-pci.iommu_platform */
QEMU_CAPS_VIRTIO_PCI_ATS, /* virtio-*-pci.ats */
+QEMU_CAPS_VHOST_NET_POLL_US, /* vhost-net with -netdev poll-us= */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
index 70cce64..f833d4a 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
@@ -132,6 +132,7 @@
+
2007000
0
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
index 49c0462..2ab44cf 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
@@ -205,6 +205,7 @@
+
2007000
0
(v2.7.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
index 51be9bc..98f763d 100644
--- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
@@ -134,6 +134,7 @@
+
2007093
0
diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
index 01edbc8..bafe08f 100644
--- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
@@ -207,6 +207,7 @@
+
2008000
0
(v2.8.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
index 58dd9f6..dc93b5a 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
@@ -218,6 +218,7 @@
+
2009000
0
(v2.9.0)
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] conf: introduce 'poll_us' attribute for domain interface
From: Sahid Orentino Ferdjaoui
In this commit we introduce 'poll_us' attribute which will be passed
then to the QEMU command line to enable support of busy polling. This
commit also take into account that attribute to generate the XML.
Signed-off-by: Sahid Orentino Ferdjaoui
---
src/conf/domain_conf.c | 16
src/conf/domain_conf.h | 1 +
2 files changed, 17 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0507ec1..803f517 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9748,6 +9748,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
char *event_idx = NULL;
char *queues = NULL;
char *rx_queue_size = NULL;
+char *pollus = NULL;
char *str = NULL;
char *filter = NULL;
char *internal = NULL;
@@ -9921,6 +9922,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
event_idx = virXMLPropString(cur, "event_idx");
queues = virXMLPropString(cur, "queues");
rx_queue_size = virXMLPropString(cur, "rx_queue_size");
+pollus = virXMLPropString(cur, "poll_us");
} else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) {
if (filter) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -10318,6 +10320,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->driver.virtio.rx_queue_size = q;
}
+if (pollus) {
+unsigned int us;
+if (virStrToLong_uip(pollus, NULL, 10, &us) < 0) {
+virReportError(VIR_ERR_XML_DETAIL,
+ _("'poll_us' attribute must be positive number:
%s"),
+ pollus);
+goto error;
+}
+if (us > 1)
+def->driver.virtio.pollus = us;
+}
if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) {
if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -10515,6 +10528,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(event_idx);
VIR_FREE(queues);
VIR_FREE(rx_queue_size);
+VIR_FREE(pollus);
VIR_FREE(str);
VIR_FREE(filter);
VIR_FREE(type);
@@ -22308,6 +22322,8 @@ virDomainVirtioNetDriverFormat(char **outstr,
if (def->driver.virtio.rx_queue_size)
virBufferAsprintf(&buf, " rx_queue_size='%u'",
def->driver.virtio.rx_queue_size);
+if (def->driver.virtio.pollus)
+virBufferAsprintf(&buf, " poll_us='%u'", def->driver.virtio.pollus);
virDomainVirtioOptionsFormat(&buf, def->virtio);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e67b6fd..d97d776 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -967,6 +967,7 @@ struct _virDomainNetDef {
virTristateSwitch event_idx;
unsigned int queues; /* Multiqueue virtio-net */
unsigned int rx_queue_size;
+unsigned int pollus; /* busy polling for tap */
struct {
virTristateSwitch csum;
virTristateSwitch gso;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] exposing busy polling support for vhost-net
From: Sahid Orentino Ferdjaoui In version 2.7.0, QEMU introduced support of busy polling for vhost-net [0]. To avoid paraphrasing original authors of that feature and the purpose it I prefer to share a pointer [1]. This patch serie exposes throught the NIC driver-specific element a new option 'poll_us'. That option is only available with the backend driver 'vhost'. The option 'poll_us' takes a positive number. 0 means that the option is not going to be exposed. [0] 69e87b32680a41d9761191443587c595b6f5fc3f [1] https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg04595.html Sahid Orentino Ferdjaoui (3): qemu: add capability for vhost-net busy polling conf: introduce 'poll_us' attribute for domain interface qemu: add busy polling support docs/formatdomain.html.in | 12 ++- docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c | 16 +++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 6 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c| 22 + src/qemu/qemu_hotplug.c| 12 +++ tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + .../qemuxml2argv-net-virtio-netdev-pollus-fail.xml | 23 ++ ...xml2argv-net-virtio-netdev-pollus-qemu-fail.xml | 23 ++ .../qemuxml2argv-net-virtio-netdev-pollus.args | 23 ++ .../qemuxml2argv-net-virtio-netdev-pollus.xml | 23 ++ tests/qemuxml2argvtest.c | 9 + 18 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-fail.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-qemu-fail.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.xml -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
On 06/15/2017 09:44 AM, Michal Privoznik wrote: > On 06/15/2017 02:03 PM, John Ferlan wrote: >> >> >> On 06/15/2017 04:53 AM, Michal Privoznik wrote: >>> On 06/14/2017 09:50 PM, John Ferlan wrote: On 06/12/2017 11:57 AM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1431112 > > After 290a00e41d we know how to deal with file mount points. > However, when cleaning up the temporary location for preserved > mount points we are still calling rmdir(). This won't fly for > files. We need to call unlink(). Now, since we don't really care > if the cleanup succeeded or not (it's the best effort anyway), we > can call both rmdir() and unlink() without need for > differentiation between files and directories. > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_domain.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > But why call both? >>> >>> I don't think you can call unlink() over a directory, can you? And sure, >>> I could call stat() just to find out if it's a dir or a file and call >>> just one of the pair. Or I can call both and ignore any errors. The >>> result is the same, isn't it? >>> >>> Michal >>> >> >> From the unlink(3p) man page: >> >> "The path argument shall not name a directory unless the process has >> appropriate privileges and the implementation supports using unlink() on >> directories." > > That's weird. I don't see that in my man page. What I see though is the > following errno code: > > EISDIR pathname refers to a directory. (This is the non-POSIX value > returned by Linux since 2.1.132.) > > The "non-POSIX" bothers me there. But I agree that whole namespaces are > Linux specific, so it doesn't bother me there that much. > I got that from my Fedora install... But I know I've been down this path before using other U*x OS's - those details have long since left my short term memory. Like I said, eblake would be of help here, but he's away for another week or so... >> >> Then a google search on using unlink vs. rmdir uncovers more refs. I >> suppose one could also do the "if file, then unlink else rmdir. > > Well, to determine if a path is a file or a dir I'd need to call stat() > which can fail. I'm not a big fan of overcomplicating simple code. > >> >> Just seems "odd" to see both and leaves one wondering why. > > Well, there's a comment that says why. > > Michal > I recall going down this path before when I updated virFileRemove when dealing with NFS and the craziness that is a root-squash environment. I would think the following would work just fine in this case (as cut-n-paste'd from virFileRemove): if (virFileIsDir(path)) return rmdir(path); else return unlink(path); You're ignoring errors anyway so whether stat() fails [in virFileIsDir] or whether unlink() fails because it's being run on a directory in the event that virFileIsDir() fails won't matter. But at least you wouldn't always cause some failure. The way you've changed things you'll always get an error that's ignored. If rmdir() succeeded, then unlink() would fail (on directory). If rmdir() fails (on file), then unlink() could/should/would succeed. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
On 06/15/2017 02:03 PM, John Ferlan wrote: > > > On 06/15/2017 04:53 AM, Michal Privoznik wrote: >> On 06/14/2017 09:50 PM, John Ferlan wrote: >>> >>> >>> On 06/12/2017 11:57 AM, Michal Privoznik wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1431112 After 290a00e41d we know how to deal with file mount points. However, when cleaning up the temporary location for preserved mount points we are still calling rmdir(). This won't fly for files. We need to call unlink(). Now, since we don't really care if the cleanup succeeded or not (it's the best effort anyway), we can call both rmdir() and unlink() without need for differentiation between files and directories. Signed-off-by: Michal Privoznik --- src/qemu/qemu_domain.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> But why call both? >> >> I don't think you can call unlink() over a directory, can you? And sure, >> I could call stat() just to find out if it's a dir or a file and call >> just one of the pair. Or I can call both and ignore any errors. The >> result is the same, isn't it? >> >> Michal >> > > From the unlink(3p) man page: > > "The path argument shall not name a directory unless the process has > appropriate privileges and the implementation supports using unlink() on > directories." That's weird. I don't see that in my man page. What I see though is the following errno code: EISDIR pathname refers to a directory. (This is the non-POSIX value returned by Linux since 2.1.132.) The "non-POSIX" bothers me there. But I agree that whole namespaces are Linux specific, so it doesn't bother me there that much. > > Then a google search on using unlink vs. rmdir uncovers more refs. I > suppose one could also do the "if file, then unlink else rmdir. Well, to determine if a path is a file or a dir I'd need to call stat() which can fail. I'm not a big fan of overcomplicating simple code. > > Just seems "odd" to see both and leaves one wondering why. Well, there's a comment that says why. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Temporarily disable format truncation warnings
GCC 7.1 introduces a new -Wformat-truncation warning flag that reports if it thinks the maximum possible size of the formatted output will exceed the provided fixed buffer. This is enabled automatically by the -Wformat warning flag. There are quite a few places hit by this in libvirt which need rewriting. This is non-trivial work in some places, so temporarily disable the new warning until those fixes can be implemented. Signed-off-by: Daniel P. Berrange --- m4/virt-compile-warnings.m4 | 4 1 file changed, 4 insertions(+) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d7bb172f3..fa0940fc6 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -169,6 +169,10 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ wantwarn="$wantwarn -Wno-format" fi +# -Wformat enables this by default, and we should keep it, +# but need to rewrite various areas of code first +wantwarn="$wantwarn -Wno-format-truncation" + # This should be < 256 really. Currently we're down to 4096, # but using 1024 bytes sized buffers (mostly for virStrerror) # stops us from going down further -- 2.13.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
On Thu, Jun 15, 2017 at 06:50:34AM -0400, John Ferlan wrote:
>
>
> On 06/15/2017 02:39 AM, Pavel Hrdina wrote:
> > On Tue, Jun 13, 2017 at 12:35:10PM -0400, John Ferlan wrote:
> >>
> >>
> >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
> >>> Signed-off-by: Pavel Hrdina
> >>> ---
> >>>
> >>> Notes:
> >>> new in v2
> >>>
> >>> src/conf/domain_conf.c | 46
> >>> +++--
> >>> src/conf/domain_conf.h | 9
> >>> src/security/security_dac.c | 26 ++-
> >>> src/security/security_manager.c | 4 ++--
> >>> src/security/security_selinux.c | 24 +
> >>> 5 files changed, 49 insertions(+), 60 deletions(-)
> >>>
> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >>> index c7e20b8ba1..68dc2832cb 100644
> >>> --- a/src/conf/domain_conf.c
> >>> +++ b/src/conf/domain_conf.c
> >>> @@ -2076,12 +2076,21 @@
> >>> virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
> >>>
> >>> void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
> >>> {
> >>> +size_t i;
> >>> +
> >>> if (!def)
> >>> return;
> >>>
> >>> virDomainChrSourceDefClear(def);
> >>> virObjectUnref(def->privateData);
> >>>
> >>> +if (def->seclabels) {
> >>> +for (i = 0; i < def->nseclabels; i++)
> >>> +virSecurityDeviceLabelDefFree(def->seclabels[i]);
> >>> +VIR_FREE(def->seclabels);
> >>> +}
> >>> +
> >>> +
> >>> VIR_FREE(def);
> >>> }
> >>>
> >>> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const
> >>> virDomainChrSourceDef *src,
> >>>
> >>> void virDomainChrDefFree(virDomainChrDefPtr def)
> >>> {
> >>> -size_t i;
> >>> -
> >>> if (!def)
> >>> return;
> >>>
> >>> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
> >>> virDomainChrSourceDefFree(def->source);
> >>> virDomainDeviceInfoClear(&def->info);
> >>>
> >>> -if (def->seclabels) {
> >>> -for (i = 0; i < def->nseclabels; i++)
> >>> -virSecurityDeviceLabelDefFree(def->seclabels[i]);
> >>> -VIR_FREE(def->seclabels);
> >>> -}
> >>> -
> >>> VIR_FREE(def);
> >>> }
> >>>
> >>> @@ -10688,8 +10689,8 @@
> >>> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >>> if (chr_def) {
> >>
> >> Is the @chr_def check necessary still? I assume it's there because
> >> chr_def can be passed as NULL in some cases.
> >>
> >> Looks like all this was added as part of commit 'f8b08d0e'
> >>
> >> The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
> >> each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
> >> leads me to believe that the @chr_def should be removed.
> >
> > But this hunk is from virDomainChrSourceDefParseXML() function.
> >
>
> Yes, I knew that when I wrote the comment, but that wasn't the point.
>
> Since you've moved the labels into @def instead and similarly altered
> calls to virDomainChrSourceDefFormat such that they don't receive
> chr_def (in the very next hunk of changes BTW), then I would think at
> this point removing chr_def would be safe, but I suppose I could be
> wrong. Hence why I asked.
>
> So does it hurt to keep it, probably not; however, IIUC the reason why
> it was there was because if it wasn't, then dereferencing chr_def to get
> the [n]seclabels in the subsequent call wouldn't end well.
Oh right. The only thing what that check currently does is that for
smartcard, rng and redirdev it skips parsing of the seclabel. I would
probably leave it to a separate patch which would ensure that we don't
start parsing the seclabel for these devices.
Pavel
>
> John
>
> >> The rest looks good, so
> >>
> >> Reviewed-by: John Ferlan
> >
> > Thanks
> >
> > Pavel
> >
>
> --
> 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
[libvirt] [PATCH] qemu: Allow live-updates of coalesce settings
Change the settings from qemuDomainUpdateDeviceLive() as otherwise the
call would succeed even though nothing has changed.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1460862
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_hotplug.c | 13 ++
src/util/virnetdev.c| 103 +++-
src/util/virnetdev.h| 3 +-
src/util/virnetdevtap.c | 2 +-
4 files changed, 83 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0b8d3d80f173..244dd5e605e6 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2986,6 +2986,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
bool needLinkStateChange = false;
bool needReplaceDevDef = false;
bool needBandwidthSet = false;
+bool needCoalesceChange = false;
int ret = -1;
int changeidx = -1;
@@ -3280,6 +3281,12 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
virDomainNetGetActualBandwidth(newdev)))
needBandwidthSet = true;
+if (!!olddev->coalesce != !!newdev->coalesce ||
+(olddev->coalesce && newdev->coalesce &&
+ !memcmp(olddev->coalesce, newdev->coalesce,
+ sizeof(*olddev->coalesce
+needCoalesceChange = true;
+
/* FINALLY - actually perform the required actions */
if (needReconnect) {
@@ -3315,6 +3322,12 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
needReplaceDevDef = true;
}
+if (needCoalesceChange) {
+if (virNetDevSetCoalesce(newdev->ifname, newdev->coalesce, true) < 0)
+goto cleanup;
+needReplaceDevDef = true;
+}
+
if (needLinkStateChange &&
qemuDomainChangeNetLinkState(driver, vm, olddev, newdev->linkstate) <
0) {
goto cleanup;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index e75a1c970505..90b7bee34ae7 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3086,7 +3086,8 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap
ATTRIBUTE_UNUSED,
/**
* virNetDevSetCoalesce:
* @ifname: interface name to modify
- * @coalesce: Coalesce settings to set and update
+ * @coalesce: Coalesce settings to set or update
+ * @update: Whether this is an update for existing settings or not
*
* This function sets the various coalesce settings for a given interface
* @ifname and updates them back into @coalesce.
@@ -3094,40 +3095,44 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap
ATTRIBUTE_UNUSED,
* Returns 0 in case of success or -1 on failure
*/
int virNetDevSetCoalesce(const char *ifname,
- virNetDevCoalescePtr coalesce)
+ virNetDevCoalescePtr coalesce,
+ bool update)
{
int fd = -1;
int ret = -1;
struct ifreq ifr;
struct ethtool_coalesce coal = {0};
-if (!coalesce)
+if (!coalesce && !update)
return 0;
-coal = (struct ethtool_coalesce) {
-.cmd = ETHTOOL_SCOALESCE,
-.rx_max_coalesced_frames = coalesce->rx_max_coalesced_frames,
-.rx_coalesce_usecs_irq = coalesce->rx_coalesce_usecs_irq,
-.rx_max_coalesced_frames_irq = coalesce->rx_max_coalesced_frames_irq,
-.tx_coalesce_usecs = coalesce->tx_coalesce_usecs,
-.tx_max_coalesced_frames = coalesce->tx_max_coalesced_frames,
-.tx_coalesce_usecs_irq = coalesce->tx_coalesce_usecs_irq,
-.tx_max_coalesced_frames_irq = coalesce->tx_max_coalesced_frames_irq,
-.stats_block_coalesce_usecs = coalesce->stats_block_coalesce_usecs,
-.use_adaptive_rx_coalesce = coalesce->use_adaptive_rx_coalesce,
-.use_adaptive_tx_coalesce = coalesce->use_adaptive_tx_coalesce,
-.pkt_rate_low = coalesce->pkt_rate_low,
-.rx_coalesce_usecs_low = coalesce->rx_coalesce_usecs_low,
-.rx_max_coalesced_frames_low = coalesce->rx_max_coalesced_frames_low,
-.tx_coalesce_usecs_low = coalesce->tx_coalesce_usecs_low,
-.tx_max_coalesced_frames_low = coalesce->tx_max_coalesced_frames_low,
-.pkt_rate_high = coalesce->pkt_rate_high,
-.rx_coalesce_usecs_high = coalesce->rx_coalesce_usecs_high,
-.rx_max_coalesced_frames_high = coalesce->rx_max_coalesced_frames_high,
-.tx_coalesce_usecs_high = coalesce->tx_coalesce_usecs_high,
-.tx_max_coalesced_frames_high = coalesce->tx_max_coalesced_frames_high,
-.rate_sample_interval = coalesce->rate_sample_interval,
-};
+if (coalesce) {
+coal = (struct ethtool_coalesce) {
+.rx_max_coalesced_frames = coalesce->rx_max_coalesced_frames,
+.rx_coalesce_usecs_irq = coalesce->rx_coalesce_usecs_irq,
+.rx_max_coalesced_frames_irq =
coalesce->rx_max_coalesced_frames_irq,
+.tx_coalesce_usecs = coalesce->tx_coalesce_usecs,
+.tx_max_coalesced_frames = coalesce->tx_max_coalesced_frames,
+.tx_coalesce_usecs_irq = coalesce->tx_co
Re: [libvirt] [PATCH] hacking: Improve 'git send-email' documentation
On 06/14/2017 11:31 PM, Andrea Bolognani wrote: > For the benefit of first time contributors, we point out that 'git > send-email' might have to be installed separately; however, we omit > the fact that some configuration will likely be needed before it > can successfully deliver patches to the mailing list. > > Some minor tweaks to the existing contents are included as well. > > Signed-off-by: Andrea Bolognani > --- > docs/hacking.html.in | 36 > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/docs/hacking.html.in b/docs/hacking.html.in > index d6a574c..434fb68 100644 > --- a/docs/hacking.html.in > +++ b/docs/hacking.html.in > @@ -29,8 +29,8 @@ > file from zanata. > > > - Post patches using "git send-email", with git rename > -detection enabled. You need a one-time setup of: > + Post patches using git send-email, with git > +rename detection enabled. You need a one-time setup of: > >git config diff.renames true > > @@ -52,20 +52,32 @@ >git send-email --cover-letter --no-chain-reply-to --annotate \ > --to=libvir-list@redhat.com master > FWIW: I have in my cut-n-paste command "--confirm=always" - it just makes sure I really want to send things, but has saved me a couple of times! > -(Note that the "git send-email" subcommand may not be in > -the main git package and using it may require installation of a > -separate package, for example the "git-email" package in > -Fedora.) For a single patch you can omit > ---cover-letter, but a series of two or more > -patches needs a cover letter. If you get tired of typing > ---to=libvir-list@redhat.com designation you can > -set it in git config: > +Note that the git send-email subcommand may not > +be in the main git package and using it may require installation > +of a separate package, for example the "git-email" package in > +Fedora and Debian. If this is your first time using > +git send-email, you might need to configure it to > +point it to your SMTP server with something like: > + > + git config --global sendemail.smtpServer stmp.youremailprovider.net > + > +If you get tired of typing > +--to=libvir-list@redhat.com all the time, you can > +configure that to be automatically handled as well: > >git config sendemail.to libvir-list@redhat.com > As long as we're adding stuff... For those that use libvir-list to post patches from multiple repositories (e.g. libvirt-python, libvirt-perl, etc.) using: git config --local format.subjectprefix "libvirt-python PATCH" (or libvirt-perl or whatever the repository is) really *helps* reviewers focus whether the patch is meant for mainline libvirt or whether it's for some other git repo. Also (how strongly can we say the following)... Please, please, please, pretty-please... Do *NOT* use the "--cc" option to copy random developers. We're all subscribed to the list and will *definitely* see the patches. I would even say I'd rather not see it for -vN patches too. > +For a single patch you can omit > +--cover-letter, but a series of two or more > +patches needs a cover letter. > +If everything went well, your patch should show up on the > +https://www.redhat.com/archives/libvir-list/";>libvir-list > +archives in a matter of minutes; if you still can't find it on > +there after an hour or so, you should double-check your setup. Another "hint" I got when first starting out - send your patch to your own email address instead of libvir-list in order to make sure it "looks right" or "similar to" how patches are posted on the list. One could also go as far as "saving" those patches and creating a new git branch to test whether a "git am -3" would work on their patch(es). That at least ensures someone else applying an on list patch would be able to "use" whatever was posted. Thanks for diving into the shallow end of pool and updating this ;-) John > Please follow this as close as you can, especially the rebase and > -git send-email part, as it makes life easier for other developers to > -review your patch set. One should avoid sending patches as > attachments, > +git send-email part, as it makes life easier for other > +developers to review your patch set. > +One should avoid sending patches as attachments, > but rather send them in email body along with commit message. If a > developer is sending another version of the patch (e.g. to address > review comments), they are advised to note differences to previous > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: fixing internationalization problem with sched kernel entry.
On 06/14/2017 12:25 PM, Julio Faracco wrote:
> In qemuGetSchedInfo(), libvirt is trying to parse some field of /proc/*/sched.
> Some fields are floating point numbers and their separator is always a dot.
> When you change the locale of the system, you can change the mantissa
> separator (to a comma separator) of floating point numbers and it affects the
> parser.
>
> This commit introduces the virStrToDoubleSafe() to avoid comma as a separator.
>
> 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/qemu/qemu_driver.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
FWIW: Since you'll have to post a v2 anyway due to 1/2 NACK, may I
suggest checking out the very recent on-list discussion about sending
patches using --cover-letter for multiple patch series from the hacking
page:
https://www.redhat.com/archives/libvir-list/2017-June/msg00638.html
and the hacking guidelines:
http://libvirt.org/hacking.html
Keeping "related" patches threaded is helpful for reviewers and doesn't
leave a stray on the list...
Tks -
John
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ba1dba5..f1b104f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1369,7 +1369,7 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
> while (*line == ' ')
> line++;
>
> -if (virStrToDouble(line, NULL, &val) < 0) {
> +if (virStrToDoubleSafe(line, NULL, &val) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Unable to parse sched info value '%s'"),
> line);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 1/4] conf : Add loadparm boot option for a boot device
On 06/14/2017 01:12 PM, John Ferlan wrote:
On 06/01/2017 12:36 PM, Farhan Ali wrote:
Update the per device boot schema to add an optional loadparm parameter.
eg:
Extend the virDomainDeviceInfo to support loadparm option.
Modify the appropriate functions to parse loadparm from boot device xml.
Signed-off-by: Farhan Ali
Reviewed-by: Bjoern Walk
Reviewed-by: Boris Fiuczynski
Reviewed-by: Marc Hartmayer
---
docs/formatdomain.html.in | 9 --
docs/schemas/domaincommon.rng | 7 +
src/conf/device_conf.h| 1 +
src/conf/domain_conf.c| 69 +--
4 files changed, 82 insertions(+), 4 deletions(-)
I believe it was stated previously that the xml2xml tests should be in
this patch. So I moved them. The xml2xml test doesn't require all the
flags that were set and I moved it to be with the other "machine-*" tests.
Reviewed-by: John Ferlan
I will push this (and the subsequent patches) later this week just to be
sure no one chimes in after my review to note something I missed (or in
case some of the wording I modified below needs more adjustment).
John
Hi John,
The changes look good to me. Thanks for reviewing it.
Thanks
Farhan
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 07208ee..837be18 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3022,8 +3022,13 @@
boot
Specifies that the disk is bootable. The order
attribute determines the order in which devices will be tried during
-boot sequence. The per-device boot elements cannot be
-used together with general boot elements in
+boot sequence. On S390 architecture only the first boot device is used.
s/On/On the/
+The optional loadparm attribute is an 8 byte parameter
s/byte parameter/character string/
+which can be queried by guests on S390 via sclp or diag 308. Linux
+guests on S390 can use loadparm to select a boot entry.
+Since 3.5.0
+The per-device boot elements cannot be used together
+with general boot elements in
BIOS bootloader section.
Since 0.8.8
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4d9f8d1..ef09860 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5031,6 +5031,13 @@
+
+
+
+[a-zA-Z0-9.\s]{1,8}
+
+
+
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a20de85..48782be 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -167,6 +167,7 @@ struct _virDomainDeviceInfo {
* assignment, never saved and never reported.
*/
int pciConnectFlags; /* enum virDomainPCIConnectFlags */
+char *loadparm;
};
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c7e20b8..cd35ad2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3520,6 +3520,7 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
memset(&info->addr, 0, sizeof(info->addr));
info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
VIR_FREE(info->romfile);
+VIR_FREE(info->loadparm);
}
@@ -5213,6 +5214,48 @@ virDomainDefValidate(virDomainDefPtr def,
return 0;
}
Now prefer two blank lines between functions.
+/**
+ * virDomainDeviceLoadparmIsValid
+ * @loadparm : The string to validate
+ *
+ * The valid set of values for loadparm are [a-zA-Z0-9.]
+ * and blank spaces.
+ * The maximum allowed length is 8 characters.
+ * An empty string is considered invalid
+ */
+static bool
+virDomainDeviceLoadparmIsValid(const char *loadparm)
+{
+size_t i;
+
+if (virStringIsEmpty(loadparm)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("loadparm cannot be an empty string"));
+return false;
+}
+
+if (strlen(loadparm) > 8) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("loadparm '%s' exceeds 8 characters"), loadparm);
+return false;
+}
+
+for (i = 0; i < strlen(loadparm); i++) {
+uint8_t c = loadparm[i];
+
+if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') ||
+(c == '.') || (c == ' ')) {
+continue;
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid loadparm char '%c', expecting chars"
+ " in set of [a-zA-Z0-9.] and blank spaces"), c);
+return false;
+}
+}
+
+return true;
+}
/* Generate a string representation of a device address
* @info address Device address to stringify
@@ -5222,9 +5265,14 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virDomainDeviceInfoPtr info,
unsigned int flags)
{
-if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex)
-
Re: [libvirt] [PATCH] Disable the -Wduplicated-branches warning
On Wed, Jun 14, 2017 at 03:11:35PM +0100, Daniel P. Berrange wrote: Depending on the platform/architecture, a number of conditionals in libvirt code expand the same on both branches. This is expected behaviour and harmless, so disable the warning to avoid creating unexpected build failures Two examples, mingw32: ../../src/util/vircommand.c: In function 'virCommandWait': ../../src/util/vircommand.c:2562:51: error: this condition has identical branches [-Werror=duplicated-branches] *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); ^ and gcc7.1 In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~ Signed-off-by: Daniel P. Berrange --- m4/virt-compile-warnings.m4 | 3 +++ 1 file changed, 3 insertions(+) ACK diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 768a5c8..d7bb172 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -61,6 +61,9 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ dontwarn="$dontwarn -Wenum-compare" # gcc 5.1 -Wformat-signedness mishandles enums, not ready for prime time dontwarn="$dontwarn -Wformat-signedness" +# Several conditionals expand the same on both branches +# depending on the particular platform/architecture +dontwarn="$dontwarn -Wduplicated-branches" # gcc 4.2 treats attribute(format) as an implicit attribute(nonnull), # which triggers spurious warnings for our usage -- 2.9.3 -- 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] [PATCH 3/3] qemuDomainGetPreservedMounts: Fix suffixes for corner cases
On 06/15/2017 04:53 AM, Michal Privoznik wrote: > On 06/14/2017 09:58 PM, John Ferlan wrote: >> >> >> On 06/12/2017 11:57 AM, Michal Privoznik wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112 >>> >>> Imagine a FS mounted on /dev/blah/blah2. Our process of creating >>> suffix for temporary location where all the mounted filesystems >>> are moved is very simplistic. We want: >>> >>> /var/run/libvirt/qemu/$domName.$suffix\ >>> >>> were $suffix is just the mount point path stripped of the "/dev/" >>> preffix. For instance: >> >> s/preffix/prefix >> >>> >>> /var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue >>> /var/run/libvirt/qemu/fedora.pts for /dev/pts >>> >>> and so on. Now if we plug /dev/blah/blah2 into the example we see >>> some misbehaviour: >>> >>> /var/run/libvirt/qemu/fedora.blah/blah2 >>> >>> Well, misbehaviour if /dev/blah/blah2 is a file, because in that >>> case we call virFileTouch() instead of virFileMakePath(). >>> >> >> You didn't finish my bedtime story! >> >> Am I to assume that instead of : >> >> /var/run/libvirt/qemu/fedora.blah/blah2 >> >> we would get >> >> /var/run/libvirt/qemu/fedora.blah.blah2 > > Yes. > >> >> taking things one step further... >> >> would /dev/blah/blah2/blah3 >> >> be >> >> /var/run/libvirt/qemu/fedora.blah.blah2.blah3 > > > Yes. > >> >> That's what I see coded at least... Or should the path be: >> >> /var/run/libvirt/qemu/fedora.blah/blah2.blah3 > > > Nope. The former one. > >> >> >> It would seem you'd want to get to the end, reverse search on '/' then >> if that spot is greater than @off, then convert it to a '.', > > So basically, this is my approach just reversed. What'd be the benefits? > I find my algorithm small and easy to understand. > >> but what do >> I know. I keep to the simple life and don't use namespaces. > > Well, until a7cc039dc I didn't know that you can bind mount files. What > a strange thing to learn. What I want to say - you can learn some new > stuff when using namespaces ;-) > > Michal > OK fair enough - be sure to finish the bed time story that this patch converts @suffix directory 'layers' into the flat namespace using '.' instead of '/'. The whole comment for 'mounts[i] is ...' could be simplified to indicate that we're turning the complete @suffix from a possible multi-directory level into a single flat file reference. Reviewed-by: John Ferlan Just so you know IDC if you keep the for (...) in patch 2, I'm just not a fan. You're the author, you got my OK for your method even though it's not my personal preference. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
On 06/15/2017 04:53 AM, Michal Privoznik wrote: > On 06/14/2017 09:50 PM, John Ferlan wrote: >> >> >> On 06/12/2017 11:57 AM, Michal Privoznik wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112 >>> >>> After 290a00e41d we know how to deal with file mount points. >>> However, when cleaning up the temporary location for preserved >>> mount points we are still calling rmdir(). This won't fly for >>> files. We need to call unlink(). Now, since we don't really care >>> if the cleanup succeeded or not (it's the best effort anyway), we >>> can call both rmdir() and unlink() without need for >>> differentiation between files and directories. >>> >>> Signed-off-by: Michal Privoznik >>> --- >>> src/qemu/qemu_domain.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >> >> But why call both? > > I don't think you can call unlink() over a directory, can you? And sure, > I could call stat() just to find out if it's a dir or a file and call > just one of the pair. Or I can call both and ignore any errors. The > result is the same, isn't it? > > Michal > >From the unlink(3p) man page: "The path argument shall not name a directory unless the process has appropriate privileges and the implementation supports using unlink() on directories." Then a google search on using unlink vs. rmdir uncovers more refs. I suppose one could also do the "if file, then unlink else rmdir. Just seems "odd" to see both and leaves one wondering why. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On 06/15/2017 03:11 AM, Pavel Hrdina wrote: > On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote: >> >> >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote: >>> In the case that virtlogd is used as stdio handler we pass to QEMU >>> only FD to a PIPE connected to virtlogd instead of the file itself. >>> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 >>> >>> Signed-off-by: Pavel Hrdina >>> --- >>> >>> Notes: >>> new in v2 >>> >>> src/lxc/lxc_process.c| 6 ++--- >>> src/qemu/qemu_security.c | 9 +-- >>> src/security/security_apparmor.c | 7 -- >>> src/security/security_dac.c | 54 >>> +++- >>> src/security/security_driver.h | 6 +++-- >>> src/security/security_manager.c | 12 ++--- >>> src/security/security_manager.h | 6 +++-- >>> src/security/security_nop.c | 6 +++-- >>> src/security/security_selinux.c | 53 >>> ++- >>> src/security/security_stack.c| 12 ++--- >>> tests/securityselinuxlabeltest.c | 2 +- >>> 11 files changed, 127 insertions(+), 46 deletions(-) >>> >> >> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why >> is (!chr_seclabel) even matter? > > If you configure the label we shouldn't ignore it in some cases, that's > just wrong. If the label is set for the char device we will configure > it every time even if it will fail to start the guest, it's a > responsibility of the user to configure proper label if it is provided. > When email doesn't convey the question... Ugh... I'm also trying to speed learn an area of the code and review at the same time. If I go back to commit id 'f8b08d0e' where the original implementation to add labels for chardevs was done (but has been modified for patch 1 to change where the label is stored), I get the impression that a label should be added either from something specifically supplied for the or to use the per domain one: "The source element may contain an optional seclabel to override the way that labelling is done on the socket path. If this element is not present, the security label is inherited from the per-domain setting." So if I look at the condition "(!chr_seclabel && chardevStdioLogd)" added by this patch to decide whether or not to supply the label, I'm left with the impression that if for this particular chardev a label doesn't exist *and* the configuration option chardevStdioLogd is true, then we're going to return happy status *and* not inherit the per-domain setting. So the bug is then that applying the default domain label for a chardev configured to use a stdio handle is incorrect? Perhaps I didn't get that from reading bz or the patch. >> IIUC, whether or not someone set the label for the chardev, for this >> particular issue/config where the chardev has a , we >> don't want to set (or restore) the label. I feel like I'm missing >> something subtle. Maybe a bit more explanation of the adjustment would >> help me... > > This is not for the but for the . > We don't relabel $path for at all. > hmm.. ah, right... I kept scrolling back and forth in the bz and the docs, but missed this in the bz: 3) Check the virtlogd.log: error : virRotatingFileWriterEntryNew:113 : Unable to open file: /var/log/libvirt/qemu/log: Permission denied I guess I got lost in the power of suggestion of reading the docs regarding the "optional log file" that can be associated paragraph and trying to learn on the fly so that you at least get a review in a somewhat timely manner ;-) >> Wouldn't these changes end up selecting "any" chardev if >> chardevStdioLogd ended up being set regardless of whether they were >> actually using the log file? > > I don't know what you mean by this sentence? > Well let's see, chardevStdioLogd is set to true when meeting the two conditions a qemu.conf global "cfg->stdioLogd" && a per domain or emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND". So, conceivably chardevStdioLogd could be true for *any* domain as long as those conditions are met, right? If you have a domain that has chardev's which are not configured to use the stdio handler, then the chardevStdioLogd could still be true, right? If that's the case and the chardev doesn't have a label associated, then we just return happy status and we do not inherit the per domain setting. Wouldn't that be incorrect? My concern is more we're making a change in a (mostly) common set of functions for a (very) specific problem. >> As an aside, I think there's an "oddity" when it comes to the Restore, >> but I'm not sure how to put it into words exactly. If a guest is running >> code prior to this set of changes, would it have successfully run a Set? >> If so, then after applying this change and restarting, the label >> wouldn't be reset, right? What happens at guest shutdown - does the >> label not get unset now? Of course this is all "interaction" with >> virtlogd restart that
[libvirt] [PATCH v2] qemu: Pass the number of heads even with -vga qxl
When added in multiple previous commits, it was used only with -device
qxl(-vga), but for some QEMUs (< 1.6) we need to add this
functionality when using -vga qxl as well.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283207
Signed-off-by: Martin Kletzander
---
v2:
- Do not change the domain definition
- Adjust the code so that it looks the same as other chunks before above
v1:
- https://www.redhat.com/archives/libvir-list/2017-June/msg00606.html
src/qemu/qemu_command.c| 7
.../qemuxml2argv-video-vga-qxl-heads.args | 30 ++
.../qemuxml2argv-video-vga-qxl-heads.xml | 47 ++
tests/qemuxml2argvtest.c | 3 ++
4 files changed, 87 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8c12b2be086a..88ce3787d968 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4590,6 +4590,7 @@ qemuBuildVgaVideoCommand(virCommandPtr cmd,
unsigned int vram = video->vram;
unsigned int vram64 = video->vram64;
unsigned int vgamem = video->vgamem;
+unsigned int heads = video->heads;
if (ram) {
virCommandAddArg(cmd, "-global");
@@ -4613,6 +4614,12 @@ qemuBuildVgaVideoCommand(virCommandPtr cmd,
virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
dev, vgamem / 1024);
}
+if (heads &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_MAX_OUTPUTS)) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "%s.max_outputs=%u",
+ dev, heads);
+}
}
if (video->vram &&
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.args
b/tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.args
new file mode 100644
index ..411a2eedbc4b
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.args
@@ -0,0 +1,30 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-vga qxl \
+-global qxl-vga.ram_size=67108864 \
+-global qxl-vga.vram_size=67108864 \
+-global qxl-vga.max_outputs=1 \
+-device qxl,id=video1,ram_size=67108864,vram_size=33554432,max_outputs=3,\
+bus=pci.0,addr=0x4 \
+-device qxl,id=video2,ram_size=67108864,vram_size=67108864,max_outputs=7,\
+bus=pci.0,addr=0x5 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.xml
b/tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.xml
new file mode 100644
index ..d878ddcd6d2e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.xml
@@ -0,0 +1,47 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219136
+ 219136
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-i686
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 799aea9faf54..34edc546b068 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1735,6 +1735,9 @@ mymain(void)
QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
QEMU_CAPS_DEVICE_QXL,
QEMU_CAPS_QXL_MAX_OUTPUTS);
+DO_TEST("video-vga-qxl-heads",
+QEMU_CAPS_DEVICE_QXL,
+QEMU_CAPS_QXL_MAX_OUTPUTS);
DO_TEST("video-qxl-noheads",
QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
QEMU_CAPS_DEVICE_QXL,
--
2.13.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
On 06/15/2017 02:39 AM, Pavel Hrdina wrote:
> On Tue, Jun 13, 2017 at 12:35:10PM -0400, John Ferlan wrote:
>>
>>
>> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina
>>> ---
>>>
>>> Notes:
>>> new in v2
>>>
>>> src/conf/domain_conf.c | 46
>>> +++--
>>> src/conf/domain_conf.h | 9
>>> src/security/security_dac.c | 26 ++-
>>> src/security/security_manager.c | 4 ++--
>>> src/security/security_selinux.c | 24 +
>>> 5 files changed, 49 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index c7e20b8ba1..68dc2832cb 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr
>>> dest,
>>>
>>> void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
>>> {
>>> +size_t i;
>>> +
>>> if (!def)
>>> return;
>>>
>>> virDomainChrSourceDefClear(def);
>>> virObjectUnref(def->privateData);
>>>
>>> +if (def->seclabels) {
>>> +for (i = 0; i < def->nseclabels; i++)
>>> +virSecurityDeviceLabelDefFree(def->seclabels[i]);
>>> +VIR_FREE(def->seclabels);
>>> +}
>>> +
>>> +
>>> VIR_FREE(def);
>>> }
>>>
>>> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const
>>> virDomainChrSourceDef *src,
>>>
>>> void virDomainChrDefFree(virDomainChrDefPtr def)
>>> {
>>> -size_t i;
>>> -
>>> if (!def)
>>> return;
>>>
>>> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
>>> virDomainChrSourceDefFree(def->source);
>>> virDomainDeviceInfoClear(&def->info);
>>>
>>> -if (def->seclabels) {
>>> -for (i = 0; i < def->nseclabels; i++)
>>> -virSecurityDeviceLabelDefFree(def->seclabels[i]);
>>> -VIR_FREE(def->seclabels);
>>> -}
>>> -
>>> VIR_FREE(def);
>>> }
>>>
>>> @@ -10688,8 +10689,8 @@
>>> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>>> if (chr_def) {
>>
>> Is the @chr_def check necessary still? I assume it's there because
>> chr_def can be passed as NULL in some cases.
>>
>> Looks like all this was added as part of commit 'f8b08d0e'
>>
>> The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
>> each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
>> leads me to believe that the @chr_def should be removed.
>
> But this hunk is from virDomainChrSourceDefParseXML() function.
>
Yes, I knew that when I wrote the comment, but that wasn't the point.
Since you've moved the labels into @def instead and similarly altered
calls to virDomainChrSourceDefFormat such that they don't receive
chr_def (in the very next hunk of changes BTW), then I would think at
this point removing chr_def would be safe, but I suppose I could be
wrong. Hence why I asked.
So does it hurt to keep it, probably not; however, IIUC the reason why
it was there was because if it wasn't, then dereferencing chr_def to get
the [n]seclabels in the subsequent call wouldn't end well.
John
>> The rest looks good, so
>>
>> Reviewed-by: John Ferlan
>
> Thanks
>
> Pavel
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] hacking: Improve 'git send-email' documentation
On Thu, Jun 15, 2017 at 11:31:43AM +0800, Andrea Bolognani wrote: > For the benefit of first time contributors, we point out that 'git > send-email' might have to be installed separately; however, we omit > the fact that some configuration will likely be needed before it > can successfully deliver patches to the mailing list. > > Some minor tweaks to the existing contents are included as well. > > Signed-off-by: Andrea Bolognani > --- > docs/hacking.html.in | 36 > 1 file changed, 24 insertions(+), 12 deletions(-) [...] > -(Note that the "git send-email" subcommand may not be in > -the main git package and using it may require installation of a > -separate package, for example the "git-email" package in > -Fedora.) For a single patch you can omit > ---cover-letter, but a series of two or more > -patches needs a cover letter. For multiple patch series, is it worthwhile to mention the "--stat" option to include the diffstat in the cover letter? When reading a patch series, I find it useful to see the diffstat at a glance. E.g. when sending the top 4 commits: $ git send-email -4 --cover-letter --no-chain-reply-to \ --annotate --to=lib...@redhat.com master --stat Maybe others here have their own preferred method to include diffstat in cover letters. [...] -- /kashyap -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Pass the number of heads even with -vga qxl
On Wed, Jun 14, 2017 at 04:48:10PM +0200, Ján Tomko wrote: On Wed, Jun 14, 2017 at 03:56:15PM +0200, Martin Kletzander wrote: When added in multiple previous commits, it was used only with -device qxl(-vga), but for some QEMUs (< 1.6) we need to add this functionality when using -vga qxl as well. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283207 Signed-off-by: Martin Kletzander --- src/qemu/qemu_command.c| 8 .../qemuxml2argv-video-vga-qxl-heads.args | 30 ++ .../qemuxml2argv-video-vga-qxl-heads.xml | 47 ++ tests/qemuxml2argvtest.c | 3 ++ 4 files changed, 88 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-qxl-heads.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3e1552a1b593..9687d367a6d4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4613,6 +4613,14 @@ qemuBuildVgaVideoCommand(virCommandPtr cmd, virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u", dev, vgamem / 1024); } + +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_MAX_OUTPUTS)) +video->heads = 0; I would expect a qemuBuild.*Command function to build the command line, not alter the domain definition. Also, this goes against commit ef11e770 qemu_command: don't modify heads for graphics device I, actually think that patch should've not made it in, but I must've missed that it was posted. The setting merely updates the XML so that it is visible whether or not the setting was applied. It then also works when migrating to a newer qemu, so that it doesn't reset the limitation. But I'll leave this to other time and will post a v2 without that. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemuDomainGetPreservedMounts: Prune nested mount points
On 06/14/2017 09:52 PM, John Ferlan wrote:
>
>
> On 06/12/2017 11:57 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
>>
>> There can be nested mount points. For instance /dev/shm/blah can
>> be a mount point and /dev/shm too. It doesn't make much sense to
>> return the former path because callers preserve the latter (and
>> with that the former too). Therefore prune nested mount points.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_domain.c | 23 ++-
>> 1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 23b92606e..accf05a6f 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -7533,7 +7533,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr
>> cfg,
>> size_t *ndevPath)
>> {
>> char **paths = NULL, **mounts = NULL;
>> -size_t i, nmounts;
>> +size_t i, j, nmounts;
>>
>> if (virFileGetMountSubtree(PROC_MOUNTS, "/dev",
>> &mounts, &nmounts) < 0)
>> @@ -7545,6 +7545,27 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr
>> cfg,
>> return 0;
>> }
>>
>> +/* There can be nested mount points. For instance
>> + * /dev/shm/blah can be a mount point and /dev/shm too. It
>> + * doesn't make much sense to return the former path because
>> + * caller preserves the latter (and with that the former
>> + * too). Therefore prune nested mount points.
>> + * NB mounts[0] is "/dev". Should we start the outer loop
>> + * from the beginning of the array all we'd be left with is
>> + * just the first element. Think about it.
>> + */
>> +for (i = 1; i < nmounts; i++) {
>> +for (j = i + 1; j < nmounts;) {
>> +if (STRPREFIX(mounts[j], mounts[i])) {
>> +VIR_DEBUG("Dropping path %s because of %s", mounts[j],
>> mounts[i]);
>> +VIR_DELETE_ELEMENT(mounts, j, nmounts);
>> +} else {
>> +j++;
>> +}
>
> Ewww
>
> I prefer a :
>
>j = i + 1;
>while (j < nmounts) {
> if ()
> ...
> else
> j++;
>}
Okay. I prefer the other approach (what I have here), but I don't care
that much.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
On 06/14/2017 09:50 PM, John Ferlan wrote: > > > On 06/12/2017 11:57 AM, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1431112 >> >> After 290a00e41d we know how to deal with file mount points. >> However, when cleaning up the temporary location for preserved >> mount points we are still calling rmdir(). This won't fly for >> files. We need to call unlink(). Now, since we don't really care >> if the cleanup succeeded or not (it's the best effort anyway), we >> can call both rmdir() and unlink() without need for >> differentiation between files and directories. >> >> Signed-off-by: Michal Privoznik >> --- >> src/qemu/qemu_domain.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> > > But why call both? I don't think you can call unlink() over a directory, can you? And sure, I could call stat() just to find out if it's a dir or a file and call just one of the pair. Or I can call both and ignore any errors. The result is the same, isn't it? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemuDomainGetPreservedMounts: Fix suffixes for corner cases
On 06/14/2017 09:58 PM, John Ferlan wrote: > > > On 06/12/2017 11:57 AM, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1431112 >> >> Imagine a FS mounted on /dev/blah/blah2. Our process of creating >> suffix for temporary location where all the mounted filesystems >> are moved is very simplistic. We want: >> >> /var/run/libvirt/qemu/$domName.$suffix\ >> >> were $suffix is just the mount point path stripped of the "/dev/" >> preffix. For instance: > > s/preffix/prefix > >> >> /var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue >> /var/run/libvirt/qemu/fedora.pts for /dev/pts >> >> and so on. Now if we plug /dev/blah/blah2 into the example we see >> some misbehaviour: >> >> /var/run/libvirt/qemu/fedora.blah/blah2 >> >> Well, misbehaviour if /dev/blah/blah2 is a file, because in that >> case we call virFileTouch() instead of virFileMakePath(). >> > > You didn't finish my bedtime story! > > Am I to assume that instead of : > > /var/run/libvirt/qemu/fedora.blah/blah2 > > we would get > > /var/run/libvirt/qemu/fedora.blah.blah2 Yes. > > taking things one step further... > > would /dev/blah/blah2/blah3 > > be > > /var/run/libvirt/qemu/fedora.blah.blah2.blah3 Yes. > > That's what I see coded at least... Or should the path be: > > /var/run/libvirt/qemu/fedora.blah/blah2.blah3 Nope. The former one. > > > It would seem you'd want to get to the end, reverse search on '/' then > if that spot is greater than @off, then convert it to a '.', So basically, this is my approach just reversed. What'd be the benefits? I find my algorithm small and easy to understand. > but what do > I know. I keep to the simple life and don't use namespaces. Well, until a7cc039dc I didn't know that you can bind mount files. What a strange thing to learn. What I want to say - you can learn some new stuff when using namespaces ;-) Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Creating mediated devices with libvirt
On Thu, Jun 15, 2017 at 12:06:43AM +0200, Erik Skultety wrote: > Hi all, > > so there's been an off-list discussion about finally implementing creation of > mediated devices with libvirt and it's more than desired to get as many > opinions > on that as possible, so please do share your ideas. This did come up already > as > part of some older threads ([1] for example), so this will be a respin of the > discussions. Long story short, we decided to put device creation off and focus > on the introduction of the framework as such first and build upon that later, > i.e. now. > > [1] https://www.redhat.com/archives/libvir-list/2017-February/msg00177.html > > > PART 1: NODEDEV-DRIVER > > > API-wise, device creation through the nodedev driver should be pretty > straightforward and without any issues, since virNodeDevCreateXML takes an XML > and does support flags. Looking at the current device XML: > > > mdev_0cce8709_0640_46ef_bd14_962c7f73cc6f > > /sys/devices/pci:00/.../0cce8709-0640-46ef-bd14-962c7f73cc6f > pci__03_00_0 > > vfio_mdev > > > > > UUID > > > > We can ignore ,, elements, since these are useless > during creation. We also cannot use since we don't support arbitrary > names and we also can't rely on users providing a name in correct form which > we > would need to further parse in order to get the UUID. > So since the only thing missing to successfully use create an mdev using XML > is > the UUID (if user doesn't want it to be generated automatically), how about > having a subelement under just like PCIs have and > friends, USBs have & , interfaces have to uniquely > identify the device even if the name itself is unique. > Removal of a device should work as well, although we might want to > consider creating a *Flags version of the API. > > = > PART 2: DOMAIN XML & DEVICE AUTO-CREATION, NO POLICY INVOLVED! > = > > There were some doubts about auto-creation mentioned in [1], although they > weren't specified further. So hopefully, we'll get further in the discussion > this time. > > From my perspective there are two main reasons/benefits to that: > > 1) Convenience > For apps like virt-manager, user will want to add a host device transparently, > "hey libvirt, I want an mdev assigned to my VM, can you do that". Even for > higher management apps, like oVirt, even they might not care about the parent > device at all times and considering that they would need to enumerate the > parents, pick one, create the device XML and pass it to the nodedev driver, > IMHO > it would actually be easier and faster to just do it directly through sysfs, > bypassing libvirt once again The convenience only works if the policy we've provided in libvirt actually matches the policy the application wants. I think it is quite likely that with cloud the mdevs will be created out of band from the domain startup process. It is possible the app will just have a fixed set of mdevs pre-created when the host starts up. Or that the mgmt app wants the domain startup process to be a two phase setup, where it first allocates the resources needed, and later then tries to start the guest. This is why I keep saying that putting this kind of "convenient" policy in libvirt is a bad idea - it is essentially just putting a bit of virt-manager code into libvirt - more advanced apps will need more flexibility in this area. > 2) Future domain migration > Suppose now that the mdev backing physical devices support state dump and > reload. Chances are, that the corresponding mdev doesn't even exist or has a > different UUID on the destination, so libvirt would do its best to handle this > before the domain could be resumed. This is not an unusual scenario - there are already many other parts of the device backend config that need to change prior to migration, especially for anything related to host devices, so apps already have support for doing this, which is more flexible & convenient becasue it doesn't tie creation of the mdevs to running of the migrate command. IOW, I'm still against adding any kind of automatic creation policy for mdevs in libvirt. Just provide the node device API support. 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
Re: [libvirt] [PATCH 1/2] util: implement virStrToDoubleSafe().
On Wed, Jun 14, 2017 at 01:24:42PM -0300, Julio Faracco wrote:
Following the GNU Documentation, functions to convert double/float to string
and vice versa, use the locale to set the mantissa separator. Some languages
use comma and other use dot as a separator.
For example: 1,212.67 (en_US) = 1.112,67 (pt_BR).
This can be used to parse values in float/double from XML and other definition
files.
Signed-off-by: Julio Faracco
---
src/libvirt_private.syms | 1 +
src/util/virstring.c | 31 +++
src/util/virstring.h | 4
3 files changed, 36 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 044510f..9d791e6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2659,6 +2659,7 @@ virStringTrimOptionalNewline;
virStrncpy;
virStrndup;
virStrToDouble;
+virStrToDoubleSafe;
virStrToLong_i;
virStrToLong_l;
virStrToLong_ll;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 089b539..6dad00f 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -537,6 +537,37 @@ virStrToDouble(char const *s,
}
int
+virStrToDoubleSafe(char const *s,
+ char **end_ptr,
+ double *result)
+{
+char *cur_locale = NULL;
+char *saved_locale = NULL;
+int ret = -1;
+
+cur_locale = setlocale (LC_ALL, NULL);
+if (!cur_locale)
+return ret;
+
+if (VIR_STRDUP(saved_locale, cur_locale) < 0)
+goto cleanup;
+
+if (!setlocale (LC_ALL, "C"))
+goto cleanup;
+
This is MT-Unsafe. We cannot set this for the whole process, so NACK to
this approach. You need to use uselocale() as in commit 43bfa23e6f96
(or just see virDoubleToStr()).
Martin
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] hacking: Improve 'git send-email' documentation
On Thu, Jun 15, 2017 at 11:31:43AM +0800, Andrea Bolognani wrote: For the benefit of first time contributors, we point out that 'git send-email' might have to be installed separately; however, we omit the fact that some configuration will likely be needed before it can successfully deliver patches to the mailing list. Some minor tweaks to the existing contents are included as well. Signed-off-by: Andrea Bolognani --- docs/hacking.html.in | 36 1 file changed, 24 insertions(+), 12 deletions(-) git config sendemail.to libvir-list@redhat.com +For a single patch you can omit +--cover-letter, but a series of two or more +patches needs a cover letter. +If everything went well, your patch should show up on the +https://www.redhat.com/archives/libvir-list/";>libvir-list +archives in a matter of minutes; if you still can't find it on +there after an hour or so, you should double-check your setup. First time posts to the mailing list still have to be approved by the moderator, AFAIK. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote: > > > On 05/29/2017 10:31 AM, Pavel Hrdina wrote: > > In the case that virtlogd is used as stdio handler we pass to QEMU > > only FD to a PIPE connected to virtlogd instead of the file itself. > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 > > > > Signed-off-by: Pavel Hrdina > > --- > > > > Notes: > > new in v2 > > > > src/lxc/lxc_process.c| 6 ++--- > > src/qemu/qemu_security.c | 9 +-- > > src/security/security_apparmor.c | 7 -- > > src/security/security_dac.c | 54 > > +++- > > src/security/security_driver.h | 6 +++-- > > src/security/security_manager.c | 12 ++--- > > src/security/security_manager.h | 6 +++-- > > src/security/security_nop.c | 6 +++-- > > src/security/security_selinux.c | 53 > > ++- > > src/security/security_stack.c| 12 ++--- > > tests/securityselinuxlabeltest.c | 2 +- > > 11 files changed, 127 insertions(+), 46 deletions(-) > > > > Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why > is (!chr_seclabel) even matter? If you configure the label we shouldn't ignore it in some cases, that's just wrong. If the label is set for the char device we will configure it every time even if it will fail to start the guest, it's a responsibility of the user to configure proper label if it is provided. > IIUC, whether or not someone set the label for the chardev, for this > particular issue/config where the chardev has a , we > don't want to set (or restore) the label. I feel like I'm missing > something subtle. Maybe a bit more explanation of the adjustment would > help me... This is not for the but for the . We don't relabel $path for at all. > Wouldn't these changes end up selecting "any" chardev if > chardevStdioLogd ended up being set regardless of whether they were > actually using the log file? I don't know what you mean by this sentence? > As an aside, I think there's an "oddity" when it comes to the Restore, > but I'm not sure how to put it into words exactly. If a guest is running > code prior to this set of changes, would it have successfully run a Set? > If so, then after applying this change and restarting, the label > wouldn't be reset, right? What happens at guest shutdown - does the > label not get unset now? Of course this is all "interaction" with > virtlogd restart that really throws a monkey wrench into things. No, that's not correct. The @chardevStdioLogd is stored in the status XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when the libvirtd is stopped and started with this patch applied the status XML doesn't have the @chardevStdioLogd stored in it so it will be false and we will reset the label. The @chardevStdioLogd is updated only when the domain is started and we will store the value in the status XML only with new libvirtd and only in that case we will not set/restore the label. > Also, why is the Smartcard chardev handling not using this The smartcard doesn't ever use virtlogd as stdio handler. Pavel signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
