[libvirt] 答复: Re: [PATCH v2] network: allow to specify buffer size fornetlink socket
>On 07/17/2017 08:00 AM, John Ferlan wrote:>>> On 07/16/2017 05:04 PM, Laine >Stump wrote:>> On 07/11/2017 09:01 PM, ZhiPeng Lu wrote:>>> This patchs allow >to set the buffer size for netlink socket in>>> the libvirtd configuration >file. The default buffer size remain>>> as before at 128k.>> See my more >detailed response to your earlier patch here:>> >https://www.redhat.com/archives/libvir-list/2017-July/msg00566.html There >should be no need to configure the initial libnl buffer size,>> because we >enable MSG_PEEK on the libnl sockets (and recent versions of>> libnl have it >turned on by default anyway). If that's not permitting the>> buffer to >auto-grow as necessary, then there is a different bug somewhere. If an old >version of libnl is the problem, then perhaps a patch which>> just adds a >comment in virNetlinkCreateSocket to "summarize" what gets>> discovered w/r/t >MSG_PEEK and the "correct" minimum version of libnl so> that the "next" person >to come this way will have a chance at>> understanding what needs to be done >without going through the submit a>> patch changing the size again!>>> >All >that said, having it be configurable could be useful for someone who> >has a >system that doesn't have that version, while still working as > >expected for the right version.>I think it may need to be a fairly unusual > >combination of kernel,>libvirt, and libnl versions, combined with pretty > >"big" hardware in>order for that to happen. More information from ZhiPeng > >about the>versions of those packages might allow us to make a better > >informed>decision.>Workarounds are okay when necessary. But adding a config > >parameter is>something that would need to be left in forever, leaving more > >code to>maintain, and all for a bug that shouldn't even be there today, > >much>less 6 months or a year from now - turning on message peek was > >supposed>to "eliminate this problem totally and permanently". If it didn't, > >I'd>like to know why.>ZhipPeng - can you tell us more about your setup? > >package versions,>hardware, example XML, gdb backtrace at the instant the > >error message is>logged? - Thanks. i will try to update libnl3 to 3.2.29 ,now libnl3 is 3.2.8 in my host 芦志朋 luzhipeng IT开发工程师 IT Development Engineer 操作系统产品部/中心研究院/系统产品 OS Product Dept./Central R＆D Institute/System Product 深圳市南山区科技南路55号中兴通讯研发大楼33楼 33/F, R&D Building, ZTE Corporation Hi-tech Road South, Hi-tech Industrial Park Nanshan District, Shenzhen, P.R.China, 518057 T: +86 755 F:+86 755 M: +86 xxx E: lu.zhip...@zte.com.cn www.zte.com.cn 原始邮件 发件人： 收件人： 抄送人： 芦志朋10108272 日 期 ：2017年07月21日 10:01 主 题 ：Re: [libvirt] [PATCH v2] network: allow to specify buffer size fornetlink socket On 07/17/2017 08:00 AM, John Ferlan wrote: > > On 07/16/2017 05:04 PM, Laine Stump wrote: >> On 07/11/2017 09:01 PM, ZhiPeng Lu wrote: >>> This patchs allow to set the buffer size for netlink socket in >>> the libvirtd configuration file. The default buffer size remain >>> as before at 128k. >> See my more detailed response to your earlier patch here: >> >> >> https://www.redhat.com/archives/libvir-list/2017-July/msg00566.html >> >> There should be no need to configure the initial libnl buffer size, >> because we enable MSG_PEEK on the libnl sockets (and recent versions of >> libnl have it turned on by default anyway). If that's not permitting the >> buffer to auto-grow as necessary, then there is a different bug somewhere. >> > If an old version of libnl is the problem, then perhaps a patch which > just adds a comment in virNetlinkCreateSocket to "summarize" what gets > discovered w/r/t MSG_PEEK and the "correct" minimum version of libnl so > that the "next" person to come this way will have a chance at > understanding what needs to be done without going through the submit a > patch changing the size again! > > All that said, having it be configurable could be useful for someone who > has a system that doesn't have that version, while still working as > expected for the right version. I think it may need to be a fairly unusual combination of kernel, libvirt, and libnl versions, combined with pretty "big" hardware in order for that to happen. More information from ZhiPeng about the versions of those packages might allow us to make a better informed decision. Workarounds are okay when necessary. But adding a config parameter is something that would need to be left in forever, leaving more code to maintain, and all for a bug that shouldn't even be there today, much less 6 months or a year from now - turning on message peek was supposed to "eliminate this problem totally and permanently". If it didn't, I'd like to know why. ZhipPeng - can you tell us more about your setup? package versions, hardware, example XML, gdb backtrace at the instant the error message is logged?-- libvir-list mailing list libvir-list@redhat.com https://www.
Re: [libvirt] [PATCH v2] network: allow to specify buffer size for netlink socket
On 07/17/2017 08:00 AM, John Ferlan wrote: > > On 07/16/2017 05:04 PM, Laine Stump wrote: >> On 07/11/2017 09:01 PM, ZhiPeng Lu wrote: >>> This patchs allow to set the buffer size for netlink socket in >>> the libvirtd configuration file. The default buffer size remain >>> as before at 128k. >> See my more detailed response to your earlier patch here: >> >> >> https://www.redhat.com/archives/libvir-list/2017-July/msg00566.html >> >> There should be no need to configure the initial libnl buffer size, >> because we enable MSG_PEEK on the libnl sockets (and recent versions of >> libnl have it turned on by default anyway). If that's not permitting the >> buffer to auto-grow as necessary, then there is a different bug somewhere. >> > If an old version of libnl is the problem, then perhaps a patch which > just adds a comment in virNetlinkCreateSocket to "summarize" what gets > discovered w/r/t MSG_PEEK and the "correct" minimum version of libnl so > that the "next" person to come this way will have a chance at > understanding what needs to be done without going through the submit a > patch changing the size again! > > All that said, having it be configurable could be useful for someone who > has a system that doesn't have that version, while still working as > expected for the right version. I think it may need to be a fairly unusual combination of kernel, libvirt, and libnl versions, combined with pretty "big" hardware in order for that to happen. More information from ZhiPeng about the versions of those packages might allow us to make a better informed decision. Workarounds are okay when necessary. But adding a config parameter is something that would need to be left in forever, leaving more code to maintain, and all for a bug that shouldn't even be there today, much less 6 months or a year from now - turning on message peek was supposed to "eliminate this problem totally and permanently". If it didn't, I'd like to know why. ZhipPeng - can you tell us more about your setup? package versions, hardware, example XML, gdb backtrace at the instant the error message is logged? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/3] exposing busy polling support for vhost-net
On 07/17/2017 07:39 AM, Peter Krempa wrote:
> On Mon, Jul 17, 2017 at 07:27:24 -0400, sferd...@redhat.com wrote:
>> 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' and that because libvirt automatically fallback to QEMU
>> if the driver is not specified where that option is not available.
>>
>> The option 'poll_us' takes a positive. 0 means that the option
>> is not going to be exposed.
> We had a similar attempt to do this for disk polling, but that was
> rejected since it's not very straightforward for the users to tune this
> variable. I think this falls into the same category.
>
> Here's the discussion for iothread polling:
>
> https://www.redhat.com/archives/libvir-list/2017-February/msg01048.html
The problem is there are already many similar driver-specific
odd/not-straightforward tuning parameters present in the XML, so it's
difficult to see where to draw the line. I think this may be due to the
fact that now whenever a new option is added to qemu, a bugzilla record
is semi-automatically opened (by a human, but they almost always do it)
requesting libvirt to support that new option, and libvirt developers
like to be accommodating (and while some reviews/reviewers base the
conversation on "should we even do this?", many/most are simply based on
"assuming we want to do this, is this a proper way to do it?", or even
"assuming we want to do this, and this is the proper way to do it, has
all allocated memory been freed, and error conditions handled?" (e.g. my
own review of V1 of these patches :-P).
In one of the followups to the iothread polling above, Stefan pointed
out a way to accomplish the tuning via qemu commandline passthrough,
using qemu's "-set" commandline argument. If libvirt doesn't add support
for this attribute in the XML, that's how it will need to be set if
someone requires it. And it *is* possible to set poll-us for an
interface in this way, e.g.:
which results in this addition to the commandline:
-set netdev.hostnet1.poll-us=20
As long as the commandline generated for the interface was something
like this:
-netdev tap,fd=30,id=hostnet1,vhost=on,vhostfd=32
everything will work out properly.
There are 3 issues with this though:
1) it depends on the specific interface you want to control having the
id ("alias" in libvirt-speak) "hostnet0". If the config is modified to
add/remove any interfaces prior to this one in the config, the alias
will change, and the qemu:commandline will silently begin modifying the
wrong interface. (although we've discussed it, I think it is still not
possible to explicitly set the alias for a device - they are always
automatically determined when the domain is started / device is hotplugged)
2) libvirt documentation explicitly says that use of
is not supported, and any use of it will taint the domain config. This
means that nobody will be able to use it in a supported commercial setting.
Well, I forget what the 3rd reason was, but it was a *good* one, and it
made a lot of sense 3 days ago when I started writing this message :-P.
Oh, wait, now I remember:
3) It's not possible to set options using when
hotplugging a device, so the functionality would only be available for
interfaces that were present when the domain was started.
Anyway, I'm sympathetic to the sentiment of "don't add frivolous new
knobs to libvirt config just because someone asked for it and it's easy
to do". I've made that same argument before myself. I just think that if
we're going to be restrictive like that, we need to be consistent about
it, and need to have a story for how to accommodate the request for the
functionality in a different way that doesn't have limited
functionality, and doesn't render the software unsupportable. I think
that in this case none of those is true.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Add support for virtio-net.tx_queue_size
On 07/18/2017 07:12 AM, Michal Privoznik wrote: > [Adding MST who wrote qemu patches] > > On 07/18/2017 12:21 PM, Peter Krempa wrote: >> On Tue, Jul 18, 2017 at 09:01:31 +0200, Michal Privoznik wrote: >>> On 07/18/2017 08:23 AM, Peter Krempa wrote: On Mon, Jul 17, 2017 at 15:39:56 +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1462653 This bugzilla is not public. >>> Okay, I'll drop it from the commit message. >> Also add proper explanation what the benefits are, since upstream can't >> read the motivation from the bugzilla. >> > Just like I've added support for setting rx_queue_size (in > c56cdf259 and friends), qemu just gained support for setting tx > ring size. >> [...] >> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index c12efcf78..58662cf48 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in [...] > @@ -5201,6 +5201,20 @@ 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. > > + tx_queue_size > + > +The optional tx_queue_size attribute controls > +the size of virtio ring for each queue as described above. > +The default value is hypervisor dependent and may change > +across its releases. Moreover, some hypervisors may pose > +some restrictions on actual value. For instance, latest > +QEMU (as of 2017-07-13) requires value to be a power of two >> Refer to a proper version of qemu when this is supported, not a date. >> > +from [256, 1024] range. > +Since 3.6.0 (QEMU and KVM > only) This is ridiculous. Since we can't figure out how to set this, how are users supposed to figure this out? >>> Well, you've cut off the line that reads; >>> >>> In general you should leave this option alone, unless you >>> are very certain you know what you are doing. >>> >>> So only users that know how virtio works under the hood are expected to >>> also know what rx/tx queue size is and how to set it. But frankly, I >> This statement is ridiculous by itself. > Why? There are more experienced users (for whom libvirt's just a stable > API) and less experienced ones (for whom libvirt's and tools on the top > of it are great for their automatic chose of parameters, e.g. gnome boxes). Beyond that, that same statement (or something nearly identical) is repeated in multiple places throughout the XML documentation. There are at least two classes of these attributes that I can think of: 1) attributes that are automatically set to a sane value by libvirt when not specified (and they usually *aren't* specified), and saved in the config XML in order to assure they are set the same every time the domain is started (to preserve guest ABI). These are intended to record whatever was the default setting for the attribute at the time the domain was created. For example, a pcie-root-port controller might have set, if that was the only type of pcie-root-port available at the time a domain was created; this comes in handy now that there is a generic pcie-root-port (which also has ) - existing domains don't get their ABI screwed up when they're migrated to a newer host. 2) knobs that have been added in over the years at the request/demand from below (qemu) and above (ovirt / openstack), many of them obscure, difficult to explain with any amount of useful detail, and almost never worthy of being manually set (and if you "don't know what you're doing", you're just as likely to make things worse as to make them better). tx_queue_size is one of the latter. For either of these types of attributes, they need to be documented so that someone encountering them (or actively searching them out) will at least have a basic idea what the attribute is named / used for, but we also need to warn the casual user to not mess with them. I don't see anything ridiculous about that. > >>> think users setting this are always gonna go with the highest value >>> avaliable (1024). Such detailed description is a copy of rx_virtio_queue >>> size description which is result of review. >>> Is it really needed? How should it be configured? Can't we or qemu pick a sane value? >>> No. Some users need bigger virtio rings otherwise they see a packet >>> drop. So this is a fine tuning that heavily depends on the use case. >>> Thus libvirt should not try to come up with some value. >> That's why I think it's wrong. What's the drawback of setting it to >> maximum? Which workloads will hit it? Why is the default not sufficient? >> >> And most notably, how do the users figure out that they need it? I think it's a bit too much burden on libvirt to expect that much detail to be included in formatdomain.html. Heck, I don't know if anyone even *knows* that much detail rig
Re: [libvirt] [PATCH 11/19] storage: Introduce virStoragePoolObjNew
On 07/11/2017 11:17 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:18AM -0400, John Ferlan wrote:
>> Create/use a helper to perform object allocation.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virstorageobj.c | 34 +++---
>> 1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
>> index 7d6b311..0079472 100644
>> --- a/src/conf/virstorageobj.c
>> +++ b/src/conf/virstorageobj.c
>> @@ -37,6 +37,27 @@
>> VIR_LOG_INIT("conf.virstorageobj");
>>
>>
>> +static virStoragePoolObjPtr
>> +virStoragePoolObjNew(virStoragePoolDefPtr def)
>> +{
>> +virStoragePoolObjPtr obj;
>> +
>> +if (VIR_ALLOC(obj) < 0)
>> +return NULL;
>> +
>> +if (virMutexInit(&obj->lock) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("cannot initialize mutex"));
>> +VIR_FREE(obj);
>> +return NULL;
>> +}
>> +virStoragePoolObjLock(obj);
>> +obj->active = 0;
>> +obj->def = def;
>> +return obj;
>> +}
>> +
>> +
>> virStoragePoolDefPtr
>> virStoragePoolObjGetDef(virStoragePoolObjPtr obj)
>> {
>> @@ -386,24 +407,15 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr
>> pools,
>> return obj;
>> }
>>
>> -if (VIR_ALLOC(obj) < 0)
>> +if (!(obj = virStoragePoolObjNew(def)))
>> return NULL;
>
> The new virStoragePoolObjNew() doesn't have to take @def and ...
>
>>
>> -if (virMutexInit(&obj->lock) < 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("cannot initialize mutex"));
>> -VIR_FREE(obj);
>> -return NULL;
>> -}
>> -virStoragePoolObjLock(obj);
>> -obj->active = 0;
>> -
>> if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) {
>> +obj->def = NULL;
>
> ... need to set the @obj->def to NULL and ...
>
>> virStoragePoolObjUnlock(obj);
>> virStoragePoolObjFree(obj);
>> return NULL;
>> }
>> -obj->def = def;
>
> ... just keep this line as it is.
>
These are all changed in my local branch.
I figure as long as you're good with my most recent comments to 8/19
that since I have ACKs now through 9/19 - I'll push those, then post a
new version for the rest (eventually, but not right away).
Hopefully that works. I think I want to work through the NodeDev,
Secrets, NWFilter, and Interface first. Then jump back into Storage and
Network before perhaps one day considering domain.
Thanks for the reviews here though - I certainly appreciate it.
John
>>
>> return obj;
>> }
>> --
>> 2.9.3
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/19] storage: Create accessor API's for virStoragePoolObj
On 07/14/2017 09:55 AM, Pavel Hrdina wrote: > On Tue, May 09, 2017 at 11:30:17AM -0400, John Ferlan wrote: >> In preparation for making a private object, create accessor API's for >> consumer storage functions to use: >> >> virStoragePoolObjGetDef >> virStoragePoolObjGetNewDef >> virStoragePoolObjStealNewDef >> virStoragePoolObjGetConfigFile >> virStoragePoolObjSetConfigFile >> virStoragePoolObjIsActive >> virStoragePoolObjSetActive >> virStoragePoolObjGetAsyncjobs >> virStoragePoolObjIncrAsyncjobs >> virStoragePoolObjDecrAsyncjobs >> >> Signed-off-by: John Ferlan >> --- >> src/conf/virstorageobj.c | 74 >> >> src/conf/virstorageobj.h | 36 +++ >> src/libvirt_private.syms | 10 +++ >> 3 files changed, 115 insertions(+), 5 deletions(-) > > Base on the review of following patches we need to add one more accessor > virStoragePoolObjGetAutostartLink(). > I will place it right after virStoragePoolObjGetConfigFile since the _virStoragePoolObj has autotstartLink after configFile. John > The rest is OK. > > Pavel > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/19] storage: Create accessor API's for virStoragePoolObj
On 07/14/2017 11:37 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:17AM -0400, John Ferlan wrote:
>> In preparation for making a private object, create accessor API's for
>> consumer storage functions to use:
>>
>> virStoragePoolObjGetDef
>> virStoragePoolObjGetNewDef
>> virStoragePoolObjStealNewDef
>> virStoragePoolObjGetConfigFile
>> virStoragePoolObjSetConfigFile
>> virStoragePoolObjIsActive
>> virStoragePoolObjSetActive
>> virStoragePoolObjGetAsyncjobs
>> virStoragePoolObjIncrAsyncjobs
>> virStoragePoolObjDecrAsyncjobs
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virstorageobj.c | 74
>>
>> src/conf/virstorageobj.h | 36 +++
>> src/libvirt_private.syms | 10 +++
>> 3 files changed, 115 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
>> index 23346f3..7d6b311 100644
>> --- a/src/conf/virstorageobj.c
>> +++ b/src/conf/virstorageobj.c
>> @@ -37,6 +37,80 @@
>> VIR_LOG_INIT("conf.virstorageobj");
>
> [...]
>
>> +void
>> +virStoragePoolObjStealNewDef(virStoragePoolObjPtr obj)
>> +{
>> +virStoragePoolDefFree(obj->def);
>> +obj->def = obj->newDef;
>> +obj->newDef = NULL;
>> +}
>
> I didn't notice this until the usage in following patches, the "Steal"
> part of the name is confusing. We have a macro "VIR_STEAL_PTR" which
> returns pointer and sets the original one to NULL. This function
> doesn't return the pointer, it replaces @def with @newDef.
>
> How about virStoragePoolObjUseNewDef() or
> virStoragePoolObjDefUseNewDef() or feel free to come up with another
> name which would be better than "Steel".
>
> Pavel
>
It's theft by deception.
How about virStoragePoolObjDefSwapNewDef
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/19] storage: Use consistent variable names for driver
On 07/14/2017 11:07 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:15AM -0400, John Ferlan wrote:
>> A virStoragePoolObjPtr will be an 'obj'.
>>
>> A virStoragePoolPtr will be a 'pool'.
>>
>> A virStorageVolPtr will be a 'vol'.
>>
>> A virStorageVolDefPtr will be a 'voldef'.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/storage/storage_driver.c | 1158
>> +-
>> src/storage/storage_driver.h |4 +-
>> 2 files changed, 582 insertions(+), 580 deletions(-)
>>
I have had some more time to think about the other comment regarding
whether @obj should be @poolObj or @poolobj...
If some day in the future (as noted in the patch 7 response) the @pools
changes to @poolobjs it'll be eye test in order to spot the difference
between @poolobj and @poolobjs, so I'd like to keep it as @obj. Long
time ago I had the sheer joy of trying to search code for 'l' and '1' as
well as 'O' and '0'
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 2103ed1..6122396 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
[...] lots to cut to first comment I saw [...]
>>
>> static virStorageVolPtr
>> -storageVolCreateXML(virStoragePoolPtr obj,
>> +storageVolCreateXML(virStoragePoolPtr pool,
>> const char *xmldesc,
>> unsigned int flags)
>> {
>> -virStoragePoolObjPtr pool;
>> +virStoragePoolObjPtr obj;
>> virStorageBackendPtr backend;
>> virStorageVolDefPtr voldef = NULL;
>> -virStorageVolPtr ret = NULL, volobj = NULL;
>> +virStorageVolPtr vol = NULL, volobj = NULL;
>
> The @volobj should be also renamed, I would rename it like this:
>
> @ret -> @vol
> @volobj -> @newvol
>
> and ...
>
>>
>> virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>>
>> -if (!(pool = virStoragePoolObjFromStoragePool(obj)))
>> +if (!(obj = virStoragePoolObjFromStoragePool(pool)))
>> return NULL;
>>
>> -if (!virStoragePoolObjIsActive(pool)) {
>> +if (!virStoragePoolObjIsActive(obj)) {
>> virReportError(VIR_ERR_OPERATION_INVALID,
>> - _("storage pool '%s' is not active"),
>> pool->def->name);
>> + _("storage pool '%s' is not active"),
>> obj->def->name);
>> goto cleanup;
>> }
>>
>> -if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
>> +if ((backend = virStorageBackendForType(obj->def->type)) == NULL)
>> goto cleanup;
>>
>> -voldef = virStorageVolDefParseString(pool->def, xmldesc,
>> +voldef = virStorageVolDefParseString(obj->def, xmldesc,
>> VIR_VOL_XML_PARSE_OPT_CAPACITY);
>> if (voldef == NULL)
>> goto cleanup;
>> @@ -1799,10 +1799,10 @@ storageVolCreateXML(virStoragePoolPtr obj,
>> goto cleanup;
>> }
>>
>> -if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0)
>> +if (virStorageVolCreateXMLEnsureACL(pool->conn, obj->def, voldef) < 0)
>> goto cleanup;
>>
>> -if (virStorageVolDefFindByName(pool, voldef->name)) {
>> +if (virStorageVolDefFindByName(obj, voldef->name)) {
>> virReportError(VIR_ERR_STORAGE_VOL_EXIST,
>> _("'%s'"), voldef->name);
>> goto cleanup;
>> @@ -1815,21 +1815,21 @@ storageVolCreateXML(virStoragePoolPtr obj,
>> goto cleanup;
>> }
>>
>> -if (VIR_REALLOC_N(pool->volumes.objs,
>> - pool->volumes.count+1) < 0)
>> +if (VIR_REALLOC_N(obj->volumes.objs,
>> + obj->volumes.count + 1) < 0)
>> goto cleanup;
>>
>> /* Wipe any key the user may have suggested, as volume creation
>> * will generate the canonical key. */
>> VIR_FREE(voldef->key);
>> -if (backend->createVol(obj->conn, pool, voldef) < 0)
>> +if (backend->createVol(pool->conn, obj, voldef) < 0)
>> goto cleanup;
>>
>> -pool->volumes.objs[pool->volumes.count++] = voldef;
>> -volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
>> +obj->volumes.objs[obj->volumes.count++] = voldef;
>> +volobj = virGetStorageVol(pool->conn, obj->def->name, voldef->name,
>>voldef->key, NULL, NULL);
>> if (!volobj) {
>> -pool->volumes.count--;
>> +obj->volumes.count--;
>> goto cleanup;
>> }
>>
>> @@ -1850,24 +1850,24 @@ storageVolCreateXML(virStoragePoolPtr obj,
>> memcpy(buildvoldef, voldef, sizeof(*voldef));
>>
>> /* Drop the pool lock during volume allocation */
>> -pool->asyncjobs++;
>> +obj->asyncjobs++;
>> voldef->building = true;
>> -virStoragePoolObjUnlock(pool);
>> +virStoragePoolObjUnlock(obj);
>>
>> -buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags);
>> +buildret = backend->buildVol(pool->co
[libvirt] [PATCH] tools: virsh: Adding unix socket support to 'domdisplay' command.
This commit adds the unix socket URL support to 'domdisplay' command.
Before, even if an user was using unix socket to define a spice graphics,
the command 'domdisplay' showed that the settings were not supported. Now,
the command shows the proper URL: spice+unix://foo/bar.sock.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336720
Signed-off-by: Julio Faracco
---
tools/virsh-domain.c | 49 ++---
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0684979..090714b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10948,6 +10948,8 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
char *xpath = NULL;
char *listen_addr = NULL;
int port, tls_port = 0;
+char *type_conn = NULL;
+char *socket = NULL;
char *passwd = NULL;
char *output = NULL;
const char *scheme[] = { "vnc", "spice", "rdp", NULL };
@@ -11008,9 +11010,6 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
if (tmp)
tls_port = 0;
-if (!port && !tls_port)
-continue;
-
/* Create our XPATH lookup for the current display's address */
if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "@listen") < 0)
goto cleanup;
@@ -11021,6 +11020,29 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
listen_addr = virXPathString(xpath, ctxt);
VIR_FREE(xpath);
+/* Create our XPATH lookup for the current spice type. */
+if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "listen/@type") < 0)
+goto cleanup;
+
+/* Attempt to get the type of spice connection */
+VIR_FREE(type_conn);
+type_conn = virXPathString(xpath, ctxt);
+VIR_FREE(xpath);
+
+if (STREQ_NULLABLE(type_conn, "socket")) {
+if (!socket) {
+if (virAsprintf(&xpath, xpath_fmt, scheme[iter],
"listen/@socket") < 0)
+goto cleanup;
+
+socket = virXPathString(xpath, ctxt);
+
+VIR_FREE(xpath);
+}
+}
+
+if (!port && !tls_port && !socket)
+continue;
+
if (!listen_addr) {
/* The subelement address - -
* *should* have been automatically backfilled into its
@@ -11035,11 +11057,9 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
listen_addr = virXPathString(xpath, ctxt);
VIR_FREE(xpath);
-}
-
-/* If listen_addr is 0.0.0.0 or [::] we should try to parse URI and set
- * listen_addr based on current URI. */
-if (listen_addr) {
+} else {
+/* If listen_addr is 0.0.0.0 or [::] we should try to parse URI
and set
+ * listen_addr based on current URI. */
if (virSocketAddrParse(&addr, listen_addr, AF_UNSPEC) > 0 &&
virSocketAddrIsWildcard(&addr)) {
@@ -11078,17 +11098,22 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
VIR_FREE(xpath);
/* Build up the full URI, starting with the scheme */
-virBufferAsprintf(&buf, "%s://", scheme[iter]);
+if (socket)
+virBufferAsprintf(&buf, "%s+unix://", scheme[iter]);
+else
+virBufferAsprintf(&buf, "%s://", scheme[iter]);
/* There is no user, so just append password if there's any */
if (STREQ(scheme[iter], "vnc") && passwd)
virBufferAsprintf(&buf, ":%s@", passwd);
/* Then host name or IP */
-if (!listen_addr)
+if (!listen_addr && !socket)
virBufferAddLit(&buf, "localhost");
-else if (strchr(listen_addr, ':'))
+else if (!socket && strchr(listen_addr, ':'))
virBufferAsprintf(&buf, "[%s]", listen_addr);
+else if (STREQ(scheme[iter], "spice"))
+virBufferAsprintf(&buf, "%s", socket);
else
virBufferAsprintf(&buf, "%s", listen_addr);
@@ -11148,6 +11173,8 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
cleanup:
VIR_FREE(xpath);
+VIR_FREE(socket);
+VIR_FREE(type_conn);
VIR_FREE(passwd);
VIR_FREE(listen_addr);
VIR_FREE(output);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] qemu: Add AAVMF32 to the list of known UEFIs
On 07/20/2017 03:56 PM, dann frazier wrote:
> Add a path for UEFI VMs for AArch32 VMs, based on the path Debian is using.
> libvirt is the de facto canonical location for defining where distros
> should place these firmware images, so let's define this path here to try
> and minimize distro fragmentation.
> ---
> src/qemu/qemu.conf | 3 ++-
> src/qemu/qemu_conf.c | 12
>
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.6.0-gicv2-virt.aarch64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.6.0-gicv3-virt.aarch64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml| 1 +
> tests/domaincapsschemadata/qemu_2.6.0.ppc64le.xml| 1 +
> tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.7.0.s390x.xml | 1 +
> tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.8.0.s390x.xml | 1 +
> tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 +
> tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 +
> tests/domaincapstest.c | 1 +
> 17 files changed, 25 insertions(+), 5 deletions(-)
>
FWIW: I did consult with the QEMU UEFI (Laszlo Ersek) who was fine with
the /usr/share/AAVMF/AAVMF32_{CODE,VARS}.fd. He also forwarded along to
Leif Lindholm and Ard Biesheuvel and there were no objections there.
I think what's here does look good - I'd like to give it a couple of
days to "percolate" to ensure no one has heartburn before pushing though
as there are some folks on this list far more involved in the AAVMF than
I am.
John
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index e6c0832662..1d81472df0 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -677,7 +677,8 @@
> #nvram = [
> # "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd",
> # "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd",
> -# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd"
> +# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd",
> +# "/usr/share/AAVMF/AAVMF32_CODE.fd:/usr/share/AAVMF/AAVMF32_VARS.fd"
> #]
>
> # The backend to use for handling stdout/stderr output from
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index a65c92a262..9dd9442d63 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -131,6 +131,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
> #define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
> #define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd"
> #define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd"
> +#define VIR_QEMU_AAVMF32_LOADER_PATH "/usr/share/AAVMF/AAVMF32_CODE.fd"
> +#define VIR_QEMU_AAVMF32_NVRAM_PATH "/usr/share/AAVMF/AAVMF32_VARS.fd"
>
> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> {
> @@ -335,11 +337,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
> privileged)
> goto error;
>
> #else
> -if (VIR_ALLOC_N(cfg->firmwares, 3) < 0)
> +if (VIR_ALLOC_N(cfg->firmwares, 4) < 0)
> goto error;
> -cfg->nfirmwares = 3;
> +cfg->nfirmwares = 4;
> if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 0
> ||
> -VIR_ALLOC(cfg->firmwares[2]) < 0)
> +VIR_ALLOC(cfg->firmwares[2]) < 0 || VIR_ALLOC(cfg->firmwares[3]) < 0)
> goto error;
>
> if (VIR_STRDUP(cfg->firmwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 0
> ||
> @@ -347,7 +349,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
> privileged)
> VIR_STRDUP(cfg->firmwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 ||
> VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0 ||
> VIR_STRDUP(cfg->firmwares[2]->name, VIR_QEMU_OVMF_SEC_LOADER_PATH) <
> 0 ||
> -VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) <
> 0)
> +VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) <
> 0 ||
> +VIR_STRDUP(cfg->firmwares[3]->name, VIR_QEMU_AAVMF32_LOADER_PATH) <
> 0 ||
> +VIR_STRDUP(cfg->firmwares[3]->nvram, VIR_QEMU_AAVMF32_NVRAM_PATH) <
> 0)
> goto error;
> #endif
>
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in
> b/src/qemu/test_libvirtd_qemu.aug.in
> index 3e317bc7e9..676d48cf5c 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -90,6 +90,7 @@ module Test_libvirtd_qemu =
> { "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF
Re: [libvirt] [PATCH v5 2/3] nodedev: Convert virNodeDeviceObjListPtr to use hash tables
On 07/20/2017 10:48 AM, Erik Skultety wrote:
>> @@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
>> };
>>
>> struct _virNodeDeviceObjList {
>> -size_t count;
>> -virNodeDeviceObjPtr *objs;
>> +virObjectLockable parent;
>> +
>> +/* name string -> virNodeDeviceObj mapping
>> + * for O(1), lockless lookup-by-uuid */
>
> I think you meant lookup-by-name ^here. More importantly though, I don't
> understand the comment at all, well I understand the words :), but the context
> is all noise - why should be the lookup lockless? You always lock the objlist
> prior to calling virHashLookup, followed by ref'ing and returning the result,
> I
> know we have that in other conf/vir*obj* modules and those don't make any more
> sense to me either.
>
hrmph... this showed up after I posted v6 I'll provide my comments
here...
Sure I meant "by-name"... The comment was originally sourced from
_virDomainObjList... I've always just copied it around ;-)
The comment in/for domain objs list was added in commit id 'a3adcce79'
when the code was still part of domain_conf.c
I think the "decoding" is that prior to that one would have 0(n) mutex
locks taken for each domain obj in the list as the list was being
traversed. With hash tables one as 0(1) mutex locks taken to lock the
list before get the entry out of the hash table.
>> +virHashTable *objs;
>> +
>> };
>>
>>
>> static virClassPtr virNodeDeviceObjClass;
>> +static virClassPtr virNodeDeviceObjListClass;
>> static void virNodeDeviceObjDispose(void *opaque);
>> +static void virNodeDeviceObjListDispose(void *opaque);
>>
>> static int
>> virNodeDeviceObjOnceInit(void)
>> @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
>>virNodeDeviceObjDispose)))
>> return -1;
>>
>> +if (!(virNodeDeviceObjListClass =
>> virClassNew(virClassForObjectLockable(),
>> + "virNodeDeviceObjList",
>> +
>> sizeof(virNodeDeviceObjList),
>> +
>> virNodeDeviceObjListDispose)))
>> +return -1;
>> +
>> return 0;
>> }
>>
>> @@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj
>> *obj)
>> }
>>
>>
>> +static int
>> +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
>> +const void *name
>> ATTRIBUTE_UNUSED,
>> +const void *opaque)
>> +{
>> +virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
>> +virNodeDeviceDefPtr def;
>
> I recall having discussed ^this with you already, but this is one-time use
> only
> and I simply don't think it's necessary, I'd use helper variables for the sole
> purpose of getting rid of multiple levels of dereference either in a multi-use
> situation, or the number of levels dereferenced makes the line really long and
> the usage unreadable. (this also occurs on multiple places...no need to repeat
> this...)
>
This avoids obj->def->sysfs_path below. While obj->def isn't being made
part of the object by any of these changes, it could well be someday. I
guess it's mostly a personal preference. The compiler will certainly
optimize away my preferences.
>> +const char *sysfs_path = opaque;
>> +int want = 0;
>> +
>> +virObjectLock(obj);
>> +def = obj->def;
>> +if (STREQ_NULLABLE(def->sysfs_path, sysfs_path))
>> +want = 1;
>> +virObjectUnlock(obj);
>> +return want;
>> +}
>> +
>> +
>> virNodeDeviceObjPtr
>> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>> const char *sysfs_path)
>> {
>> -size_t i;
>> +virNodeDeviceObjPtr obj;
>>
>> -for (i = 0; i < devs->count; i++) {
>> -virNodeDeviceObjPtr obj = devs->objs[i];
>> -virNodeDeviceDefPtr def;
>> +virObjectLock(devs);
>> +obj = virHashSearch(devs->objs,
>> virNodeDeviceObjListFindBySysfsPathCallback,
>> +(void *)sysfs_path);
>> +virObjectRef(obj);
>> +virObjectUnlock(devs);
>
> ^This pattern occurs multiple times within the patch, I think it deserves a
> simple helper
>
Oh - I've been down that fork in the road before - create what I think
is a "simple" helper combining things only to get shot down during
review because it was deemed unnecessary or that each of these should
have their own separate pair of API's
Each one of these is a unique way to search the objs list for a piece of
data by some value that is not a key...
FindBySysfsPath
FindByWWNs
FindByFabricWWN
FindByCap
FindSCSIHostByWWNs
What comes to mind to me as a simple helper would be to create a 'typed'
structure and API's to use switch/case in order to make the comparison.
Perhaps some future change could do that.
>>
>> +if (obj)
>> virObjectLock(obj);
>> -def = obj->def;
>> -if ((def->sysf
[libvirt] [PATCH v3 0/2] qemu: Add AAVMF32 to the list of known UEFIs
Changes since v2: * Include updates to qemu.conf * Update tests for parity w/ AAVMF * Drop reference to 'experimental' in commit message Changes since v1: * Add a virt-aa-helper-test case. dann frazier (2): qemu: Add AAVMF32 to the list of known UEFIs virt-aa-helper-test: Add test for aarch32 UEFI image path src/qemu/qemu.conf | 3 ++- src/qemu/qemu_conf.c | 12 src/qemu/test_libvirtd_qemu.aug.in | 1 + tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.6.0-gicv2-virt.aarch64.xml | 1 + tests/domaincapsschemadata/qemu_2.6.0-gicv3-virt.aarch64.xml | 1 + tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml| 1 + tests/domaincapsschemadata/qemu_2.6.0.ppc64le.xml| 1 + tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.7.0.s390x.xml | 1 + tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.8.0.s390x.xml | 1 + tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 + tests/domaincapstest.c | 1 + tests/virt-aa-helper-test| 1 + 18 files changed, 26 insertions(+), 5 deletions(-) -- 2.13.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/2] virt-aa-helper-test: Add test for aarch32 UEFI image path
Signed-off-by: dann frazier --- tests/virt-aa-helper-test | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 51072f622c..0599046408 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -308,6 +308,7 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml" testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd" testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd" testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd" +testfw "AAVMF32" "/usr/share/AAVMF/AAVMF32_CODE.fd" testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml" -- 2.13.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/2] qemu: Add AAVMF32 to the list of known UEFIs
Add a path for UEFI VMs for AArch32 VMs, based on the path Debian is using.
libvirt is the de facto canonical location for defining where distros
should place these firmware images, so let's define this path here to try
and minimize distro fragmentation.
---
src/qemu/qemu.conf | 3 ++-
src/qemu/qemu_conf.c | 12
src/qemu/test_libvirtd_qemu.aug.in | 1 +
tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml | 1 +
tests/domaincapsschemadata/qemu_2.6.0-gicv2-virt.aarch64.xml | 1 +
tests/domaincapsschemadata/qemu_2.6.0-gicv3-virt.aarch64.xml | 1 +
tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml| 1 +
tests/domaincapsschemadata/qemu_2.6.0.ppc64le.xml| 1 +
tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml | 1 +
tests/domaincapsschemadata/qemu_2.7.0.s390x.xml | 1 +
tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml | 1 +
tests/domaincapsschemadata/qemu_2.8.0.s390x.xml | 1 +
tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml | 1 +
tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml | 1 +
tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml | 1 +
tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml | 1 +
tests/domaincapstest.c | 1 +
17 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index e6c0832662..1d81472df0 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -677,7 +677,8 @@
#nvram = [
# "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd",
# "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd",
-# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd"
+# "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd",
+# "/usr/share/AAVMF/AAVMF32_CODE.fd:/usr/share/AAVMF/AAVMF32_VARS.fd"
#]
# The backend to use for handling stdout/stderr output from
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a65c92a262..9dd9442d63 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -131,6 +131,8 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
#define VIR_QEMU_OVMF_SEC_NVRAM_PATH "/usr/share/OVMF/OVMF_VARS.fd"
#define VIR_QEMU_AAVMF_LOADER_PATH "/usr/share/AAVMF/AAVMF_CODE.fd"
#define VIR_QEMU_AAVMF_NVRAM_PATH "/usr/share/AAVMF/AAVMF_VARS.fd"
+#define VIR_QEMU_AAVMF32_LOADER_PATH "/usr/share/AAVMF/AAVMF32_CODE.fd"
+#define VIR_QEMU_AAVMF32_NVRAM_PATH "/usr/share/AAVMF/AAVMF32_VARS.fd"
virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
{
@@ -335,11 +337,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
goto error;
#else
-if (VIR_ALLOC_N(cfg->firmwares, 3) < 0)
+if (VIR_ALLOC_N(cfg->firmwares, 4) < 0)
goto error;
-cfg->nfirmwares = 3;
+cfg->nfirmwares = 4;
if (VIR_ALLOC(cfg->firmwares[0]) < 0 || VIR_ALLOC(cfg->firmwares[1]) < 0 ||
-VIR_ALLOC(cfg->firmwares[2]) < 0)
+VIR_ALLOC(cfg->firmwares[2]) < 0 || VIR_ALLOC(cfg->firmwares[3]) < 0)
goto error;
if (VIR_STRDUP(cfg->firmwares[0]->name, VIR_QEMU_AAVMF_LOADER_PATH) < 0 ||
@@ -347,7 +349,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
VIR_STRDUP(cfg->firmwares[1]->name, VIR_QEMU_OVMF_LOADER_PATH) < 0 ||
VIR_STRDUP(cfg->firmwares[1]->nvram, VIR_QEMU_OVMF_NVRAM_PATH) < 0 ||
VIR_STRDUP(cfg->firmwares[2]->name, VIR_QEMU_OVMF_SEC_LOADER_PATH) < 0
||
-VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) < 0)
+VIR_STRDUP(cfg->firmwares[2]->nvram, VIR_QEMU_OVMF_SEC_NVRAM_PATH) < 0
||
+VIR_STRDUP(cfg->firmwares[3]->name, VIR_QEMU_AAVMF32_LOADER_PATH) < 0
||
+VIR_STRDUP(cfg->firmwares[3]->nvram, VIR_QEMU_AAVMF32_NVRAM_PATH) < 0)
goto error;
#endif
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index 3e317bc7e9..676d48cf5c 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -90,6 +90,7 @@ module Test_libvirtd_qemu =
{ "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" }
{ "2" =
"/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd" }
{ "3" = "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/AAVMF/AAVMF_VARS.fd" }
+{ "4" =
"/usr/share/AAVMF/AAVMF32_CODE.fd:/usr/share/AAVMF/AAVMF32_VARS.fd" }
}
{ "stdio_handler" = "logd" }
{ "gluster_debug_level" = "9" }
diff --git a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
index 1eadba393f..8d1ad86570 100644
--- a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
@@ -7,6 +7,7 @@
/usr/share/AAVMF/AAVMF_CODE.fd
+ /usr/share/AAVMF/AAVMF32_C
[libvirt] [PATCH v3 4/4] storage: Disallow usage of the HBA for a fc_host backing
Disallow providing the wwnn/wwpn of the HBA in the adapter XML:
This should be considered a configuration error since a vHBA
would not be created. In order to use the HBA as the backing the
following XML should be used:
This also alters the caller such that the @parent_name param
into checkParent can be NULL so as to confirm that at least
the provided wwnn/wwpn found a vHBA instead of an HBA.
Signed-off-by: John Ferlan
---
docs/formatstorage.html.in | 27 +--
src/storage/storage_backend_scsi.c | 45 --
2 files changed, 49 insertions(+), 23 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 4946ddf..27578e8 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -207,18 +207,21 @@
wwnn and wwpn
- The "World Wide Node Name" (wwnn) and "World Wide
-Port Name" (wwpn) are used by the "fc_host" adapter
-to uniquely identify the device in the Fibre Channel storage fabric
-(the device can be either a HBA or vHBA). Both wwnn and wwpn should
-be specified. Use the command 'virsh nodedev-dumpxml' to determine
-how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
-wwpn have very specific numerical format requirements based on the
-hypervisor being used, thus care should be taken if you decide to
-generate your own to follow the standards; otherwise, the pool
-will fail to start with an opaque error message indicating failure
-to write to the vport_create file during vport create/delete due
-to "No such file or directory".
+ The required "World Wide Node Name" (wwnn) and
+"World Wide Port Name" (wwpn) are used by the
+"fc_host" adapter to uniquely identify the vHBA device in the
+Fibre Channel storage fabric. If the vHBA device already exists
+as a Node Device, then libvirt will use it; otherwise, the vHBA
+will be created using the provided values. It is considered a
+configuration error use the values from the HBA as those would
+be for a "scsi_host" type pool instead. The
+wwnn and wwpn have very specific
+format requirements based on the hypervisor being used, thus
+care should be taken if you decide to generate your own to
+follow the standards; otherwise, the pool will fail to start
+with an opaque error message indicating failure to write to
+the vport_create file during vport create/delete due to
+"No such file or directory".
Since 1.0.4
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index af12889..c802738 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -231,22 +231,47 @@ checkParent(virConnectPtr conn,
if (!conn)
return true;
-if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
- _("parent '%s' is not properly formatted"),
- parent_name);
+/* If there's a parent_name, then make sure it's valid */
+if (parent_name) {
+if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' is not properly formatted"),
+ parent_name);
+goto cleanup;
+}
+
+if (!virVHBAPathExists(NULL, host_num)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("parent '%s' is not an fc_host for the
wwnn/wwpn"),
+ parent_name);
+goto cleanup;
+}
+}
+
+if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+goto cleanup;
+
+if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("host name '%s' is not properly formatted"),
+ name);
goto cleanup;
}
-if (!virVHBAPathExists(NULL, host_num)) {
+/* If scsi_host_name is vport capable, then it's an HBA. This is
+ * a configuration error as the wwnn/wwpn should only be for a vHBA */
+if (virVHBAIsVportCapable(NULL, host_num)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("parent '%s' is not an fc_host for the wwnn/wwpn"),
- parent_name);
+ _("the wwnn/wwpn for '%s' are assigned to an HBA"),
+ scsi_host_name);
goto cleanup;
}
-if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+/* No parent name, then no need to get/compare against vhba_parent */
+if (!parent_name) {
+
[libvirt] [PATCH v3 3/4] storage: Check if provided parent is vHBA capable
https://bugzilla.redhat.com/show_bug.cgi?id=1458708
If the parent provided for the storage pool adapter is not vHBA
capable, then issue a configuration error even though the provided
wwnn/wwpn were found.
It is a configuration error to provide a mismatched parent to
the wwnn/wwpn. The @parent is optional and is used as a means to
perform duplicate pool source checks.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_scsi.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 359d2d2..af12889 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -220,6 +220,7 @@ checkParent(virConnectPtr conn,
const char *name,
const char *parent_name)
{
+unsigned int host_num;
char *scsi_host_name = NULL;
char *vhba_parent = NULL;
bool retval = false;
@@ -230,6 +231,20 @@ checkParent(virConnectPtr conn,
if (!conn)
return true;
+if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' is not properly formatted"),
+ parent_name);
+goto cleanup;
+}
+
+if (!virVHBAPathExists(NULL, host_num)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("parent '%s' is not an fc_host for the wwnn/wwpn"),
+ parent_name);
+goto cleanup;
+}
+
if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
goto cleanup;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/4] storage: Fix existing parent check for vHBA creation
https://bugzilla.redhat.com/show_bug.cgi?id=1472277
Commit id '106930aaa' altered the order of checking for an existing
vHBA (e.g something created via nodedev-create functionality outside
of the storage pool logic) which inadvertantly broke the code to
decide whether to alter/force the fchost->managed field to be 'yes'
because the storage pool will be managing the created vHBA in order
to ensure when the storage pool is destroyed that the vHBA is also
destroyed.
This patch moves the check (and checkParent helper) for an existing
vHBA back into the createVport in storage_backend_scsi. It also
adjusts the checkParent logic to more closely follow the intentions
prior to commit id '79ab0935'. The changes made by commit id '08c0ea16f'
are only necessary to run the virStoragePoolFCRefreshThread when
a vHBA was really created because there's a timing lag such that
the refreshPool call made after a startPool from storagePoolCreate*
wouldn't necessarily find LUNs, but the thread would. For an already
existing vHBA, using the thread is unnecessary since the vHBA already
exists and the lag to configure the LUNs wouldn't exist.
Signed-off-by: John Ferlan
---
src/conf/node_device_conf.c| 55 --
src/storage/storage_backend_scsi.c | 54 +
2 files changed, 54 insertions(+), 55 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 503b129..9c0ffa5 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2259,48 +2259,6 @@ virNodeDeviceGetParentName(virConnectPtr conn,
}
-/*
- * Using the host# name found via wwnn/wwpn lookup in the fc_host
- * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
- */
-static bool
-checkParent(virConnectPtr conn,
-const char *name,
-const char *parent_name)
-{
-char *scsi_host_name = NULL;
-char *vhba_parent = NULL;
-bool retval = false;
-
-VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
-
-/* autostarted pool - assume we're OK */
-if (!conn)
-return true;
-
-if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
-goto cleanup;
-
-if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
-goto cleanup;
-
-if (STRNEQ(parent_name, vhba_parent)) {
-virReportError(VIR_ERR_XML_ERROR,
- _("Parent attribute '%s' does not match parent '%s' "
- "determined for the '%s' wwnn/wwpn lookup."),
- parent_name, vhba_parent, name);
-goto cleanup;
-}
-
-retval = true;
-
- cleanup:
-VIR_FREE(vhba_parent);
-VIR_FREE(scsi_host_name);
-return retval;
-}
-
-
/**
* @conn: Connection pointer
* @fchost: Pointer to vHBA adapter
@@ -2326,19 +2284,6 @@ virNodeDeviceCreateVport(virConnectPtr conn,
VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'",
conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn);
-/* If we find an existing HBA/vHBA within the fc_host sysfs
- * using the wwnn/wwpn, then a nodedev is already created for
- * this pool and we don't have to create the vHBA
- */
-if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
-/* If a parent was provided, let's make sure the 'name' we've
- * retrieved has the same parent. If not this will cause failure. */
-if (fchost->parent && checkParent(conn, name, fchost->parent))
-VIR_FREE(name);
-
-return name;
-}
-
if (fchost->parent) {
if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
goto cleanup;
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index f7378d3..e6aa643 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -211,6 +211,48 @@ getAdapterName(virStorageAdapterPtr adapter)
}
+/*
+ * Using the host# name found via wwnn/wwpn lookup in the fc_host
+ * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
+ */
+static bool
+checkParent(virConnectPtr conn,
+const char *name,
+const char *parent_name)
+{
+char *scsi_host_name = NULL;
+char *vhba_parent = NULL;
+bool retval = false;
+
+VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
+
+/* autostarted pool - assume we're OK */
+if (!conn)
+return true;
+
+if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+goto cleanup;
+
+if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+goto cleanup;
+
+if (STRNEQ(parent_name, vhba_parent)) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Parent attribute '%s' does not match parent '%s' "
+ "determined for the '%s' wwnn/wwpn lookup."),
+ parent_name, vhba_parent, name);
+
[libvirt] [PATCH v3 2/4] storage: Remove @conn from virNodeDeviceCreateVport
It's no longer needed since the checkParent code moved back to
storage_backend_scsi.c
Signed-off-by: John Ferlan
---
src/conf/node_device_conf.c| 8 +++-
src/conf/node_device_conf.h| 3 +--
src/storage/storage_backend_scsi.c | 2 +-
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 9c0ffa5..82f02fa 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2260,7 +2260,6 @@ virNodeDeviceGetParentName(virConnectPtr conn,
/**
- * @conn: Connection pointer
* @fchost: Pointer to vHBA adapter
*
* Create a vHBA for Storage. This code accomplishes this via searching
@@ -2273,16 +2272,15 @@ virNodeDeviceGetParentName(virConnectPtr conn,
* Returns vHBA name on success, NULL on failure with an error message set
*/
char *
-virNodeDeviceCreateVport(virConnectPtr conn,
- virStorageAdapterFCHostPtr fchost)
+virNodeDeviceCreateVport(virStorageAdapterFCHostPtr fchost)
{
unsigned int parent_host;
char *name = NULL;
char *parent_hoststr = NULL;
bool skip_capable_check = false;
-VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'",
- conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn);
+VIR_DEBUG("parent='%s', wwnn='%s' wwpn='%s'",
+ NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn);
if (fchost->parent) {
if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index d10683d..da56eaf 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -384,8 +384,7 @@ virNodeDeviceGetParentName(virConnectPtr conn,
const char *nodedev_name);
char *
-virNodeDeviceCreateVport(virConnectPtr conn,
- virStorageAdapterFCHostPtr fchost);
+virNodeDeviceCreateVport(virStorageAdapterFCHostPtr fchost);
int
virNodeDeviceDeleteVport(virConnectPtr conn,
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index e6aa643..359d2d2 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -294,7 +294,7 @@ createVport(virConnectPtr conn,
}
}
-if (!(name = virNodeDeviceCreateVport(conn, fchost)))
+if (!(name = virNodeDeviceCreateVport(fchost)))
goto cleanup;
/* Creating our own VPORT didn't leave enough time to find any LUN's,
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/4] Fix a couple issues found w/ vHBA logic
v2: https://www.redhat.com/archives/libvir-list/2017-July/msg00661.html Changes since v2: * Patch 1 - make the requested logic adjustment * Patch 2 - no change * Patch 3 - make this *just* the check that the provided @parent_name is a valid fc_host (whether it's vHBA or HBA). * Patch 4 - Alter the checkParent logic to cause a failure if the passed @name (from the wwnn/wwpn's provided) are for an HBA instead of a vHBA. Make this an invalid configuration. Modify the docs to describe that the provided wwnn/wwpn must be for an existing vHBA, are to be used to create a vHBA, but cannot be from an existing HBA. NB: No change to the Destroy code since it's possible one of these exists and there needs to be a way to shut it down. The failure could turn into a "conversion" of sorts as well by changing the type to 'scsi_host', saving the generated scsi_host name, removing the parent and wwnn & wwpn fields. Tested using my vHBA system with various valid and invalid configurations. John Ferlan (4): storage: Fix existing parent check for vHBA creation storage: Remove @conn from virNodeDeviceCreateVport storage: Check if provided parent is vHBA capable storage: Disallow usage of the HBA for a fc_host backing docs/formatstorage.html.in | 27 ++- src/conf/node_device_conf.c| 63 ++--- src/conf/node_device_conf.h| 3 +- src/storage/storage_backend_scsi.c | 94 +- 4 files changed, 112 insertions(+), 75 deletions(-) -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] storage: Fix existing parent check for vHBA creation
[...] >> >> Because it hasn't really been characterized as a misconfiguration >> previously. I doubt anyone outside of QE has ever done something like >> this as there's no reason to do so. If they want to use the HBA they'd >> use the 'scsi_host' format. >> >> IMO: something with 'fc_host' what uses the HBA wwnn/wwpn is >> misconfigured because it's not a vHBA then, but there's been no attempt >> to prohibit that configuration, hence the current mess. >> >> I'd be perfectly fine with turning this particular bz/check into - don't >> configure things this way because it's not how it's supposed to work. If >> you want a storage pool backed to an HBA, then use the scsi_host syntax. >> If you want a vHBA/NPIV then use the fc_host syntax. > > After re-reading the docs, I'm quite convinced we should enforce the > configuration, hopefully it would clean up the code significantly. > > Erik > Not sure how much gets "cleaned up" as only the Create/Destroy storage pool code paths need to know. While I agree in principle that it's incorrect configuration, it has been allowed and removing something that was allowed at one time gets into a gray area. For the destroy path, we'd still have to be able to handle both since a storage pool could exist prior to disallowing the create path to work and we'd have no way to destroy the pool. For the create path, it's an adjustment "post" in patch3 to add checks for whether the parent or the parent found by wwnn/wwpn is an HBA, then disallow the startup. In any case, I'll post a new series with patch1 adjust as requested and patch 3 split to handle the two things - the first being checking the parent_name validity and the second to ensure the @name is not an HBA John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] docs: fix documentation of enum constants
The documentation string has to follow the definition of a constant in
the enum. Otherwise, the HTML documentation will be generated
incorrectly.
Signed-off-by: Tomáš Golembiovský
---
include/libvirt/libvirt-domain.h | 62
1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 45f939a8c..2f3162d0f 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct
*virDomainInterfaceStatsPtr;
* Memory Statistics Tags:
*/
typedef enum {
-/* The total amount of data read from swap space (in kB). */
VIR_DOMAIN_MEMORY_STAT_SWAP_IN = 0,
-/* The total amount of memory written out to swap space (in kB). */
+/* The total amount of data read from swap space (in kB). */
VIR_DOMAIN_MEMORY_STAT_SWAP_OUT= 1,
+/* The total amount of memory written out to swap space (in kB). */
+VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT = 2,
+VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT = 3,
/*
* Page faults occur when a process makes a valid access to virtual memory
* that is not available. When servicing the page fault, if disk IO is
* required, it is considered a major fault. If not, it is a minor fault.
* These are expressed as the number of faults that have occurred.
*/
-VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT = 2,
-VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT = 3,
+VIR_DOMAIN_MEMORY_STAT_UNUSED = 4,
/*
* The amount of memory left completely unused by the system. Memory that
* is available but used for reclaimable caches should NOT be reported as
* free. This value is expressed in kB.
*/
-VIR_DOMAIN_MEMORY_STAT_UNUSED = 4,
+VIR_DOMAIN_MEMORY_STAT_AVAILABLE = 5,
/*
* The total amount of usable memory as seen by the domain. This value
* may be less than the amount of memory assigned to the domain if a
* balloon driver is in use or if the guest OS does not initialize all
* assigned pages. This value is expressed in kB.
*/
-VIR_DOMAIN_MEMORY_STAT_AVAILABLE = 5,
-/* Current balloon value (in KB). */
VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON = 6,
+/* Current balloon value (in KB). */
+VIR_DOMAIN_MEMORY_STAT_RSS = 7,
/* Resident Set Size of the process running the domain. This value
* is in kB */
-VIR_DOMAIN_MEMORY_STAT_RSS = 7,
+VIR_DOMAIN_MEMORY_STAT_USABLE = 8,
/*
* How much the balloon can be inflated without pushing the guest system
* to swap, corresponds to 'Available' in /proc/meminfo
*/
-VIR_DOMAIN_MEMORY_STAT_USABLE = 8,
-/* Timestamp of the last update of statistics, in seconds. */
VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE = 9,
+/* Timestamp of the last update of statistics, in seconds. */
+VIR_DOMAIN_MEMORY_STAT_NR = 10,
/*
* The number of statistics supported by this version of the interface.
* To add new statistics, add them to the enum and increase this value.
*/
-VIR_DOMAIN_MEMORY_STAT_NR = 10,
# ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR
@@ -683,22 +683,23 @@ typedef enum {
/* Domain migration flags. */
typedef enum {
+VIR_MIGRATE_LIVE = (1 << 0),
/* Do not pause the domain during migration. The domain's memory will
* be transferred to the destination host while the domain is running.
* The migration may never converge if the domain is changing its memory
* faster then it can be transferred. The domain can be manually paused
* anytime during migration using virDomainSuspend.
*/
-VIR_MIGRATE_LIVE = (1 << 0),
+VIR_MIGRATE_PEER2PEER = (1 << 1),
/* Tell the source libvirtd to connect directly to the destination host.
* Without this flag the client (e.g., virsh) connects to both hosts and
* controls the migration process. In peer-to-peer mode, the source
* libvirtd controls the migration by calling the destination daemon
* directly.
*/
-VIR_MIGRATE_PEER2PEER = (1 << 1),
+VIR_MIGRATE_TUNNELLED = (1 << 2),
/* Tunnel migration data over libvirtd connection. Without this flag the
* source hypervisor sends migration data directly to the destination
* hypervisor. This flag can only be used when VIR_MIGRATE_PEER2PEER is
@@ -707,26 +708,26 @@ typedef enum {
* Note the less-common spelling that we're stuck with:
* VIR_MIGRATE_TUNNELLED should be VIR_MIGRATE_TUNNELED.
*/
-VIR_MIGRATE_TUNNELLED = (1 << 2),
+VIR_MIGRATE_PERSIST_DEST = (1 << 3),
/* Define the domain as persistent on the destination host after suc
Re: [libvirt] [PATCH v2 3/3] qemu: Enable NUMA node tag in pci-root for PPC64
On Mon, 2017-07-17 at 21:29 +0530, Shivaprasad G Bhat wrote: > @@ -3786,6 +3786,11 @@ > part of the specified NUMA node (it is up to the user of the > libvirt API to attach host devices to the correct > pci-expander-bus when assigning them to the domain). > +On PPC64, the PCI devices can be specified to be part of a NUMA > +node using only the pci-root controller with an optional > +subelement within the > + subelement. The PCI devices on the > +given pci-root controller will be part of the specified NUMA node. Instead of adding an entire new sentence here, it would make more sense to rephrase what's already present, something like: Some PCI controllers (pci-expander-bus for the pc machine type, pcie-expander-bus for the q35 machine type and pci-root for the pseries machine type) can have an optional subelement [...] > @@ -9457,6 +9457,12 @@ virDomainControllerDefParseXML(xmlNodePtr node, > goto error; > } > } > +if (def->idx == 0 && numaNode >= 0) { > +virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Only the PCI controller with index != 0 can " > + "have NUMA node property specified")); > +goto error; > +} > if (numaNode >= 0) > def->opts.pciopts.numaNode = numaNode; The check you're adding can be merged with the one below it. The error message should also be reworded, something like: The PCI controller with index=0 can't be associated with a NUMA node. > @@ -3458,9 +3458,14 @@ > qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, > * that NUMA node is configured in the guest > * array. NUMA cell id's in this array are numbered > * from 0 .. size-1. > + * > + * On PSeries, the NUMA node is set at the PHB. As above, reworking the existing comment would work better than merely appending to it. > */ > -if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS || > - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS) && > +if (((qemuDomainIsPSeries(def) && > + cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && > + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) || > + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS || > + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS)) > && > (int) virDomainNumaGetNodeCount(def->numa) > <= cont->opts.pciopts.numaNode) { I actually don't see any point in the condition being this complex: it could just be if (cont->opts.pciopts.numaNode >= 0 && cont->opts.pciopts.numaNode >= (int) virDomainNumaGetNodeCount(def->numa)) because we've already made sure, while parsing, that numaNode is only set for controllers that allow it. > +++ b/tests/qemuxml2argvdata/qemuxml2argv-spapr-pci-host-bridge-numa-node.xml > @@ -0,0 +1,54 @@ > + > + QEMUGuest1 > + 87eedafe-eedc-4336-8130-ed9fe5dc90c8 > + 2097152 > + 2048 You don't need > + 8 > + > + > + > + > + > + > + > + > + > + > + > +hvm > + is unnecessary. > + > + > + destroy > + restart > + destroy and can be removed. > + > +/usr/bin/qemu-system-ppc64 > + > + > + > + > + > + is irrelevant for the test case at hand. > + The model should be 'none'. > + The SCSI controller is also not useful here. > + > + > + > + > + > +1 > + > + > + > + > + > + > + > +0 > + > + > + > + You can omit . > +++ b/tests/qemuxml2argvtest.c > @@ -2739,6 +2739,9 @@ mymain(void) > DO_TEST_PARSE_ERROR("cpu-cache-emulate-l2", QEMU_CAPS_KVM); > DO_TEST_PARSE_ERROR("cpu-cache-passthrough3", QEMU_CAPS_KVM); > DO_TEST_PARSE_ERROR("cpu-cache-passthrough-l3", QEMU_CAPS_KVM); > +DO_TEST("spapr-pci-host-bridge-numa-node", QEMU_CAPS_NUMA, > +QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, > +QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE); This should be moved up to be close to the pserie-phb* test cases, and renamed to something like 'pseries-phb-numa-node'. There should be one capability per line. You also need to have an identical test case in qemuxml2xml, and at least one negative test to show that trying to assign the default PHB to a NUMA node will result in failure. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/12] qemu: support chardev for all machvirt config
On Thu, Jul 20, 2017 at 4:27 PM, Andrea Bolognani wrote: > On Thu, 2017-07-20 at 13:17 +0200, Christoffer Dall wrote: >> > Can't think of anything specific, but when I backported >> > the fixes to libvirt 3.2.0 there were quite a few conflicts >> > to take care of, so if Linaro is using a much older libvirt >> > version I can imagine the backport would get even trickier. >> >> Is the 3.2.0 backport available somewhere public? > > I'm afraid that's not yet the case. I see you're using 3.4.0 > as your base version, so it wouldn't be of much help anyway. > > I took a quick peek at the git tree pointed to in the bug > report and it looks like the backport was created not through > cherry-picking of the relevant upstream commits, but by > applying the patches straight off the mailing list. > > That is *not* the correct way to perform a backport, for a > number or reasons: > > * you lose metadata such as R-b tags; > > * you don't have any information linking the downstream > commit to the respective upstream commit, which will > make it more difficult to figure out which commits can > be dropped during the next rebase; > > * most importantly, any issue pointed out during review > that has a straightforward enough fix not to warrant > a respin will *still be present* in the backport. > Sure, in Riku's defense, I think the backport was done before the patches were committed upstream. But we'll give it another go. Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/3] nodedev: Convert virNodeDeviceObjListPtr to use hash tables
> @@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
> };
>
> struct _virNodeDeviceObjList {
> -size_t count;
> -virNodeDeviceObjPtr *objs;
> +virObjectLockable parent;
> +
> +/* name string -> virNodeDeviceObj mapping
> + * for O(1), lockless lookup-by-uuid */
I think you meant lookup-by-name ^here. More importantly though, I don't
understand the comment at all, well I understand the words :), but the context
is all noise - why should be the lookup lockless? You always lock the objlist
prior to calling virHashLookup, followed by ref'ing and returning the result, I
know we have that in other conf/vir*obj* modules and those don't make any more
sense to me either.
> +virHashTable *objs;
> +
> };
>
>
> static virClassPtr virNodeDeviceObjClass;
> +static virClassPtr virNodeDeviceObjListClass;
> static void virNodeDeviceObjDispose(void *opaque);
> +static void virNodeDeviceObjListDispose(void *opaque);
>
> static int
> virNodeDeviceObjOnceInit(void)
> @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
>virNodeDeviceObjDispose)))
> return -1;
>
> +if (!(virNodeDeviceObjListClass =
> virClassNew(virClassForObjectLockable(),
> + "virNodeDeviceObjList",
> +
> sizeof(virNodeDeviceObjList),
> +
> virNodeDeviceObjListDispose)))
> +return -1;
> +
> return 0;
> }
>
> @@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj
> *obj)
> }
>
>
> +static int
> +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
> +const void *name
> ATTRIBUTE_UNUSED,
> +const void *opaque)
> +{
> +virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +virNodeDeviceDefPtr def;
I recall having discussed ^this with you already, but this is one-time use only
and I simply don't think it's necessary, I'd use helper variables for the sole
purpose of getting rid of multiple levels of dereference either in a multi-use
situation, or the number of levels dereferenced makes the line really long and
the usage unreadable. (this also occurs on multiple places...no need to repeat
this...)
> +const char *sysfs_path = opaque;
> +int want = 0;
> +
> +virObjectLock(obj);
> +def = obj->def;
> +if (STREQ_NULLABLE(def->sysfs_path, sysfs_path))
> +want = 1;
> +virObjectUnlock(obj);
> +return want;
> +}
> +
> +
> virNodeDeviceObjPtr
> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
> const char *sysfs_path)
> {
> -size_t i;
> +virNodeDeviceObjPtr obj;
>
> -for (i = 0; i < devs->count; i++) {
> -virNodeDeviceObjPtr obj = devs->objs[i];
> -virNodeDeviceDefPtr def;
> +virObjectLock(devs);
> +obj = virHashSearch(devs->objs,
> virNodeDeviceObjListFindBySysfsPathCallback,
> +(void *)sysfs_path);
> +virObjectRef(obj);
> +virObjectUnlock(devs);
^This pattern occurs multiple times within the patch, I think it deserves a
simple helper
>
> +if (obj)
> virObjectLock(obj);
> -def = obj->def;
> -if ((def->sysfs_path != NULL) &&
> -(STREQ(def->sysfs_path, sysfs_path))) {
> -return virObjectRef(obj);
> -}
> -virObjectUnlock(obj);
> -}
>
> -return NULL;
> +return obj;
> +}
> +
> +
> +static virNodeDeviceObjPtr
> +virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs,
> + const char *name)
> +{
> +return virObjectRef(virHashLookup(devs->objs, name));
> }
>
>
> @@ -238,20 +274,42 @@ virNodeDeviceObjPtr
> virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
> const char *name)
> {
> -size_t i;
> -
> -for (i = 0; i < devs->count; i++) {
> -virNodeDeviceObjPtr obj = devs->objs[i];
> -virNodeDeviceDefPtr def;
> +virNodeDeviceObjPtr obj;
>
> +virObjectLock(devs);
> +obj = virNodeDeviceObjListFindByNameLocked(devs, name);
> +virObjectUnlock(devs);
> +if (obj)
> virObjectLock(obj);
> -def = obj->def;
> -if (STREQ(def->name, name))
> -return virObjectRef(obj);
> -virObjectUnlock(obj);
> -}
>
> -return NULL;
> +return obj;
> +}
> +
> +
> +struct virNodeDeviceObjListFindByWWNsData {
> +const char *parent_wwnn;
> +const char *parent_wwpn;
> +};
> +
> +static int
> +virNodeDeviceObjListFindByWWNsCallback(const void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + const void *opaque)
> +{
> +virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +struct virNodeDeviceObjListFindByWWNs
Re: [libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir
On Wed, Jul 19, 2017 at 04:02:59PM -0400, John Ferlan wrote:
On 07/19/2017 09:56 AM, Ján Tomko wrote:
On Thu, Jun 29, 2017 at 10:32:35AM -0400, John Ferlan wrote:
#
# ca-cert.pem - the CA master certificate
# server-cert.pem - the server certificate signed with ca-cert.pem
@@ -13,6 +13,12 @@
#
# dh-params.pem - the DH params configuration file
#
+# If the directory does not exist or does not contain the necessary
files,
+# QEMU domains will fail to start if they are configured to use TLS.
+#
Isn't this sufficiently covered by 'Use of TLS requires that x509
certificates be issued'?
One would think/assume, but then again the point of the bz was about the
comments being too vague, so I've taken the opposite approach to be
somewhat overly verbose.
The bz was about a comment not matching the behavior.
+# In order to overwrite the default path alter the following. If the
provided
+# path does not exist, then startup will fail.
+#
To apply the configuration, you need to restart the daemon. And since
daemon startup will fail, I think the user will be able to notice it.
We should error out on incorrect paths as soon as we can, without
mentioning it in the documentation.
Again, since the bz indicated it wasn't clear, I was trying be overly
obvious. Sometimes being overly succinct in documentation has advantages
with respect to setting expectations.
How about if I change them to :
# In order to overwrite the default path alter the following. This path
# definition will be used as the default path for other *_tls_x509_cert_dir
# configuration settings if they are not specifically set.
Looks good.
(and assuming the changes descibed in [1] below)
#default_tls_x509_cert_dir = "/etc/pki/qemu"
@@ -79,8 +85,9 @@
# In order to override the default TLS certificate location for
# vnc certificates, supply a valid path to the certificate directory.
-# If the provided path does not exist then the default_tls_x509_cert_dir
-# path will be used.
+# If the default listed here does not exist, then the default
/etc/pki/qemu
+# is used.
If I override default_tls_x509_cert_dir, without overriding all the
specific *_tls_x509_cert_dir values, I expect they will all use my
value, not the hardcoded default of /etc/pki/qemu.
So the behavior described by the original comment makes more sense.
But that doesn't reflect the actuality of the code. So are you expecting
the code to change too?
Yes.
During virQEMUDriverConfigNew (SET_TLS_X509_CERT_DEFAULT), if the
"/etc/pki/libvirt-*" doesn't exist (where * would be vnc, spice,
chardev, migrate), then by default it is set to /etc/pki/qemu.
If someone then later changes default_tls_x509_cert_dir, then that
directory is used instead of the default /etc/pki/qemu. If the other
settings remain commented out, then their defaults are /etc/pki/qemu.
That being said - the virQEMUDriverConfigSetCertDir could change to add
code that could check if "default" was set to something other than the
default, then copy in "default" (and assume it was already checked) [1].
If uncommented and the provided path does not exist, then startup
+# will fail.
#
#vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
Strange snipping seems to have missed a [...] since the patch definitely
had more here, but they're all the same...
I will work on destrangifying my snipping.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 73c33d6..4eb6f0c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr
hugetlbfs,
}
+static int
+virQEMUDriverConfigSetCertDir(virConfPtr conf,
+ const char *setting,
+ char **value)
+{
+char *tlsCertDir = NULL;
+
+if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
+return -1;
+
+if (!tlsCertDir)
[1] (from above comments)...
If the entry was commented out, then if cfg->defaultTLSx509certdir is
not /etc/pki/qemu (the default), then STRDUP into *value:
if (!tlsCertDir) {
if (STRNEQ_NULLABLE(defaultTLS, *value)) {
if (VIR_STRDUP(*value, defaultTLS) < 0)
return -1
}
return 0;
If the default values should be propagated, it would be simpler
to let the parser only fill the specified paths first and then
fill out the defaults. That way all the auto-magic logic would
be in one place.
}
where defaultTLS is either cfg->defaultTLSx509certdir or NULL for
default. Since for the default case, the STRDUP isn't necessary;
however, for others (vnc, spice, chardev, and migrate) the *value would
be set to whatever cfg->defaultTLSx509certdir is.
If this happens, then the keeping the original comment is fine and the
bz would change it's "expected results" perhaps although it isn't clear
because it's not "default" that's changing.
+return 0;
+
+if (!virFileExists(tlsCertDir)) {
+virReportError(VIR_ERR_CONF_SYNTAX,
This is not
Re: [libvirt] [PATCH 00/12] qemu: support chardev for all machvirt config
On Thu, 2017-07-20 at 13:17 +0200, Christoffer Dall wrote: > > Can't think of anything specific, but when I backported > > the fixes to libvirt 3.2.0 there were quite a few conflicts > > to take care of, so if Linaro is using a much older libvirt > > version I can imagine the backport would get even trickier. > > Is the 3.2.0 backport available somewhere public? I'm afraid that's not yet the case. I see you're using 3.4.0 as your base version, so it wouldn't be of much help anyway. I took a quick peek at the git tree pointed to in the bug report and it looks like the backport was created not through cherry-picking of the relevant upstream commits, but by applying the patches straight off the mailing list. That is *not* the correct way to perform a backport, for a number or reasons: * you lose metadata such as R-b tags; * you don't have any information linking the downstream commit to the respective upstream commit, which will make it more difficult to figure out which commits can be dropped during the next rebase; * most importantly, any issue pointed out during review that has a straightforward enough fix not to warrant a respin will *still be present* in the backport. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: domain: Store and restore autoCpuset to status XML
On Thu, 2017-07-20 at 15:46 +0200, Peter Krempa wrote: > > > -virBufferAsprintf(buf, "\n", nodeset); > > > +if (priv->autoCpuset && > > > +!((cpuset = virBitmapFormat(priv->autoCpuset > > > +goto cleanup; > > > > The previous implementation didn't format the > > element at all if there was nodeset, whereas the new one > > will always format it. You could add > > > > if (!nodeset && !cpuset) > > goto cleanup; > > If you call virBitmapFormat on an empty or NULL bitmap you still get a > (empty) string allocated so that condition is basically identical to the > one that's already there due to how the bitmaps are formatted: > > if (!priv->autoNodeset && !priv->autoCpuset) > return 0; > > if (priv->autoNodeset && > !((nodeset = virBitmapFormat(priv->autoNodeset > goto cleanup; > > if (priv->autoCpuset && > !((cpuset = virBitmapFormat(priv->autoCpuset > goto cleanup; Oh, you're right. Nevermind then. > > > -if (!nodeset) > > > +if (!nodeset && !cpuset) > > > return 0; > > > > I don't think you want this hunk. > > > > With the new condition, you will perform an early exit only > > if both nodeset and cpuset are NULL; if nodeset is NULL but > > cpuset isn't, the first call to virBitmapParse() a few lines > > below will error out. > > That shouldn't ever happen (tm) :D > > I'll can add a condition that if nodeset is not in the XML the parsing > will be skipped. So in that case only cpuset can be present (for future > use). > > I'll also add a note that accessing autoNodeset in the else branch of if > (cpuset) is safe. Works for me :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v6 4/4] nodedev: Remove driver locks around object list mgmt code
Since virnodedeviceobj now has a self-lockable hash table, there's no
need to lock the table from the driver for processing. Thus remove the
locks from the driver for NodeDeviceObjList mgmt.
This includes the test driver as well.
Signed-off-by: John Ferlan
---
src/node_device/node_device_driver.c | 61 +++-
src/node_device/node_device_hal.c| 16 ++
src/node_device/node_device_udev.c | 13 +++-
src/test/test_driver.c | 25 +++
4 files changed, 22 insertions(+), 93 deletions(-)
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index 920d877..facfeb6 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -174,19 +174,13 @@ nodeNumOfDevices(virConnectPtr conn,
const char *cap,
unsigned int flags)
{
-int ndevs = 0;
-
if (virNodeNumOfDevicesEnsureACL(conn) < 0)
return -1;
virCheckFlags(0, -1);
-nodeDeviceLock();
-ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap,
- virNodeNumOfDevicesCheckACL);
-nodeDeviceUnlock();
-
-return ndevs;
+return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap,
+virNodeNumOfDevicesCheckACL);
}
@@ -197,20 +191,14 @@ nodeListDevices(virConnectPtr conn,
int maxnames,
unsigned int flags)
{
-int nnames;
-
if (virNodeListDevicesEnsureACL(conn) < 0)
return -1;
virCheckFlags(0, -1);
-nodeDeviceLock();
-nnames = virNodeDeviceObjListGetNames(driver->devs, conn,
- virNodeListDevicesCheckACL,
- cap, names, maxnames);
-nodeDeviceUnlock();
-
-return nnames;
+return virNodeDeviceObjListGetNames(driver->devs, conn,
+virNodeListDevicesCheckACL,
+cap, names, maxnames);
}
@@ -219,19 +207,14 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
virNodeDevicePtr **devices,
unsigned int flags)
{
-int ret = -1;
-
virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1);
if (virConnectListAllNodeDevicesEnsureACL(conn) < 0)
return -1;
-nodeDeviceLock();
-ret = virNodeDeviceObjListExport(conn, driver->devs, devices,
- virConnectListAllNodeDevicesCheckACL,
- flags);
-nodeDeviceUnlock();
-return ret;
+return virNodeDeviceObjListExport(conn, driver->devs, devices,
+ virConnectListAllNodeDevicesCheckACL,
+ flags);
}
@@ -240,11 +223,7 @@ nodeDeviceObjFindByName(const char *name)
{
virNodeDeviceObjPtr obj;
-nodeDeviceLock();
-obj = virNodeDeviceObjListFindByName(driver->devs, name);
-nodeDeviceUnlock();
-
-if (!obj) {
+if (!(obj = virNodeDeviceObjListFindByName(driver->devs, name))) {
virReportError(VIR_ERR_NO_NODE_DEVICE,
_("no node device with matching name '%s'"),
name);
@@ -294,11 +273,8 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
virCheckFlags(0, NULL);
-nodeDeviceLock();
-obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn);
-nodeDeviceUnlock();
-
-if (!obj)
+if (!(obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs,
+ wwnn, wwpn)))
return NULL;
def = virNodeDeviceObjGetDef(obj);
@@ -509,13 +485,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
virNodeDevicePtr device = NULL;
time_t start = 0, now = 0;
-/* The thread that creates the device takes the driver lock, so we
- * must release it in order to allow the device to be created.
- * We're not doing anything with the driver pointer at this point,
- * so it's safe to release it, assuming that the pointer itself
- * doesn't become invalid. */
-nodeDeviceUnlock();
-
nodeDeviceGetTime(&start);
while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) {
@@ -532,8 +501,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn,
break;
}
-nodeDeviceLock();
-
return device;
}
@@ -552,8 +519,6 @@ nodeDeviceCreateXML(virConnectPtr conn,
virCheckFlags(0, NULL);
virt_type = virConnectGetType(conn);
-nodeDeviceLock();
-
if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE,
virt_type)))
goto cleanup;
@@ -579,7 +544,6 @@ nodeDeviceCreateXML(virConnectPtr conn,
"wwnn '%s' and wwpn '%s'"),
def->name, wwnn, wwpn);
cleanup:
-nodeDeviceUnlock();
[libvirt] [PATCH v6 1/4] nodedev: Alter node device deletion logic
Alter the node device deletion logic to make use of the parent field
from the obj->def rather than call virNodeDeviceObjListGetParentHost.
As it turns out the saved @def won't have parent_wwnn/wwpn or
parent_fabric_wwn, so the only logical path would be to call
virNodeDeviceObjListGetParentHostByParent which we can accomplish
directly via virNodeDeviceObjListFindByName.
Signed-off-by: John Ferlan
---
src/node_device/node_device_driver.c | 26 +++---
src/test/test_driver.c | 26 +-
2 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index 0a19908..f56ff34 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -594,8 +594,9 @@ nodeDeviceDestroy(virNodeDevicePtr device)
int ret = -1;
virNodeDeviceObjPtr obj = NULL;
virNodeDeviceDefPtr def;
+char *parent = NULL;
char *wwnn = NULL, *wwpn = NULL;
-int parent_host = -1;
+unsigned int parent_host;
if (!(obj = nodeDeviceObjFindByName(device->name)))
return -1;
@@ -609,13 +610,23 @@ nodeDeviceDestroy(virNodeDevicePtr device)
if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0)
goto cleanup;
-/* virNodeDeviceGetParentHost will cause the device object's lock
- * to be taken, so grab the object def which will have the various
- * fields used to search (name, parent, parent_wwnn, parent_wwpn,
- * or parent_fabric_wwn) and drop the object lock. */
+/* Because we're about to release the lock and thus run into a race
+ * possibility (however improbable) with a udevAddOneDevice change
+ * event which would essentially free the existing @def (obj->def) and
+ * replace it with something new, we need to grab the parent field
+ * and then find the parent obj in order to manage the vport */
+if (VIR_STRDUP(parent, def->parent) < 0)
+goto cleanup;
+
virNodeDeviceObjEndAPI(&obj);
-if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def,
- EXISTING_DEVICE)) < 0)
+
+if (!(obj = virNodeDeviceObjListFindByName(driver->devs, parent))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot find parent '%s' definition"), parent);
+goto cleanup;
+}
+
+if (virSCSIHostGetNumber(parent, &parent_host) < 0)
goto cleanup;
if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0)
@@ -626,6 +637,7 @@ nodeDeviceDestroy(virNodeDevicePtr device)
cleanup:
nodeDeviceUnlock();
virNodeDeviceObjEndAPI(&obj);
+VIR_FREE(parent);
VIR_FREE(wwnn);
VIR_FREE(wwpn);
return ret;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 83ab9cc..076b17a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5618,8 +5618,9 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
int ret = 0;
testDriverPtr driver = dev->conn->privateData;
virNodeDeviceObjPtr obj = NULL;
+virNodeDeviceObjPtr parentobj = NULL;
virNodeDeviceDefPtr def;
-char *parent_name = NULL, *wwnn = NULL, *wwpn = NULL;
+char *wwnn = NULL, *wwpn = NULL;
virObjectEventPtr event = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
@@ -5629,22 +5630,22 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1)
goto cleanup;
-if (VIR_STRDUP(parent_name, def->parent) < 0)
-goto cleanup;
-
-/* virNodeDeviceGetParentHost will cause the device object's lock to be
- * taken, so we have to dup the parent's name and drop the lock
- * before calling it. We don't need the reference to the object
- * any more once we have the parent's name. */
+/* Unlike the real code we cannot run into the udevAddOneDevice race
+ * which would replace obj->def, so no need to save off the parent,
+ * but do need to drop the @obj lock so that the FindByName code doesn't
+ * deadlock on ourselves */
virObjectUnlock(obj);
-/* We do this just for basic validation, but also avoid finding a
- * vport capable HBA if for some reason our vHBA doesn't exist */
-if (virNodeDeviceObjListGetParentHost(driver->devs, def,
- EXISTING_DEVICE) < 0) {
+/* We do this just for basic validation and throw away the parentobj
+ * since there's no vport_delete to be run */
+if (!(parentobj = virNodeDeviceObjListFindByName(driver->devs,
+ def->parent))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot find parent '%s' definition"), def->parent);
virObjectLock(obj);
goto cleanup;
}
+virNodeDeviceObjEndAPI(&parentobj);
event = virNodeDeviceEventLifecycleNew(dev->name,
[libvirt] [PATCH v6 2/4] nodedev: Remove @create from virNodeDeviceObjListGetParentHost
The only callers to this function are from CreateXML paths now, so
let's just remove the unnecessary parameter.
Signed-off-by: John Ferlan
---
src/conf/virnodedeviceobj.c | 5 ++---
src/conf/virnodedeviceobj.h | 3 +--
src/node_device/node_device_driver.c | 3 +--
src/test/test_driver.c | 2 +-
4 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 4c5ee8c..035a56d 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -545,8 +545,7 @@
virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs)
int
virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs,
- virNodeDeviceDefPtr def,
- int create)
+ virNodeDeviceDefPtr def)
{
int parent_host = -1;
@@ -561,7 +560,7 @@ virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr
devs,
parent_host =
virNodeDeviceObjListGetParentHostByFabricWWN(devs, def->name,
def->parent_fabric_wwn);
-} else if (create == CREATE_DEVICE) {
+} else {
/* Try to find a vport capable scsi_host when no parent supplied */
parent_host = virNodeDeviceObjListFindVportParentHost(devs);
}
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 788fb66..06f2e9e 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -75,8 +75,7 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
int
virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs,
- virNodeDeviceDefPtr def,
- int create);
+ virNodeDeviceDefPtr def);
virNodeDeviceObjListPtr
virNodeDeviceObjListNew(void);
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index f56ff34..920d877 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -563,8 +563,7 @@ nodeDeviceCreateXML(virConnectPtr conn,
if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1)
goto cleanup;
-if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def,
- CREATE_DEVICE)) < 0)
+if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def)) <
0)
goto cleanup;
if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 076b17a..bb2e7ba 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5580,7 +5580,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
/* Unlike the "real" code we don't need the parent_host in order to
* call virVHBAManageVport, but still let's make sure the code finds
* something valid and no one messed up the mock environment. */
-if (virNodeDeviceObjListGetParentHost(driver->devs, def, CREATE_DEVICE) <
0)
+if (virNodeDeviceObjListGetParentHost(driver->devs, def) < 0)
goto cleanup;
/* In the real code, we'd call virVHBAManageVport followed by
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v6 3/4] nodedev: Convert virNodeDeviceObjListPtr to use hash tables
Rather than use a forward linked list of elements, it'll be much more
efficient to use a hash table to reference the elements by unique name
and to perform hash searches.
This patch does all the heavy lifting of converting the list object to
use a self locking list that contains the hash table. Each of the FindBy
functions that do not involve finding the object by it's key (name) is
converted to use virHashSearch in order to find the specific object.
When searching for the key (name), it's possible to use virHashLookup.
For any of the list perusal functions that are required to evaluate
each object, the virHashForEach function is used.
Signed-off-by: John Ferlan
---
src/conf/virnodedeviceobj.c | 575 ++--
1 file changed, 400 insertions(+), 175 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 035a56d..58481e7 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -25,6 +25,7 @@
#include "viralloc.h"
#include "virnodedeviceobj.h"
#include "virerror.h"
+#include "virhash.h"
#include "virlog.h"
#include "virstring.h"
@@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
};
struct _virNodeDeviceObjList {
-size_t count;
-virNodeDeviceObjPtr *objs;
+virObjectLockable parent;
+
+/* name string -> virNodeDeviceObj mapping
+ * for O(1), lockless lookup-by-uuid */
+virHashTable *objs;
+
};
static virClassPtr virNodeDeviceObjClass;
+static virClassPtr virNodeDeviceObjListClass;
static void virNodeDeviceObjDispose(void *opaque);
+static void virNodeDeviceObjListDispose(void *opaque);
static int
virNodeDeviceObjOnceInit(void)
@@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
virNodeDeviceObjDispose)))
return -1;
+if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(),
+ "virNodeDeviceObjList",
+ sizeof(virNodeDeviceObjList),
+
virNodeDeviceObjListDispose)))
+return -1;
+
return 0;
}
@@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
}
+static int
+virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
+const void *name ATTRIBUTE_UNUSED,
+const void *opaque)
+{
+virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+virNodeDeviceDefPtr def;
+const char *sysfs_path = opaque;
+int want = 0;
+
+virObjectLock(obj);
+def = obj->def;
+if (STREQ_NULLABLE(def->sysfs_path, sysfs_path))
+want = 1;
+virObjectUnlock(obj);
+return want;
+}
+
+
virNodeDeviceObjPtr
virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
const char *sysfs_path)
{
-size_t i;
+virNodeDeviceObjPtr obj;
-for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjPtr obj = devs->objs[i];
-virNodeDeviceDefPtr def;
+virObjectLock(devs);
+obj = virHashSearch(devs->objs,
virNodeDeviceObjListFindBySysfsPathCallback,
+(void *)sysfs_path, NULL);
+virObjectRef(obj);
+virObjectUnlock(devs);
+if (obj)
virObjectLock(obj);
-def = obj->def;
-if ((def->sysfs_path != NULL) &&
-(STREQ(def->sysfs_path, sysfs_path))) {
-return virObjectRef(obj);
-}
-virObjectUnlock(obj);
-}
-return NULL;
+return obj;
+}
+
+
+static virNodeDeviceObjPtr
+virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs,
+ const char *name)
+{
+return virObjectRef(virHashLookup(devs->objs, name));
}
@@ -238,20 +274,42 @@ virNodeDeviceObjPtr
virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
const char *name)
{
-size_t i;
-
-for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjPtr obj = devs->objs[i];
-virNodeDeviceDefPtr def;
+virNodeDeviceObjPtr obj;
+virObjectLock(devs);
+obj = virNodeDeviceObjListFindByNameLocked(devs, name);
+virObjectUnlock(devs);
+if (obj)
virObjectLock(obj);
-def = obj->def;
-if (STREQ(def->name, name))
-return virObjectRef(obj);
-virObjectUnlock(obj);
-}
-return NULL;
+return obj;
+}
+
+
+struct virNodeDeviceObjListFindByWWNsData {
+const char *parent_wwnn;
+const char *parent_wwpn;
+};
+
+static int
+virNodeDeviceObjListFindByWWNsCallback(const void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ const void *opaque)
+{
+virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+struct virNodeDeviceObjListFindByWWNsData *data =
+
[libvirt] [PATCH v6 0/4] Make virNodeDeviceObj and virNodeDeviceObjList private
NB: Like v5, keeping same title for consistency v5: https://www.redhat.com/archives/libvir-list/2017-July/msg00610.html Changes since v5: * Replace patch 1 w/ the aha moment from review. The deletion API's don't need to use virNodeDeviceObjListGetParentHost. Instead they can just use virNodeDeviceObjListFindByName as the parent_wwnn/wwpn and parent_fabric_wwn are not stored in the vHBA. They're merely for the purpose of creation. The vHBA *does* store the def->parent by name and we can use that instead * Add patch 2 in order to remove the CREATE_DEVICE boolean from virNodeDeviceObjListGetParentHost as it won't be necessary since the only consumers are now CreateXML callers * Modify patch 3 calls to virHashSearch to add a NULL parameter since commit '38e516a52' added this (a real buzzkill for other changes too) * Modify patch 4 to include removing the locks in Nodedev calls for test_driver as well. John Ferlan (4): nodedev: Alter node device deletion logic nodedev: Remove @create from virNodeDeviceObjListGetParentHost nodedev: Convert virNodeDeviceObjListPtr to use hash tables nodedev: Remove driver locks around object list mgmt code src/conf/virnodedeviceobj.c | 580 --- src/conf/virnodedeviceobj.h | 3 +- src/node_device/node_device_driver.c | 90 ++ src/node_device/node_device_hal.c| 16 +- src/node_device/node_device_udev.c | 13 +- src/test/test_driver.c | 53 ++-- 6 files changed, 459 insertions(+), 296 deletions(-) -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: domain: Store and restore autoCpuset to status XML
On Thu, Jul 20, 2017 at 14:12:44 +0200, Andrea Bolognani wrote:
> On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote:
> > @@ -1765,20 +1765,30 @@
> > qemuDomainObjPtrivateXMLFormatAutomaticPlacement(virBufferPtr buf,
> > qemuDomainObjPrivatePtr
> >priv)
> > {
> [...]
> > -virBufferAsprintf(buf, "\n", nodeset);
> > +if (priv->autoCpuset &&
> > +!((cpuset = virBitmapFormat(priv->autoCpuset
> > +goto cleanup;
>
> The previous implementation didn't format the
> element at all if there was nodeset, whereas the new one
> will always format it. You could add
>
> if (!nodeset && !cpuset)
> goto cleanup;
If you call virBitmapFormat on an empty or NULL bitmap you still get a
(empty) string allocated so that condition is basically identical to the
one that's already there due to how the bitmaps are formatted:
if (!priv->autoNodeset && !priv->autoCpuset)
return 0;
if (priv->autoNodeset &&
!((nodeset = virBitmapFormat(priv->autoNodeset
goto cleanup;
if (priv->autoCpuset &&
!((cpuset = virBitmapFormat(priv->autoCpuset
goto cleanup;
> here to restore that behavior, but maybe you just decided
> it's not worth it. Just felt like I should point that out.
>
> > +virBufferAddLit(buf, " > +virBufferEscapeString(buf, " nodeset='%s'", nodeset);
> > +virBufferEscapeString(buf, " cpuset='%s'", cpuset);
>
> I had to look up the implementation of virBufferEscapeString()
> to figure out that nodeset or cpuset possibly being NULL is
> handled automatically by that function, which I found to be a
> pretty surprising behavior. Not a problem with your patch
> though :)
>
> > @@ -1958,11 +1968,13 @@
> > qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver,
> > {
> > virCapsPtr caps = NULL;
> > char *nodeset;
> > +char *cpuset;
> > int ret = -1;
> >
> > nodeset = virXPathString("string(./numad/@nodeset)", ctxt);
> > +cpuset = virXPathString("string(./numad/@cpuset)", ctxt);
> >
> > -if (!nodeset)
> > +if (!nodeset && !cpuset)
> > return 0;
>
> I don't think you want this hunk.
>
> With the new condition, you will perform an early exit only
> if both nodeset and cpuset are NULL; if nodeset is NULL but
> cpuset isn't, the first call to virBitmapParse() a few lines
> below will error out.
That shouldn't ever happen (tm) :D
I'll can add a condition that if nodeset is not in the XML the parsing
will be skipped. So in that case only cpuset can be present (for future
use).
I'll also add a note that accessing autoNodeset in the else branch of if
(cpuset) is safe.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1] numa: compute and set matching vcpus for numa domains
On Thu, Jul 20, 2017 at 14:17:29 +0100, Daniel Berrange wrote: > On Thu, Jul 20, 2017 at 03:03:25PM +0200, Wim ten Have wrote: > > On Thu, 20 Jul 2017 11:10:31 +0100 > > "Daniel P. Berrange" wrote: > > > > > On Thu, Jul 20, 2017 at 11:29:26AM +0200, Wim Ten Have wrote: > > > > From: Wim ten Have > > > > > > > > The QEMU driver can erroneously allocate more vpus to a domain > > > > than there are cpus in the domain if the element is used > > > > to describe element topology. Fix this by calculating > > > > the number of cpus described in the element of a > > > > element and comparing it to the number of vcpus. If the number > > > > of vcpus is greater than the number of cpus, cap the number of > > > > vcpus to the number of cpus. > > > > > > > > This patch also adds a small libvirt documentation update under > > > > the "CPU Allocation" section. > > > > > > > > Signed-off-by: Wim ten Have > > > > --- > > > > docs/formatdomain.html.in | 9 - > > > > src/conf/domain_conf.c| 14 +++--- > > > > 2 files changed, 19 insertions(+), 4 deletions(-)a [...] > > Without the fix under former domain description libvirt would > > bring whole '16' vcpus to the guest, and if there was a current value > > given all by current on top of the > for that matter :-( so wrong; > > > > 16 > > .. > > > > > > > > > > > > > > This is saying that we have 16 CPU sockets, but you're only > assigning NUMA affinity to 4 of the sockets. IMHO that is > a configuration mistake. All 16 CPU sockets need to have > affinity assigned, so that when they're later brought online > they can be placed suitably on the host. > > > > > With the fix, its post configuration action will now nicely rewrite > > the element as shown below; > > > > 16 > > .. > > > > > > > > > > > > > > IMHO that is still just as broken as the previous config, > because it isn't saying how to set affinity on the remaining > 12 vCPUs that can be brought online. Yes, exactly. The reason for leaving this in the 'gray zone' was that we did not check this previously and qemu just picked one of the nodes and dumped the rest of the vcpus there. The correct way would be to enforce full mapping in NUMA. Note that this needs to be done in the validation callbacks rather than the post parse callbacks, since there's real risk of VMs vanishing. Validation callbacks are triggered only at start of the VM so provide an error. Peter signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Refactored OVS VLAN configuration
On 07/17/2017 05:48 PM, Antoine Millet wrote:
> Moved OVS VLAN configuration arguments construction into a separated
> function to be reused.
> ---
> src/util/virnetdevopenvswitch.c | 104
>
> src/util/virnetdevopenvswitch.h | 5 ++
> 2 files changed, 68 insertions(+), 41 deletions(-)
>
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index a5ecfb691..89ed0c91c 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -64,6 +64,67 @@ virNetDevOpenvswitchAddTimeout(virCommandPtr cmd)
> }
>
> /**
> + * virNetDevOpenvswitchConstructVlans:
> + * @cmd: command to construct
> + * @virtVlan: VLAN configuration to be applied
> + *
> + * Construct the VLAN configuration parameters to be passed to ovs-vsctl
> command.
> + *
> + * Returns 0 in case of success or -1 in case of failure.
> + */
> +int virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr
> virtVlan)
> +{
> +int ret = -1;
> +size_t i = 0;
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +if (virtVlan && virtVlan->nTags > 0) {
How about reversing this so that the rest of the function can be
indented one level to the left? I mean:
if (!virtVlan || !virtVlan->nTags)
return 0;
switch() {
...
> +
> +switch (virtVlan->nativeMode) {
> +case VIR_NATIVE_VLAN_MODE_TAGGED:
> +virCommandAddArg(cmd, "vlan_mode=native-tagged");
> +virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> +break;
> +case VIR_NATIVE_VLAN_MODE_UNTAGGED:
> +virCommandAddArg(cmd, "vlan_mode=native-untagged");
> +virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> +break;
> +case VIR_NATIVE_VLAN_MODE_DEFAULT:
> +default:
> +break;
> +}
> +
> +if (virtVlan->trunk) {
> +virBufferAddLit(&buf, "trunk=");
> +
> +/*
> + * Trunk ports have at least one VLAN. Do the first one
> + * outside the "for" loop so we can put a "," at the
> + * start of the for loop if there are more than one VLANs
> + * on this trunk port.
> + */
> +virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
> +
> +for (i = 1; i < virtVlan->nTags; i++) {
> +virBufferAddLit(&buf, ",");
> +virBufferAsprintf(&buf, "%d", virtVlan->tag[i]);
> +}
> +
> +if (virBufferCheckError(&buf) < 0)
> +goto cleanup;
> +virCommandAddArg(cmd, virBufferCurrentContent(&buf));
> +} else if (virtVlan->nTags) {
> +virCommandAddArgFormat(cmd, "tag=%d", virtVlan->tag[0]);
> +}
> +}
> +
> +ret = 0;
> +cleanup:
> +virBufferFreeAndReset(&buf);
> +return ret;
Indent.
> +}
> +
> +/**
> * virNetDevOpenvswitchAddPort:
> * @brname: the bridge name
> * @ifname: the network interface name
> @@ -82,7 +143,6 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
> char *ifname,
> virNetDevVlanPtr virtVlan)
> {
> int ret = -1;
> -size_t i = 0;
> virCommandPtr cmd = NULL;
> char macaddrstr[VIR_MAC_STRING_BUFLEN];
> char ifuuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -91,7 +151,6 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
> char *ifname,
> char *ifaceid_ex_id = NULL;
> char *profile_ex_id = NULL;
> char *vmid_ex_id = NULL;
> -virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> virMacAddrFormat(macaddr, macaddrstr);
> virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -117,44 +176,8 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
> virCommandAddArgList(cmd, "--", "--if-exists", "del-port",
> ifname, "--", "add-port", brname, ifname, NULL);
>
> -if (virtVlan && virtVlan->nTags > 0) {
> -
> -switch (virtVlan->nativeMode) {
> -case VIR_NATIVE_VLAN_MODE_TAGGED:
> -virCommandAddArg(cmd, "vlan_mode=native-tagged");
> -virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> -break;
> -case VIR_NATIVE_VLAN_MODE_UNTAGGED:
> -virCommandAddArg(cmd, "vlan_mode=native-untagged");
> -virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> -break;
> -case VIR_NATIVE_VLAN_MODE_DEFAULT:
> -default:
> -break;
> -}
> -
> -if (virtVlan->trunk) {
> -virBufferAddLit(&buf, "trunk=");
> -
> -/*
> - * Trunk ports have at least one VLAN. Do the first one
> - * outside the "for" loop so we can put a "," at the
> - * start of the for loop if there are more than one VLANs
> - * on this trunk port.
> - */
> -virBufferAsprintf(&buf,
Re: [libvirt] [PATCH 0/2] Handle hotplug change on VLAN configuration using OVS
On 07/17/2017 05:48 PM, Antoine Millet wrote: > This patch set allow to change VLAN configuration of running guest using OVS > as networking backend. > > Use case here: > https://www.redhat.com/archives/libvirt-users/2017-July/msg00043.html > > "Refactored OVS VLAN configuration" moves the code building the VLAN > configuration > arguments passed to ovs-vsctl into a separated function to be reused by > "Handle > hotplug change on VLAN configuration using OVS" which implements the handling > of > VLAN change into qemuDomainChangeNet. > > Antoine Millet (2): > Refactored OVS VLAN configuration > Handle hotplug change on VLAN configuration using OVS > > src/libvirt_private.syms| 1 + > src/qemu/qemu_hotplug.c | 16 - > src/util/virnetdevopenvswitch.c | 145 > > src/util/virnetdevopenvswitch.h | 9 +++ > 4 files changed, 127 insertions(+), 44 deletions(-) > I've fixed all the issues I found, ACKed and pushed. Congratulations on your first libvirt contribution! Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1] numa: compute and set matching vcpus for numa domains
On Thu, Jul 20, 2017 at 03:03:25PM +0200, Wim ten Have wrote:
> On Thu, 20 Jul 2017 11:10:31 +0100
> "Daniel P. Berrange" wrote:
>
> > On Thu, Jul 20, 2017 at 11:29:26AM +0200, Wim Ten Have wrote:
> > > From: Wim ten Have
> > >
> > > The QEMU driver can erroneously allocate more vpus to a domain
> > > than there are cpus in the domain if the element is used
> > > to describe element topology. Fix this by calculating
> > > the number of cpus described in the element of a
> > > element and comparing it to the number of vcpus. If the number
> > > of vcpus is greater than the number of cpus, cap the number of
> > > vcpus to the number of cpus.
> > >
> > > This patch also adds a small libvirt documentation update under
> > > the "CPU Allocation" section.
> > >
> > > Signed-off-by: Wim ten Have
> > > ---
> > > docs/formatdomain.html.in | 9 -
> > > src/conf/domain_conf.c| 14 +++---
> > > 2 files changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > index 07208ee..3c85307 100644
> > > --- a/docs/formatdomain.html.in
> > > +++ b/docs/formatdomain.html.in
> > > @@ -501,7 +501,14 @@
> > >vcpu
> > >The content of this element defines the maximum number of
> > > virtual
> > > CPUs allocated for the guest OS, which must be between 1 and
> > > -the maximum supported by the hypervisor.
> > > +the maximum supported by the hypervisor. If a numa
> > > +element is contained within the cpu element, the
> > > +number of virtual CPUs to be allocated is compared with the sum
> > > +of the cpus attributes across the cell
> > > +elements within the numa element. If the number of
> > > +virtual CPUs is greater than the sum of the cpus
> > > +attributes, the number of virtual CPUs is capped at sum of the
> > > +cpus attributes.
> > >
> > > cpuset
> > >
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index 958a5b7..73d7f4f 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> > > @@ -3844,12 +3844,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
> > > unsigned long long numaMemory = 0;
> > > unsigned long long hotplugMemory = 0;
> > >
> > > -/* Attempt to infer the initial memory size from the sum NUMA memory
> > > sizes
> > > - * in case ABI updates are allowed or the element wasn't
> > > specified */
> > > +/* Attempt to infer the initial memory size from the sum NUMA memory
> > > + * sizes in case ABI updates are allowed or the element
> > > + * wasn't specified. Also cap the vcpu count to the sum of NUMA cpus
> > > + * allocated for all nodes. */
> > > if (def->mem.total_memory == 0 ||
> > > parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE ||
> > > -parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION)
> > > +parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION) {
> > > +unsigned int numaVcpus = 0;
> > > +
> > > +if ((numaVcpus = virDomainNumaGetCPUCountTotal(def->numa)))
> > > +virDomainDefSetVcpus(def, numaVcpus);
> > > +
> > > numaMemory = virDomainNumaGetMemorySize(def->numa);
> > > +}
> >
> > AFAICT, this scenario is simply a user configuration mistake, and so we
> > should report an error message to them to fix it.
>
> Not to push but I think this is the correct way ... O:-).
>
> Ok, perhaps the documentation note/commit message should be slightly
> rewritten aswell as the altered comment on specific code section.
>
> The change is _NOT_ changing final guest provided 'maxvcpus' but
> 'vcpus' instead. This means that it adds the "current='#count'" under
> the vcpu element if such is less then vcpu 'maxvcpus' provided count.
> If equal there is no issue and if larger there is indeed a correct
> error message (exceptional condition).
>
> Imagine simpel domain description _WITHOUT_ and below
> element description;
>
> 16
>
> This nicely creates a guest with 4 domain CPUs added, where the
> administrator can "hot-add" an additional 12 CPUs making the full
> 'maxvcpus' defined 16. "hot-add" by virsh;
>
> virsh # setvcpus kvm26 16
>
> Without the fix under former domain description libvirt would
> bring whole '16' vcpus to the guest, and if there was a current value
> given all by current on top of the for that matter :-( so wrong;
>
> 16
> ..
>
>
>
>
>
>
This is saying that we have 16 CPU sockets, but you're only
assigning NUMA affinity to 4 of the sockets. IMHO that is
a configuration mistake. All 16 CPU sockets need to have
affinity assigned, so that when they're later brought online
they can be placed suitably on the host.
>
> With the fix, its post configuration action will now nicely rewrite
> the
Re: [libvirt] [PATCH 2/2] Handle hotplug change on VLAN configuration using OVS
On 07/17/2017 05:49 PM, Antoine Millet wrote:
> A new function virNetDevOpenvswitchUpdateVlan has been created to instruct
> OVS of the changes. qemuDomainChangeNet has been modified to handle the
> update of the VLAN configuration for a running guest and rely on
> virNetDevOpenvswitchUpdateVlan to do the actual update if needed.
> ---
> src/libvirt_private.syms| 1 +
> src/qemu/qemu_hotplug.c | 16 +---
> src/util/virnetdevopenvswitch.c | 41
> +
> src/util/virnetdevopenvswitch.h | 4
> 4 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 187b12b32..36caf6fbf 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2206,6 +2206,7 @@ virNetDevOpenvswitchInterfaceStats;
> virNetDevOpenvswitchRemovePort;
> virNetDevOpenvswitchSetMigrateData;
> virNetDevOpenvswitchSetTimeout;
> +virNetDevOpenvswitchUpdateVlan;
>
>
> # util/virnetdevtap.h
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4bc49720b..b9ad9e6c8 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2998,6 +2998,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> bool needReplaceDevDef = false;
> bool needBandwidthSet = false;
> bool needCoalesceChange = false;
> +bool needVlanUpdate = false;
> int ret = -1;
> int changeidx = -1;
>
> @@ -3279,12 +3280,15 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> virDomainNetGetActualDirectDev(newdev)) ||
> virDomainNetGetActualDirectMode(olddev) !=
> virDomainNetGetActualDirectMode(newdev) ||
>
> !virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev),
> -
> virDomainNetGetActualVirtPortProfile(newdev)) ||
> -!virNetDevVlanEqual(virDomainNetGetActualVlan(olddev),
> -virDomainNetGetActualVlan(newdev))) {
> +
> virDomainNetGetActualVirtPortProfile(newdev))) {
> needReconnect = true;
> }
>
> +if (!virNetDevVlanEqual(virDomainNetGetActualVlan(olddev),
> + virDomainNetGetActualVlan(newdev))) {
> +needVlanUpdate = true;
> +}
> +
> if (olddev->linkstate != newdev->linkstate)
> needLinkStateChange = true;
>
> @@ -3344,6 +3348,12 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> +if (needVlanUpdate) {
> +if (virNetDevOpenvswitchUpdateVlan(newdev->ifname, &newdev->vlan) <
> 0)
> +goto cleanup;
> +needReplaceDevDef = true;
> +}
> +
> if (needReplaceDevDef) {
> /* the changes above warrant replacing olddev with newdev in
> * the domain's nets list.
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index 89ed0c91c..a12bc3dbc 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -459,3 +459,44 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path,
> VIR_FREE(ovs_timeout);
> return ret;
> }
> +
> +/**
> + * virNetDevOpenvswitchUpdateVlan:
> + * @ifname: the network interface name
> + * @virtVlan: VLAN configuration to be applied
> + *
> + * Update VLAN configuration of an OVS port.
> + *
> + * Returns 0 in case of success or -1 in case of failure.
> + */
> +int virNetDevOpenvswitchUpdateVlan(const char *ifname,
> + virNetDevVlanPtr virtVlan)
> +{
> +int ret = -1;
> +virCommandPtr cmd = NULL;
> +
> +cmd = virCommandNew(OVSVSCTL);
> +virNetDevOpenvswitchAddTimeout(cmd);
> +virCommandAddArgList(cmd,
> + "--", "--if-exists", "clear", "Port", ifname, "tag",
> + "--", "--if-exists", "clear", "Port", ifname,
> "trunk",
> + "--", "--if-exists", "clear", "Port", ifname,
> "vlan_mode",
> + "--", "--if-exists", "set", "Port", ifname, NULL);
> +
> +if (virNetDevOpenvswitchConstructVlans(cmd, virtVlan) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to construct vlan configuration for port
> %s"), ifname);
This rewrites original error reported by
virNetDevOpenvswitchConstructVlans().
> +goto cleanup;
> +}
> +
> +if (virCommandRun(cmd, NULL) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to set vlan configuration on port %s"),
> ifname);
> +goto cleanup;
> +}
> +
> +ret = 0;
> + cleanup:
> +virCommandFree(cmd);
> +return ret;
> +}
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1] numa: compute and set matching vcpus for numa domains
On Thu, 20 Jul 2017 11:10:31 +0100
"Daniel P. Berrange" wrote:
> On Thu, Jul 20, 2017 at 11:29:26AM +0200, Wim Ten Have wrote:
> > From: Wim ten Have
> >
> > The QEMU driver can erroneously allocate more vpus to a domain
> > than there are cpus in the domain if the element is used
> > to describe element topology. Fix this by calculating
> > the number of cpus described in the element of a
> > element and comparing it to the number of vcpus. If the number
> > of vcpus is greater than the number of cpus, cap the number of
> > vcpus to the number of cpus.
> >
> > This patch also adds a small libvirt documentation update under
> > the "CPU Allocation" section.
> >
> > Signed-off-by: Wim ten Have
> > ---
> > docs/formatdomain.html.in | 9 -
> > src/conf/domain_conf.c| 14 +++---
> > 2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 07208ee..3c85307 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -501,7 +501,14 @@
> >vcpu
> >The content of this element defines the maximum number of virtual
> > CPUs allocated for the guest OS, which must be between 1 and
> > -the maximum supported by the hypervisor.
> > +the maximum supported by the hypervisor. If a numa
> > +element is contained within the cpu element, the
> > +number of virtual CPUs to be allocated is compared with the sum
> > +of the cpus attributes across the cell
> > +elements within the numa element. If the number of
> > +virtual CPUs is greater than the sum of the cpus
> > +attributes, the number of virtual CPUs is capped at sum of the
> > +cpus attributes.
> >
> > cpuset
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 958a5b7..73d7f4f 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -3844,12 +3844,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
> > unsigned long long numaMemory = 0;
> > unsigned long long hotplugMemory = 0;
> >
> > -/* Attempt to infer the initial memory size from the sum NUMA memory
> > sizes
> > - * in case ABI updates are allowed or the element wasn't
> > specified */
> > +/* Attempt to infer the initial memory size from the sum NUMA memory
> > + * sizes in case ABI updates are allowed or the element
> > + * wasn't specified. Also cap the vcpu count to the sum of NUMA cpus
> > + * allocated for all nodes. */
> > if (def->mem.total_memory == 0 ||
> > parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE ||
> > -parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION)
> > +parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION) {
> > +unsigned int numaVcpus = 0;
> > +
> > +if ((numaVcpus = virDomainNumaGetCPUCountTotal(def->numa)))
> > +virDomainDefSetVcpus(def, numaVcpus);
> > +
> > numaMemory = virDomainNumaGetMemorySize(def->numa);
> > +}
>
> AFAICT, this scenario is simply a user configuration mistake, and so we
> should report an error message to them to fix it.
Not to push but I think this is the correct way ... O:-).
Ok, perhaps the documentation note/commit message should be slightly
rewritten aswell as the altered comment on specific code section.
The change is _NOT_ changing final guest provided 'maxvcpus' but
'vcpus' instead. This means that it adds the "current='#count'" under
the vcpu element if such is less then vcpu 'maxvcpus' provided count.
If equal there is no issue and if larger there is indeed a correct
error message (exceptional condition).
Imagine simpel domain description _WITHOUT_ and below
element description;
16
This nicely creates a guest with 4 domain CPUs added, where the
administrator can "hot-add" an additional 12 CPUs making the full
'maxvcpus' defined 16. "hot-add" by virsh;
virsh # setvcpus kvm26 16
Without the fix under former domain description libvirt would
bring whole '16' vcpus to the guest, and if there was a current value
given all by current on top of the 16
..
With the fix, its post configuration action will now nicely rewrite
the element as shown below;
16
..
In case "current='#count'" description is given where it does not
match the count then such is corrected to
cap the sum of all .
Perhaps that occurance should be rejected with an error message, then
such is not exceeding the domain administrators written 'maxvcpus'
element being '16' within my example above. That is a hard limit and
of course rejected with an error.
I will not argue further ... O:-) I'll wait and if it is a _NO_ then
it is a _NO_!
Regards,
- Wim.
--
libvir-list
Re: [libvirt] [PATCH 5/5] qemu: process: Don't put memoryless NUMA nodes into autoNodeset
On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote: > 'numad' may return a nodeset which contains NUMA nodes without memory > for certain configurations. Since cgroups code will not be happy using > nodes without memory we need to store only numa nodes with memory in > autoNodeset. > > On the other hand autoCpuset should contain cpus also for nodes which > do not have any memory. > --- > src/qemu/qemu_process.c | 20 > 1 file changed, 16 insertions(+), 4 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 1/3] nodedev: Fix usage of virNodeDeviceObjListGetParentHost
On 07/20/2017 03:22 AM, Erik Skultety wrote: @@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; +char *name = NULL; +char *parent = NULL; +char *parent_wwnn = NULL; +char *parent_wwpn = NULL; +char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; @@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup; -/* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ +/* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */ >>> >>> And as I originally suggested I would allocate a new temporary @def >>> structure, >>> initialize it, pass it to the *GetParentHost method like nothing out of the >>> ordinary happened and mentioned in the commentary why we've done it that way >>> (which you already do in this patch). >>> >> >> So you'd prefer some sort of virNodeDeviceDefCopy function be created? >> Or use VIR_ALLOC(def) and copy in the 5 fields only to then >> virNodeDeviceDefFree() it afterwards? > > Yeah, exactly, I'm aware of those unused extra field, I don't know it just > looked more appealing and transparent to me, again, matter of preference, > the way I see it, you store/pass it in a much more compact way with the added > benefit of other, in this case currently unused, fields, should > virNodeDeviceObjListGetParentHost ever need them. > > Erik > You cut off my next line: "Seems like overkill to me." Still in actually trying to implement that - I've come to realize that @def for the CREATE path will be the only time parent_wwnn, parent_wwpn, and parent_fabric_wwn actually exist. They're not saved in the vHBA that's created... All that the created vHBA gets is the , so the delete paths do not have to call virNodeDeviceObjListGetParentHost instead they can just copy the parent_name and make a nodeDeviceObjFindByName on the parent after unlocking the original obj, then call virVHBAManageVport with the parent_host # That of course means virNodeDeviceObjListGetParentHost doesn't need the third parameter either. And I don't have to create virNodeDeviceDefCopy (although it's a benign exercise). John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu: process: Extract gathering of 'numad' placement into a function
On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote:
> +static int
> +qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm,
> + virCapsPtr caps)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +char *nodesetstr = NULL;
> +int ret = -1;
> +
> +/* Get the advisory nodeset from numad if 'placement' of
> + * either or is 'auto'.
> + */
> +if (!virDomainDefNeedsPlacementAdvice(vm->def))
> +return 0;
> +
> +nodesetstr = virNumaGetAutoPlacementAdvice(virDomainDefGetVcpus(vm->def),
> +
> virDomainDefGetMemoryTotal(vm->def));
> +
> +if (!nodesetstr)
> +goto cleanup;
> +
> +VIR_DEBUG("Nodeset returned from numad: %s", nodesetstr);
> +
> +if (virBitmapParse(nodesetstr, &priv->autoNodeset,
> + VIR_DOMAIN_CPUMASK_LEN) < 0)
This call is not any longer than others before or after it,
so there's no reason IMHO to distribute it among two lines.
You could even rename 'nodesetstr' to 'nodeset', which is
the name you've used for the same purpose in other places,
and shorten it a bit further ;)
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 4/6] util: storage: fill in default ports when parsing backing chain
Similarly to when parsing XML we need to fill in default ports for the
backing chain. This was missed in commit 5bda835466a8050625dd8bb10566e
---
src/util/virstoragefile.c | 3 +++
tests/virstoragetest.c| 14 +++---
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 2d0ff7812..bc1b616d4 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3340,6 +3340,9 @@ virStorageSourceNewFromBackingAbsolute(const char *path)
if (rc < 0)
goto error;
+
+if (virStorageSourceNetworkAssignDefaultPorts(ret) < 0)
+goto error;
}
return ret;
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 5352500b7..d83db78f5 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1349,7 +1349,7 @@ mymain(void)
TEST_BACKING_PARSE("://", NULL);
TEST_BACKING_PARSE("http://example.com/file";,
"\n"
- " \n"
+ " \n"
"\n");
TEST_BACKING_PARSE("rbd:testshare:id=asdf:mon_host=example.com",
"\n"
@@ -1385,14 +1385,14 @@ mymain(void)
TEST_BACKING_PARSE("json:{\"file.driver\":\"http\", "
"\"file.url\":\"http://example.com/file\"}";,
"\n"
- " \n"
+ " \n"
"\n");
TEST_BACKING_PARSE("json:{\"file\":{ \"driver\":\"http\","
"\"url\":\"http://example.com/file\"";
"}"
"}",
"\n"
- " \n"
+ " \n"
"\n");
TEST_BACKING_PARSE("json:{\"file.driver\":\"ftp\", "
"\"file.url\":\"http://example.com/file\"}";,
@@ -1400,7 +1400,7 @@ mymain(void)
TEST_BACKING_PARSE("json:{\"file.driver\":\"gluster\", "
"\"file.filename\":\"gluster://example.com/vol/file\"}",
"\n"
- " \n"
+ " \n"
"\n");
TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"gluster\","
"\"volume\":\"testvol\","
@@ -1421,7 +1421,7 @@ mymain(void)
"\n"
" \n"
" \n"
-" \n"
+" \n"
"\n");
TEST_BACKING_PARSE("json:{\"file.driver\":\"gluster\","
"\"file.volume\":\"testvol\","
@@ -1441,7 +1441,7 @@ mymain(void)
"\n"
" \n"
" \n"
-" \n"
+" \n"
"\n");
TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"nbd\","
"\"path\":\"/path/to/socket\""
@@ -1552,7 +1552,7 @@ mymain(void)
"}"
"}",
"\n"
- " \n"
+ " \n"
"\n");
TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"iscsi\","
"\"transport\":\"tcp\","
--
2.13.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/6] conf: domain: Split up virDomainStorageHostParse and rename it
Split out parsing of one host into a separate function and add a new
function to loop through all the host XML nodes.
This change removes multiple levels of nesting due to the old XML
parsing approach used.
---
src/conf/domain_conf.c | 117 +++--
1 file changed, 64 insertions(+), 53 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9320794de..a3fd7195a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6429,71 +6429,61 @@ virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node,
return ret;
}
+
static int
-virDomainStorageHostParse(xmlNodePtr node,
- virStorageNetHostDefPtr *hosts,
- size_t *nhosts)
+virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
+ virStorageNetHostDefPtr *hosts,
+ size_t *nhosts)
{
int ret = -1;
-xmlNodePtr child;
char *transport = NULL;
virStorageNetHostDef host;
memset(&host, 0, sizeof(host));
+host.transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
-child = node->children;
-while (child != NULL) {
-if (child->type == XML_ELEMENT_NODE &&
-xmlStrEqual(child->name, BAD_CAST "host")) {
-
-host.transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
-
-/* transport can be tcp (default), unix or rdma. */
-if ((transport = virXMLPropString(child, "transport"))) {
-host.transport =
virStorageNetHostTransportTypeFromString(transport);
-if (host.transport < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown protocol transport type '%s'"),
- transport);
-goto cleanup;
-}
-}
+/* transport can be tcp (default), unix or rdma. */
+if ((transport = virXMLPropString(hostnode, "transport"))) {
+host.transport = virStorageNetHostTransportTypeFromString(transport);
+if (host.transport < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown protocol transport type '%s'"),
+ transport);
+goto cleanup;
+}
+}
-host.socket = virXMLPropString(child, "socket");
+host.socket = virXMLPropString(hostnode, "socket");
-if (host.transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
-host.socket == NULL) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing socket for unix transport"));
-goto cleanup;
-}
+if (host.transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
+host.socket == NULL) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing socket for unix transport"));
+goto cleanup;
+}
-if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
-host.socket != NULL) {
-virReportError(VIR_ERR_XML_ERROR,
- _("transport '%s' does not support "
- "socket attribute"),
- transport);
-goto cleanup;
-}
+if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
+host.socket != NULL) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("transport '%s' does not support "
+ "socket attribute"),
+ transport);
+goto cleanup;
+}
-VIR_FREE(transport);
+if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) {
+if (!(host.name = virXMLPropString(hostnode, "name"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing name for host"));
+goto cleanup;
+}
-if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) {
-if (!(host.name = virXMLPropString(child, "name"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing name for host"));
-goto cleanup;
-}
+host.port = virXMLPropString(hostnode, "port");
+}
-host.port = virXMLPropString(child, "port");
-}
+if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0)
+goto cleanup;
-if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0)
-goto cleanup;
-}
-child = child->next;
-}
ret = 0;
cleanup:
@@ -6502,6 +6492,27 @@ virDomainStorageHostParse(xmlNodePtr node,
return ret;
}
+
+static int
+virDomainStorageNetworkParseHosts(xmlNodePtr node,
+ virStorageNetHostDefPtr *hosts,
+ size_t *nhosts)
+{
+xmlNodePtr child;
+
+for (
[libvirt] [PATCH v2 6/6] virStorageNetHostDef: Turn @port into integer
Currently, @port is type of string. Well, that's overkill and
waste of memory. Port is always an integer. Use it as such.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c| 20 +++---
src/libxl/libxl_conf.c| 2 +-
src/qemu/qemu_block.c | 7 +++-
src/qemu/qemu_command.c | 11 +++---
src/qemu/qemu_parse_command.c | 13 +++
src/storage/storage_backend_gluster.c | 17 ++---
src/storage/storage_driver.c | 7 +---
src/util/virstoragefile.c | 69 ++-
src/util/virstoragefile.h | 4 +-
src/xenconfig/xen_xl.c| 2 +-
10 files changed, 68 insertions(+), 84 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3fd7195a..7ba6804ac 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6437,6 +6437,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
{
int ret = -1;
char *transport = NULL;
+char *port = NULL;
virStorageNetHostDef host;
memset(&host, 0, sizeof(host));
@@ -6478,7 +6479,10 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
goto cleanup;
}
-host.port = virXMLPropString(hostnode, "port");
+if ((port = virXMLPropString(hostnode, "port"))) {
+if (virStringParsePort(port, &host.port) < 0)
+goto cleanup;
+}
}
if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0)
@@ -6489,6 +6493,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
cleanup:
virStorageNetHostDefClear(&host);
VIR_FREE(transport);
+VIR_FREE(port);
return ret;
}
@@ -7893,8 +7898,8 @@ virDomainDiskSourceParse(xmlNodePtr node,
if (virDomainStorageNetworkParseHosts(node, &src->hosts, &src->nhosts)
< 0)
goto cleanup;
-if (virStorageSourceNetworkAssignDefaultPorts(src) < 0)
-goto cleanup;
+virStorageSourceNetworkAssignDefaultPorts(src);
+
break;
case VIR_STORAGE_TYPE_VOLUME:
if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0)
@@ -14920,7 +14925,7 @@
virDomainHostdevMatchSubsysSCSIiSCSI(virDomainHostdevDefPtr first,
&second->source.subsys.u.scsi.u.iscsi;
if (STREQ(first_iscsisrc->hosts[0].name, second_iscsisrc->hosts[0].name) &&
-STREQ(first_iscsisrc->hosts[0].port, second_iscsisrc->hosts[0].port) &&
+first_iscsisrc->hosts[0].port == second_iscsisrc->hosts[0].port &&
STREQ(first_iscsisrc->path, second_iscsisrc->path))
return 1;
return 0;
@@ -21419,7 +21424,9 @@ virDomainDiskSourceFormatNetwork(virBufferPtr buf,
for (n = 0; n < src->nhosts; n++) {
virBufferAddLit(buf, "hosts[n].name);
-virBufferEscapeString(buf, " port='%s'", src->hosts[n].port);
+
+if (src->hosts[n].port)
+virBufferAsprintf(buf, " port='%u'", src->hosts[n].port);
if (src->hosts[n].transport)
virBufferAsprintf(buf, " transport='%s'",
@@ -22266,7 +22273,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
virBufferAddLit(buf, "hosts[0].name);
-virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].port);
+if (iscsisrc->hosts[0].port)
+virBufferAsprintf(buf, " port='%u'", iscsisrc->hosts[0].port);
virBufferAddLit(buf, "/>\n");
} else {
virBufferAsprintf(buf, "\n",
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a85bc71d2..4416a09dd 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -706,7 +706,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
virBufferAsprintf(&buf, "%s", src->hosts[i].name);
if (src->hosts[i].port)
-virBufferAsprintf(&buf, "\\:%s", src->hosts[i].port);
+virBufferAsprintf(&buf, "\\:%u", src->hosts[i].port);
}
}
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 93124c5ba..22de70657 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -447,6 +447,7 @@
qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src,
virJSONValuePtr ret = NULL;
virStorageNetHostDefPtr host;
const char *transport;
+char *port = NULL;
size_t i;
if (!(servers = virJSONValueNewArray()))
@@ -462,10 +463,13 @@
qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src,
else
transport = "inet";
+if (virAsprintf(&port, "%u", host->port) < 0)
+goto cleanup;
+
if (virJSONValueObjectCreate(&server,
"s:type", transport,
"s:host", host->name,
-
[libvirt] [PATCH v2 3/6] qemu: command: Remove condition to use default sheepdog port
Since we now set the default ports when parsing disks, it's not
necessary to have default port numbers encoded in the command line
generator.
---
src/qemu/qemu_command.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index be23796df..810578840 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -938,8 +938,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
goto cleanup;
} else if (src->nhosts == 1) {
if (virAsprintf(&ret, "sheepdog:%s:%s:%s",
-src->hosts->name,
-src->hosts->port ? src->hosts->port : "7000",
+src->hosts->name, src->hosts->port,
src->path) < 0)
goto cleanup;
} else {
--
2.13.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/6] Convert port to unsigned integer
Since Michal did not want to finis this I did. I also found a few bugs in my previous patches which are addressed here. Peter Krempa (6): qemu: command: Rename and move qemuNetworkDriveGetPort util: uri: Convert port number to unsigned integer qemu: command: Remove condition to use default sheepdog port util: storage: fill in default ports when parsing backing chain conf: domain: Split up virDomainStorageHostParse and rename it virStorageNetHostDef: Turn @port into integer src/conf/domain_conf.c| 133 +++--- src/libvirt_private.syms | 1 + src/libxl/libxl_conf.c| 2 +- src/qemu/qemu_block.c | 7 +- src/qemu/qemu_command.c | 30 ++-- src/qemu/qemu_parse_command.c | 13 ++-- src/storage/storage_backend_gluster.c | 17 + src/storage/storage_driver.c | 7 +- src/util/virstoragefile.c | 68 +++-- src/util/virstoragefile.h | 4 +- src/util/virstring.c | 37 ++ src/util/virstring.h | 4 + src/util/viruri.h | 2 +- src/xenconfig/xen_xl.c| 2 +- tests/virstoragetest.c| 14 ++-- 15 files changed, 182 insertions(+), 159 deletions(-) -- 2.13.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/6] util: uri: Convert port number to unsigned integer
Negative ports don't make sense so use a unsigned integer.
---
src/util/virstring.c | 2 +-
src/util/virstring.h | 2 +-
src/util/viruri.h| 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c
index fe61a3516..69e0fd173 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1356,7 +1356,7 @@ void virStringTrimOptionalNewline(char *str)
*/
int
virStringParsePort(const char *str,
- int *port)
+ unsigned int *port)
{
unsigned int p = 0;
diff --git a/src/util/virstring.h b/src/util/virstring.h
index e562bf514..ff5f0148d 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -297,7 +297,7 @@ char *virStringEncodeBase64(const uint8_t *buf, size_t
buflen);
void virStringTrimOptionalNewline(char *str);
int virStringParsePort(const char *str,
- int *port)
+ unsigned int *port)
ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
#endif /* __VIR_STRING_H__ */
diff --git a/src/util/viruri.h b/src/util/viruri.h
index 1e53abb0b..7850c38c2 100644
--- a/src/util/viruri.h
+++ b/src/util/viruri.h
@@ -42,7 +42,7 @@ struct _virURI {
char *scheme; /* the URI scheme */
char *server; /* the server part */
char *user; /* the user part */
-int port; /* the port number */
+unsigned int port; /* the port number */
char *path; /* the path string */
char *query;/* the query string */
char *fragment; /* the fragment string */
--
2.13.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/6] qemu: command: Rename and move qemuNetworkDriveGetPort
Move it to virstring.c and improve it to parse and validate ports. New
name is virStringParsePort.
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 18 +-
src/util/virstring.c | 37 +
src/util/virstring.h | 4
4 files changed, 43 insertions(+), 17 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 187b12b32..f0c2c8446 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2656,6 +2656,7 @@ virStringListJoin;
virStringListLength;
virStringListRemove;
virStringMatch;
+virStringParsePort;
virStringReplace;
virStringSearch;
virStringSortCompare;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6ac26af3e..be23796df 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -491,22 +491,6 @@ qemuSafeSerialParamValue(const char *value)
}
-static int
-qemuNetworkDriveGetPort(const char *port)
-{
-int ret = 0;
-
-if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse port number '%s'"),
- port);
-return -1;
-}
-
-return ret;
-}
-
-
/**
* qemuBuildSecretInfoProps:
* @secinfo: pointer to the secret info object
@@ -825,7 +809,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
goto cleanup;
if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
-if ((uri->port = qemuNetworkDriveGetPort(src->hosts->port)) < 0)
+if (virStringParsePort(src->hosts->port, &uri->port) < 0)
goto cleanup;
if (VIR_STRDUP(uri->scheme,
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 9b54bd6fb..fe61a3516 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1344,3 +1344,40 @@ void virStringTrimOptionalNewline(char *str)
if (*tmp == '\n')
*tmp = '\0';
}
+
+
+/**
+ * virStringParsePort:
+ * @str: port number to parse
+ * @port: pointer to parse port into
+ *
+ * Parses a string representation of a network port and validates it. Returns
+ * 0 on success and -1 on error.
+ */
+int
+virStringParsePort(const char *str,
+ int *port)
+{
+unsigned int p = 0;
+
+*port = 0;
+
+if (!str)
+return 0;
+
+if (virStrToLong_uip(str, NULL, 10, &p) < 0) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("failed to parse port number '%s'"), str);
+return -1;
+}
+
+if (p > 65535) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("port '%s' out of range"), str);
+return -1;
+}
+
+*port = p;
+
+return 0;
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 5eaaaea0a..e562bf514 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -296,4 +296,8 @@ char *virStringEncodeBase64(const uint8_t *buf, size_t
buflen);
void virStringTrimOptionalNewline(char *str);
+int virStringParsePort(const char *str,
+ int *port)
+ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
#endif /* __VIR_STRING_H__ */
--
2.13.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: domain: Store and restore autoCpuset to status XML
On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote:
> @@ -1765,20 +1765,30 @@
> qemuDomainObjPtrivateXMLFormatAutomaticPlacement(virBufferPtr buf,
> qemuDomainObjPrivatePtr
>priv)
> {
[...]
> -virBufferAsprintf(buf, "\n", nodeset);
> +if (priv->autoCpuset &&
> +!((cpuset = virBitmapFormat(priv->autoCpuset
> +goto cleanup;
The previous implementation didn't format the
element at all if there was nodeset, whereas the new one
will always format it. You could add
if (!nodeset && !cpuset)
goto cleanup;
here to restore that behavior, but maybe you just decided
it's not worth it. Just felt like I should point that out.
> +virBufferAddLit(buf, " +virBufferEscapeString(buf, " nodeset='%s'", nodeset);
> +virBufferEscapeString(buf, " cpuset='%s'", cpuset);
I had to look up the implementation of virBufferEscapeString()
to figure out that nodeset or cpuset possibly being NULL is
handled automatically by that function, which I found to be a
pretty surprising behavior. Not a problem with your patch
though :)
> @@ -1958,11 +1968,13 @@
> qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver,
> {
> virCapsPtr caps = NULL;
> char *nodeset;
> +char *cpuset;
> int ret = -1;
>
> nodeset = virXPathString("string(./numad/@nodeset)", ctxt);
> +cpuset = virXPathString("string(./numad/@cpuset)", ctxt);
>
> -if (!nodeset)
> +if (!nodeset && !cpuset)
> return 0;
I don't think you want this hunk.
With the new condition, you will perform an early exit only
if both nodeset and cpuset are NULL; if nodeset is NULL but
cpuset isn't, the first call to virBitmapParse() a few lines
below will error out.
But leaving the condition alone makes sure all scenarios are
handled successfully.
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/6] qemu: separate virQEMUCapsInitCached out of virQEMUCapsNewForBinaryInternal
On Thu, Jul 20, 2017 at 01:32:36PM +0200, Jiri Denemark wrote: > On Wed, Jul 19, 2017 at 17:09:51 +0200, Pavel Hrdina wrote: > > Preparation for switching to virFileCache where there are two callbacks, > > one to get a new data and second one to load a cached data. > > > > This also removes virQEMUCapsReset which is no longer required. > > > > Signed-off-by: Pavel Hrdina > > --- > > src/qemu/qemu_capabilities.c | 143 > > +++ > > src/qemu/qemu_capspriv.h | 1 - > > tests/qemucapsprobe.c| 2 +- > > 3 files changed, 63 insertions(+), 83 deletions(-) > > Reviewed-by: Jiri Denemark I'll push the series shortly. Thanks, Pavel signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc : fix a access for null pointer
On 07/15/2017 05:00 PM, Peng Hao wrote:
> virNetSocketRemoveIOCallback get sock's ObjectLock and will call
> virNetSocketEventFree. virNetSocketEventFree may be free sock
> object and virNetSocketRemoveIOCallback will access a null pointer
> in release sock's ObjectLock.
>
> Signed-off-by: Liu Yun
> Signed-off-by: Peng Hao
> ---
> src/rpc/virnetsocket.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
I don't think this can work.
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index d228c8a..8b550e8 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -2140,14 +2140,12 @@ static void virNetSocketEventFree(void *opaque)
> virFreeCallback ff;
> void *eopaque;
>
> -virObjectLock(sock);
> ff = sock->ff;
> eopaque = sock->opaque;
> sock->func = NULL;
> sock->ff = NULL;
> sock->opaque = NULL;
> -virObjectUnlock(sock);
I think we need the lock here. This function is called from the event
loop thread. So even if virNetSocketUpdateIOCallback() locks the @socket
this code can see it unlocked. Or locked. But the crucial part is it's
modifying the object and thus should have lock held.
> -
> +
> if (ff)
> ff(eopaque);
>
> @@ -2207,6 +2205,7 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock,
>
> void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
> {
> +virObjectRef(sock);
> virObjectLock(sock);
I think this is what actually fixes your problem. However, I also think
it introduces uneven ratio of ref:unref calls.
>
> if (sock->watch < 0) {
> @@ -2220,6 +2219,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
> sock->watch = -1;
>
> virObjectUnlock(sock);
> +virObjectRef(sock);
It definitely does so because you ref twice. Anyway, do you perhaps have
a backtrace to share?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix a couple of resource leaks
On Thu, Jul 20, 2017 at 07:00:38AM -0400, John Ferlan wrote: > Resource a couple Coverity found resource leaks > > John Ferlan (2): > daemon: Don't conditionally free @origErr in daemonStreamEvent > tests: Free @fakerootdir in error path > > daemon/stream.c | 2 +- > tests/qemumemlocktest.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) ACK series. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/6] qemu: separate virQEMUCapsInitCached out of virQEMUCapsNewForBinaryInternal
On Wed, Jul 19, 2017 at 17:09:51 +0200, Pavel Hrdina wrote: > Preparation for switching to virFileCache where there are two callbacks, > one to get a new data and second one to load a cached data. > > This also removes virQEMUCapsReset which is no longer required. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_capabilities.c | 143 > +++ > src/qemu/qemu_capspriv.h | 1 - > tests/qemucapsprobe.c| 2 +- > 3 files changed, 63 insertions(+), 83 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] qemu: domain: Extract parsing and formatting of priv->autoNodeset
On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote: > static int > +qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver, > + qemuDomainObjPrivatePtr priv, > + xmlXPathContextPtr ctxt) I think you should invert the first and last arguments to keep the signature closer to that of the calling function. Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1] numa: compute and set matching vcpus for numa domains
From: Wim ten Have The QEMU driver can erroneously allocate more vpus to a domain than there are cpus in the domain if the element is used to describe element topology. Wim ten Have (1): numa: compute and set matching vcpus for numa domains docs/formatdomain.html.in | 9 - src/conf/domain_conf.c| 14 +++--- 2 files changed, 19 insertions(+), 4 deletions(-) -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: fix an improper comment
On Thu, Jul 20, 2017 at 05:01:57PM +0800, Chen Hanxiao wrote: > From: Chen Hanxiao > > Actually we use virConnectNodeDeviceEventGenericCallback. > > Signed-off-by: Chen Hanxiao Slightly adjusted the commit subject and pushed. Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/12] qemu: support chardev for all machvirt config
On Thu, Jul 20, 2017 at 1:16 PM, Andrea Bolognani wrote: > On Thu, 2017-07-20 at 12:55 +0200, Christoffer Dall wrote: >> > Christoffer, >> > >> > now that the series has been merged, would you mind updating >> > >> > https://bugs.linaro.org/show_bug.cgi?id=2777 >> > >> > with the relevant information? >> >> I have updated the bug documenting that this works for me with upstream >> now. > > Thanks :) > >> Which would be the next libvirt release number that contains this fix, >> and when is that expected to be released? > > libvirt 3.6.0, scheduled to be released in the first week > of August, will contain the fix. > Thanks, I'll update the ticket with this info. >> I think my colleague tried to backport the changes to the libvirt >> version used in some Linaro packaged software, but it didn't work for >> him. Do you have any straight-forward thoughts on why that wouldn't >> work? > > Can't think of anything specific, but when I backported > the fixes to libvirt 3.2.0 there were quite a few conflicts > to take care of, so if Linaro is using a much older libvirt > version I can imagine the backport would get even trickier. > Is the 3.2.0 backport available somewhere public? Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/12] qemu: support chardev for all machvirt config
On Thu, 2017-07-20 at 12:55 +0200, Christoffer Dall wrote: > > Christoffer, > > > > now that the series has been merged, would you mind updating > > > > https://bugs.linaro.org/show_bug.cgi?id=2777 > > > > with the relevant information? > > I have updated the bug documenting that this works for me with upstream > now. Thanks :) > Which would be the next libvirt release number that contains this fix, > and when is that expected to be released? libvirt 3.6.0, scheduled to be released in the first week of August, will contain the fix. > I think my colleague tried to backport the changes to the libvirt > version used in some Linaro packaged software, but it didn't work for > him. Do you have any straight-forward thoughts on why that wouldn't > work? Can't think of anything specific, but when I backported the fixes to libvirt 3.2.0 there were quite a few conflicts to take care of, so if Linaro is using a much older libvirt version I can imagine the backport would get even trickier. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] 答复: [PATCH] fix 1 << -1 at JOB_MASK macro
On Tue, Jul 18, 2017 at 02:04:27 +, Caoxinhua wrote: > When we start a vm, we call JOB_MASK(QEMU_JOB_NONE) at follow backtrace > > Breakpoint 1, qemuDomainObjPrivateXMLFormat (buf=0x7f8078fcd510, > vm=0x7f808c2c7d70) at qemu/qemu_domain.c:1779 > 1779if (!qemuDomainTrackJob(job)) > (gdb) p job > $1 = QEMU_JOB_NONE > (gdb) bt > #0 qemuDomainObjPrivateXMLFormat (buf=0x7f8078fcd510, vm=0x7f808c2c7d70) at > qemu/qemu_domain.c:1779 > #1 0x7f80899e7988 in virDomainObjFormat (xmlopt=0x7f808c275c60, > obj=obj@entry=0x7f808c2c7d70, caps=0x7f808c25b450, flags=flags@entry=625) > at conf/domain_conf.c:24936 > #2 0x7f80899e7a5c in virDomainSaveStatus (xmlopt=, > statusDir=0x7f808c28de30 "/var/run/libvirt/qemu", > obj=obj@entry=0x7f808c2c7d70, caps=) at > conf/domain_conf.c:25149 > #3 0x7f806f7d8840 in qemuProcessLaunch (conn=conn@entry=0x7f805c0047c0, > driver=driver@entry=0x7f808c2d1530, vm=vm@entry=0x7f808c2c7d70, > asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_START, > incoming=incoming@entry=0x0, snapshot=snapshot@entry=0x0, > vmop=vmop@entry=VIR_NETDEV_VPORT_PROFILE_OP_CREATE, flags=flags@entry=17) > at qemu/qemu_process.c:5757 Yes, you're right. ACK to the patch. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] util: bitmap: Modify virBitmapSubtract to virBitmapIntersect
On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote:
> Since virBitmapSubtract is unused modify it to perform bitmap
> intersection.
Normally I would ask you to remove the old function in
one patch and introduce the new one in another, but I
can see you manage to reduce the test suite churn by
squeezing both changes together, so I'm okay with it :)
> diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
> index e5305d022..882c71544 100644
> --- a/tests/virbitmaptest.c
> +++ b/tests/virbitmaptest.c
> @@ -589,7 +589,7 @@ test11(const void *opaque)
> virBitmapParse(data->res, &resmap, 256) < 0)
> goto cleanup;
>
> -virBitmapSubtract(amap, bmap);
> +virBitmapIntersect(amap, bmap);
>
> if (!virBitmapEqual(amap, resmap)) {
> fprintf(stderr, "\n bitmap subtraction failed: '%s'-'%s'!='%s'\n",
You'll want to update the error message as well.
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Fix a couple of resource leaks
Resource a couple Coverity found resource leaks John Ferlan (2): daemon: Don't conditionally free @origErr in daemonStreamEvent tests: Free @fakerootdir in error path daemon/stream.c | 2 +- tests/qemumemlocktest.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] daemon: Don't conditionally free @origErr in daemonStreamEvent
Commit id '0fe4aa149' added @origErr, but since it's assigned outside
the if condition, the free should be outside as well.
Found by Coverity
Signed-off-by: John Ferlan
---
daemon/stream.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/daemon/stream.c b/daemon/stream.c
index 5077ac8..49682f1 100644
--- a/daemon/stream.c
+++ b/daemon/stream.c
@@ -239,7 +239,6 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
virStreamAbort(stream->st);
if (origErr && origErr->code != VIR_ERR_OK) {
virSetError(origErr);
-virFreeError(origErr);
} else {
if (events & VIR_STREAM_EVENT_HANGUP)
virReportError(VIR_ERR_RPC,
@@ -248,6 +247,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
virReportError(VIR_ERR_RPC,
"%s", _("stream had I/O failure"));
}
+virFreeError(origErr);
msg = virNetMessageNew(false);
if (!msg) {
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] tests: Free @fakerootdir in error path
Commit id 'dd9b29dad' added this new variable, but didn't free it in
one instance where status was returned to the caller.
Found by Coverity
Signed-off-by: John Ferlan
---
tests/qemumemlocktest.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c
index 268563d..66ebabb 100644
--- a/tests/qemumemlocktest.c
+++ b/tests/qemumemlocktest.c
@@ -80,8 +80,10 @@ mymain(void)
if (!abs_top_srcdir)
abs_top_srcdir = abs_srcdir "/..";
-if (qemuTestDriverInit(&driver) < 0)
+if (qemuTestDriverInit(&driver) < 0) {
+VIR_FREE(fakerootdir);
return EXIT_FAILURE;
+}
driver.privileged = true;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/12] qemu: support chardev for all machvirt config
Hi Andrea, On Wed, Jul 19, 2017 at 03:07:25PM +0200, Andrea Bolognani wrote: > On Tue, 2017-07-04 at 11:25 +0200, Christoffer Dall wrote: > > I tried with two mainline QEMU versions (the default old one packaged > > with Debian and a recent upstream commit) and this series seems to work > > as intended, so: > > > > Tested-by: Christoffer Dall > > Christoffer, > > now that the series has been merged, would you mind updating > > https://bugs.linaro.org/show_bug.cgi?id=2777 > > with the relevant information? I have updated the bug documenting that this works for me with upstream now. Which would be the next libvirt release number that contains this fix, and when is that expected to be released? I think my colleague tried to backport the changes to the libvirt version used in some Linaro packaged software, but it didn't work for him. Do you have any straight-forward thoughts on why that wouldn't work? Thanks, -Christoffer -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1] numa: compute and set matching vcpus for numa domains
From: Wim ten Have
The QEMU driver can erroneously allocate more vpus to a domain
than there are cpus in the domain if the element is used
to describe element topology. Fix this by calculating
the number of cpus described in the element of a
element and comparing it to the number of vcpus. If the number
of vcpus is greater than the number of cpus, cap the number of
vcpus to the number of cpus.
This patch also adds a small libvirt documentation update under
the "CPU Allocation" section.
Signed-off-by: Wim ten Have
---
docs/formatdomain.html.in | 9 -
src/conf/domain_conf.c| 14 +++---
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 07208ee..3c85307 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -501,7 +501,14 @@
vcpu
The content of this element defines the maximum number of virtual
CPUs allocated for the guest OS, which must be between 1 and
-the maximum supported by the hypervisor.
+the maximum supported by the hypervisor. If a numa
+element is contained within the cpu element, the
+number of virtual CPUs to be allocated is compared with the sum
+of the cpus attributes across the cell
+elements within the numa element. If the number of
+virtual CPUs is greater than the sum of the cpus
+attributes, the number of virtual CPUs is capped at sum of the
+cpus attributes.
cpuset
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 958a5b7..73d7f4f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3844,12 +3844,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
unsigned long long numaMemory = 0;
unsigned long long hotplugMemory = 0;
-/* Attempt to infer the initial memory size from the sum NUMA memory sizes
- * in case ABI updates are allowed or the element wasn't
specified */
+/* Attempt to infer the initial memory size from the sum NUMA memory
+ * sizes in case ABI updates are allowed or the element
+ * wasn't specified. Also cap the vcpu count to the sum of NUMA cpus
+ * allocated for all nodes. */
if (def->mem.total_memory == 0 ||
parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE ||
-parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION)
+parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION) {
+unsigned int numaVcpus = 0;
+
+if ((numaVcpus = virDomainNumaGetCPUCountTotal(def->numa)))
+virDomainDefSetVcpus(def, numaVcpus);
+
numaMemory = virDomainNumaGetMemorySize(def->numa);
+}
/* calculate the sizes of hotplug memory */
for (i = 0; i < def->nmems; i++)
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1] numa: compute and set matching vcpus for numa domains
On Thu, Jul 20, 2017 at 11:29:26AM +0200, Wim Ten Have wrote:
> From: Wim ten Have
>
> The QEMU driver can erroneously allocate more vpus to a domain
> than there are cpus in the domain if the element is used
> to describe element topology. Fix this by calculating
> the number of cpus described in the element of a
> element and comparing it to the number of vcpus. If the number
> of vcpus is greater than the number of cpus, cap the number of
> vcpus to the number of cpus.
>
> This patch also adds a small libvirt documentation update under
> the "CPU Allocation" section.
>
> Signed-off-by: Wim ten Have
> ---
> docs/formatdomain.html.in | 9 -
> src/conf/domain_conf.c| 14 +++---
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 07208ee..3c85307 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -501,7 +501,14 @@
>vcpu
>The content of this element defines the maximum number of virtual
> CPUs allocated for the guest OS, which must be between 1 and
> -the maximum supported by the hypervisor.
> +the maximum supported by the hypervisor. If a numa
> +element is contained within the cpu element, the
> +number of virtual CPUs to be allocated is compared with the sum
> +of the cpus attributes across the cell
> +elements within the numa element. If the number of
> +virtual CPUs is greater than the sum of the cpus
> +attributes, the number of virtual CPUs is capped at sum of the
> +cpus attributes.
>
> cpuset
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 958a5b7..73d7f4f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3844,12 +3844,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
> unsigned long long numaMemory = 0;
> unsigned long long hotplugMemory = 0;
>
> -/* Attempt to infer the initial memory size from the sum NUMA memory
> sizes
> - * in case ABI updates are allowed or the element wasn't
> specified */
> +/* Attempt to infer the initial memory size from the sum NUMA memory
> + * sizes in case ABI updates are allowed or the element
> + * wasn't specified. Also cap the vcpu count to the sum of NUMA cpus
> + * allocated for all nodes. */
> if (def->mem.total_memory == 0 ||
> parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE ||
> -parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION)
> +parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION) {
> +unsigned int numaVcpus = 0;
> +
> +if ((numaVcpus = virDomainNumaGetCPUCountTotal(def->numa)))
> +virDomainDefSetVcpus(def, numaVcpus);
> +
> numaMemory = virDomainNumaGetMemorySize(def->numa);
> +}
AFAICT, this scenario is simply a user configuration mistake, and so we
should report an error message to them to fix it.
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 v3 00/16] qemu: migration: show disks stats for nbd migration
On 14.07.2017 12:20, Jiri Denemark wrote: > On Fri, Jul 14, 2017 at 09:51:48 +0300, Nikolay Shirokovskiy wrote: >> ping > > Oops, I completely forgot about this series. But since it is a few > months old, could you resend the series after rebasing it to current > libvirt? I promise to review them while they are fresh. > > Jirka > Hi, Jiri. Unfortunately your email was lost in spam somehow and I would not even notice it if not Max Nestratov. I'm going to take a long vacation in a few days thus let's postpone the review till the moment I'll return back. Nikolay -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH go-xml] Add support for NAT in network forward
On Wed, Jul 12, 2017 at 01:26:03PM +0200, Thomas Hipp wrote: > Add support for NAT in network forward, and add test code. > > Signed-off-by: Thomas Hipp > --- > network.go | 20 ++-- > network_test.go | 31 +-- > 2 files changed, 43 insertions(+), 8 deletions(-) ACK & pushed 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 go-xml] Add support for DNS in network
On Wed, Jul 12, 2017 at 03:28:13PM +0200, Thomas Hipp wrote: > Add support for DNS in network, and add test code. > > Signed-off-by: Thomas Hipp > --- > network.go | 39 ++ > network_test.go | 65 > - > 2 files changed, 99 insertions(+), 5 deletions(-) ACK & pushed 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 RESEND] qemu: shared disks with cache=directsync should be safe for migration
On Sat, Jul 15, 2017 at 11:01:25PM +0800, Peng Hao wrote: > From: Hao Peng > > At present shared disks can be migrated with either readonly or cache=none. > But > cache=directsync should be safe for migration, because both cache=directsync > and cache=none > don't use the host page cache, and cache=direct write through qemu block > layer cache. > > Signed-off-by: Peng Hao > Reviewed-by: Wang Yechao > --- > src/qemu/qemu_migration.c | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrange and pushed to git master Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] nodedev: fix an improper comment
From: Chen Hanxiao
Actually we use virConnectNodeDeviceEventGenericCallback.
Signed-off-by: Chen Hanxiao
---
include/libvirt/libvirt-nodedev.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-nodedev.h
b/include/libvirt/libvirt-nodedev.h
index cd3f237..25e8724 100644
--- a/include/libvirt/libvirt-nodedev.h
+++ b/include/libvirt/libvirt-nodedev.h
@@ -142,7 +142,7 @@ int virNodeDeviceDestroy
(virNodeDevicePtr dev);
*/
typedef enum {
VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE = 0, /*
virConnectNodeDeviceEventLifecycleCallback */
-VIR_NODE_DEVICE_EVENT_ID_UPDATE = 1, /*
virConnectNodeDeviceEventUpdateCallback */
+VIR_NODE_DEVICE_EVENT_ID_UPDATE = 1, /*
virConnectNodeDeviceEventGenericCallback */
# ifdef VIR_ENUM_SENTINELS
VIR_NODE_DEVICE_EVENT_ID_LAST
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: Don't overwrite error of GetProcessLabel
On 07/20/2017 01:22 AM, Cole Robinson wrote: > Security impls of this function already raise errors, don't > overwrite them. > > Signed-off-by: Cole Robinson > --- > src/lxc/lxc_driver.c | 6 ++ > src/qemu/qemu_driver.c | 7 +-- > 2 files changed, 3 insertions(+), 10 deletions(-) Indeed. ACK. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] storage: Fix existing parent check for vHBA creation
On Wed, Jul 19, 2017 at 11:09:00AM -0400, John Ferlan wrote: > > > On 07/19/2017 10:21 AM, Erik Skultety wrote: > > On Wed, Jul 19, 2017 at 09:59:06AM -0400, John Ferlan wrote: > >> > >> > >> On 07/19/2017 07:38 AM, Erik Skultety wrote: > >>> On Tue, Jul 18, 2017 at 11:10:38AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1472277 > > Commit id '106930aaa' altered the order of checking for an existing > vHBA (e.g something created via nodedev-create functionality outside > of the storage pool logic) which inadvertantly broke the code to > decide whether to alter/force the fchost->managed field to be 'yes' > because the storage pool will be managing the created vHBA in order > to ensure when the storage pool is destroyed that the vHBA is also > destroyed. > >>> > >>> Right, I agree with this - unless the user explicitly states they want the > >>> pre-created vHBA to be managed, we don't force any setting. I was > >>> wondering > >>> though, what about a use case when the user wants the vHBA to be > >>> auto-created, > >>> but non-managed at the same time...(yeah, I know I'm stretching it a bit > >>> with > >>> these unlikely scenarios, but then, you'd surely agree, you've already > >>> seen > >>> some of similar sort and one can never expect what creative ways of > >>> defining > >>> the XML are there to be found :)) > >> > >> I think you're becoming the new vHBA expert ;-) > >> > >> In this case, I tell them to go see figure one or go read the docs. From > >> the storage pool page, managed description: "For configurations that do > >> not provide an already created vHBA from a 'virsh nodedev-create', > >> libvirt will set this property to "yes"." > >> > >>> > >>> I was also about to point out in the previous version, that I didn't like > >>> how > >>> complex virNodeDeviceCreateVport was getting - do wwnn and wwpn relate to > >>> a > >>> vHBA at all or is it a regular HBA, does the vHBA exist already or should > >>> we > >>> actually create and manage it. > >>> > >> > >> The wwnn/wwpn are the wwnn/wwpn used to create the vHBA. Typically, a > >> SAN admin will "assign" the pair (there's some specific rules about > >> them). If not provided for a storage pool, then it's an XML error. For a > >> nodedev it's possible to have libvirt generate a wwnn/wwpn, which yes, > >> is quite confusing. In doing so libvirt uses a specific base and adds to > >> it (see virRandomGenerateWWN and cover at least one eye). > >> > >> I agree in general about the "complexity" thing, but as time has gone on > >> requests for new ways to determine which parent to use has caused code > >> bloat. Complexity is a time factored calculation. When originally > >> implemented using "host#" for parent was perfectly fine, but then > >> someone realized that it should be "scsi_host#". Then someone said, that > >> "scsi_host#" wasn't good enough because it can change between reboots. > >> So parent by wwnn/wwpn was added. During the discussions for that > >> someone else said what about using the fabric_wwn in order to find a > >> parent. Oh and the "future" holds being able to define multiple vHBA's > >> because it's felt migration of domains using vHBA pools is going to need > >> more than one vHBA because on the target host using the same wwnn/wwpn > >> won't work (although I have doubts here, but haven't had the cycles to > >> investigate). > >> > >> If you're interested, go read the tail end of the wiki > >> (http://wiki.libvirt.org/page/NPIV_in_libvirt) - it'll show a bit of the > >> history of how things looked at one time. > >> > >> TBH: Sometimes I think QE reads the code and figures out a way to create > >> bugs based on assumptions the code makes rather than making sure the > >> intended use cases "work properly". Hence this whole need to know > >> whether the parent is the HBA or the vHBA. Why would *anyone* provide > >> the HBA parent wwnn/wwpn when it's perfectly fine to create a storage > >> pool of the HBA without it via: > >> > >> > >> > >> but no, someone wants to be inventive and think/believe: > >> > >> >> wwpn='HBA_wwpn'/> > >> > >> should work as well (where HBA_wwnn/wwpn are the wwnn/wwpn of scsi_host3 > >> in this example). > >> > >> The second XML isn't illegal, but because scsi_host3 has vHBA > > > > Well, I'd call it a misconfiguration, how can a device be a parent to > > itself? > > That's not what the attribute's supposed to serve and using it this way is a > > misuse - we should either ignore it in that case or return error. The > > storage > > pool won't start but it should never have in the first place and the XML def > > won't disappear in this case, so IMHO we could and probably should forbid > > it. > > > > Because it hasn't really been characterized as a misconfiguration > previously. I doubt anyone outside of QE has ever done something like > this as there's no reason to do so. If they want to use the HBA they'd > use
Re: [libvirt] [PATCH] LXC: set the right var to NULL
On Thu, 2017-07-20 at 10:02 +0800, Chen Hanxiao wrote: > From: Chen Hanxiao > > For attaching hosdev, we should set dev->data.hostdev > rather than dev->data.disk > > Signed-off-by: Chen Hanxiao > --- > src/lxc/lxc_driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index 652e9cb..6a9c528 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -4358,7 +4358,7 @@ lxcDomainAttachDeviceLive(virConnectPtr conn, > case VIR_DOMAIN_DEVICE_HOSTDEV: > ret = lxcDomainAttachDeviceHostdevLive(driver, vm, dev); > if (!ret) > -dev->data.disk = NULL; > +dev->data.hostdev = NULL; > break; > > default: ACK and pushed -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] util: uri: use unsigned numbers for port
On Thu, Jul 20, 2017 at 10:00:22 +0200, Peter Krempa wrote: > Peter Krempa (2): > qemu: command: Remove qemuNetworkDriveGetPort > util: uri: Convert port number to unsigned integer Self NACK. I'll repost this later with Michal's cleanup patch, since I'll probably change the first patch along with fixing his patch. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] util: uri: use unsigned numbers for port
Peter Krempa (2): qemu: command: Remove qemuNetworkDriveGetPort util: uri: Convert port number to unsigned integer src/qemu/qemu_command.c | 22 +- src/util/viruri.h | 2 +- 2 files changed, 6 insertions(+), 18 deletions(-) -- 2.13.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] util: uri: Convert port number to unsigned integer
Negative ports don't make sense so use a unsigned integer.
---
src/qemu/qemu_command.c | 3 +--
src/util/viruri.h | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b994940a2..f7858302b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -809,8 +809,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
goto cleanup;
if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
-if (virStrToLong_i(src->hosts->port, NULL, 10, &uri->port) < 0 ||
-uri->port < 0) {
+if (virStrToLong_uip(src->hosts->port, NULL, 10, &uri->port) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse port number '%s'"),
src->hosts->port);
diff --git a/src/util/viruri.h b/src/util/viruri.h
index 1e53abb0b..7850c38c2 100644
--- a/src/util/viruri.h
+++ b/src/util/viruri.h
@@ -42,7 +42,7 @@ struct _virURI {
char *scheme; /* the URI scheme */
char *server; /* the server part */
char *user; /* the user part */
-int port; /* the port number */
+unsigned int port; /* the port number */
char *path; /* the path string */
char *query;/* the query string */
char *fragment; /* the fragment string */
--
2.13.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: command: Remove qemuNetworkDriveGetPort
Now that the function is only parsing the string to a number, move it's
contents to the only caller.
---
src/qemu/qemu_command.c | 23 ++-
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6ac26af3e..b994940a2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -491,22 +491,6 @@ qemuSafeSerialParamValue(const char *value)
}
-static int
-qemuNetworkDriveGetPort(const char *port)
-{
-int ret = 0;
-
-if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse port number '%s'"),
- port);
-return -1;
-}
-
-return ret;
-}
-
-
/**
* qemuBuildSecretInfoProps:
* @secinfo: pointer to the secret info object
@@ -825,8 +809,13 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
goto cleanup;
if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
-if ((uri->port = qemuNetworkDriveGetPort(src->hosts->port)) < 0)
+if (virStrToLong_i(src->hosts->port, NULL, 10, &uri->port) < 0 ||
+uri->port < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse port number '%s'"),
+ src->hosts->port);
goto cleanup;
+}
if (VIR_STRDUP(uri->scheme,
virStorageNetProtocolTypeToString(src->protocol)) < 0)
--
2.13.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virStorageNetHostDef: Turn @port into integer
On Wed, Jul 19, 2017 at 17:58:15 +0200, Peter Krempa wrote:
> On Wed, Jul 19, 2017 at 17:26:27 +0200, Michal Privoznik wrote:
> > Currently, @port is type of string. Well, that's overkill and
> > waste of memory. Port is always an integer. Use it as such.
> >
> > Signed-off-by: Michal Privoznik
> > ---
> > src/conf/domain_conf.c| 25 ++--
> > src/libxl/libxl_conf.c| 2 +-
> > src/qemu/qemu_block.c | 2 +-
> > src/qemu/qemu_command.c | 28 ++---
> > src/qemu/qemu_parse_command.c | 33 +++---
> > src/storage/storage_backend_gluster.c | 17 ++---
> > src/storage/storage_driver.c | 7 +--
> > src/util/virstoragefile.c | 113
> > +-
> > src/util/virstoragefile.h | 4 +-
> > src/xenconfig/xen_xl.c| 2 +-
> > 10 files changed, 130 insertions(+), 103 deletions(-)
[...]
> > diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> > index 98992e04a..934504806 100644
> > --- a/src/util/virstoragefile.h
> > +++ b/src/util/virstoragefile.h
> > @@ -155,7 +155,7 @@ typedef struct _virStorageNetHostDef
> > virStorageNetHostDef;
> > typedef virStorageNetHostDef *virStorageNetHostDefPtr;
> > struct _virStorageNetHostDef {
> > char *name;
> > -char *port;
> > +int port;
>
> If you want to be precise ... have you ever seen negative ports?
>
> > int transport; /* virStorageNetHostTransport */
> > char *socket; /* path to unix socket */
> > };
>
> This will require a lot of fixing since you blindly copied the check
> that also checks that the port is not less than 0.
I'll send two patches that I have on a branch that convert 'port' in
virURI to unsigned, which will possibly avoid you from typecasting.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virStorageNetHostDef: Turn @port into integer
On 07/19/2017 05:58 PM, Peter Krempa wrote:
> On Wed, Jul 19, 2017 at 17:26:27 +0200, Michal Privoznik wrote:
>> Currently, @port is type of string. Well, that's overkill and
>> waste of memory. Port is always an integer. Use it as such.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/conf/domain_conf.c| 25 ++--
>> src/libxl/libxl_conf.c| 2 +-
>> src/qemu/qemu_block.c | 2 +-
>> src/qemu/qemu_command.c | 28 ++---
>> src/qemu/qemu_parse_command.c | 33 +++---
>> src/storage/storage_backend_gluster.c | 17 ++---
>> src/storage/storage_driver.c | 7 +--
>> src/util/virstoragefile.c | 113
>> +-
>> src/util/virstoragefile.h | 4 +-
>> src/xenconfig/xen_xl.c| 2 +-
>> 10 files changed, 130 insertions(+), 103 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9320794de..e8fdaa36f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>
> [...]
>
>> @@ -6486,7 +6487,15 @@ virDomainStorageHostParse(xmlNodePtr node,
>> goto cleanup;
>> }
>>
>> -host.port = virXMLPropString(child, "port");
>> +port = virXMLPropString(child, "port");
>> +if (port &&
>> +(virStrToLong_i(port, NULL, 10, &host.port) < 0 ||
>> + host.port < 0)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("failed to parse port number '%s'"),
>> + port);
>> +goto cleanup;
>> +}
>> }
>
> 'port' is leaked when multiple element are passed.
>
>>
>> if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0)
>
>
>
>> @@ -14909,7 +14919,7 @@
>> virDomainHostdevMatchSubsysSCSIiSCSI(virDomainHostdevDefPtr first,
>> &second->source.subsys.u.scsi.u.iscsi;
>>
>> if (STREQ(first_iscsisrc->hosts[0].name,
>> second_iscsisrc->hosts[0].name) &&
>> -STREQ(first_iscsisrc->hosts[0].port,
>> second_iscsisrc->hosts[0].port) &&
>> +first_iscsisrc->hosts[0].port == second_iscsisrc->hosts[0].port &&
>> STREQ(first_iscsisrc->path, second_iscsisrc->path))
>> return 1;
>> return 0;
>
>
>> @@ -22255,7 +22267,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> if (scsisrc->protocol ==
>> VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
>> virBufferAddLit(buf, "> virBufferEscapeString(buf, " name='%s'",
>> iscsisrc->hosts[0].name);
>> -virBufferEscapeString(buf, " port='%s'",
>> iscsisrc->hosts[0].port);
>> +if (iscsisrc->hosts[0].port)
>> +virBufferAsprintf(buf, " port='%d'",
>> iscsisrc->hosts[0].port);
>> virBufferAddLit(buf, "/>\n");
>> } else {
>> virBufferAsprintf(buf, "\n",
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index a85bc71d2..1b20fb906 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -706,7 +706,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
>> virBufferAsprintf(&buf, "%s", src->hosts[i].name);
>>
>> if (src->hosts[i].port)
>> -virBufferAsprintf(&buf, "\\:%s", src->hosts[i].port);
>> +virBufferAsprintf(&buf, "\\:%d", src->hosts[i].port);
>> }
>> }
>>
>> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
>> index 93124c5ba..3d64528a8 100644
>> --- a/src/qemu/qemu_block.c
>> +++ b/src/qemu/qemu_block.c
>> @@ -465,7 +465,7 @@
>> qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src,
>> if (virJSONValueObjectCreate(&server,
>> "s:type", transport,
>> "s:host", host->name,
>> - "s:port", host->port,
>> + "i:port", host->port,
>> NULL) < 0)
>> goto cleanup;
>> break;
>
>
>
>> @@ -897,8 +880,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>>
>> switch (src->hosts->transport) {
>> case VIR_STORAGE_NET_HOST_TRANS_TCP:
>> -virBufferStrcat(&buf, src->hosts->name, ":",
>> -src->hosts->port, NULL);
>> +virBufferAsprintf(&buf, "%s:%d", src->hosts->name,
>> src->hosts->port);
>
> Line too long
>
>> @@ -953,9 +935,9 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>> if (virAsprintf(&ret, "sheepdog:%s", src->path) < 0)
>> goto cleanup;
>> } else if (src->nhosts == 1) {
>> -
Re: [libvirt] [PATCH] conf: Fix backwards migration of pSeries guests
On Wed, Jul 19, 2017 at 16:41:54 +0200, Andrea Bolognani wrote:
> On Wed, 2017-07-19 at 15:33 +0200, Peter Krempa wrote:
> > > Recent commits made it so that pci-root controllers for
> >
> > Did we release this?
>
> Nope, it will be in the next release.
>
> > > +/* Don't format the model name for PHBs when migrating so that
> > > + * guests that only use the default one can be migrated to older
> > > + * libvirt version which don't know about PHBs at all */
> > > +if (virDomainControllerIsPCIHostBridge(def) &&
> >
> > This function has a confusing name, since it's explicitly checking for a
> > pSeries specific thing but the name does not really imply that.
>
> Point taken about pSeries not being mentioned in the
> name, but PHBs are a pSeries-specific concept so it didn't
> really occur to me at the time.
So I thin that function should be renamed, because the "PCI Host Bridge"
name sounds really generic.
>
> > > +flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) {
> > > +formatModelName = false;
> > > +}
> >
> > Won't this suppress the formatting of the type even if the user
> > specified it explicitly? In that case we should not suppress it IMO.
> > It will cause problems once there's a different model available.
>
> How would the code know whether the model was set manually
> by the user or automatically by libvirt?
You can record it in a boolean.
> In any case, that made me realize that not sending the model,
> even if automatically filled in, could cause issues in the
> future if a new model is added and becomes the default, as
> the guest ABI would not be preserved during migration then.
That is the main reason to fill all the values in right away. Since
there apparently was a period, where a default would be used, but not
recorded, it needs some trickery unfortunately.
In such case you basically standardize, that the now-filled-in default
model (which can't ever change) if it was not provided by the user is to
be dropped from the migratable XML.
Once you start assign a new default model, that then needs to be
explicitly sent over, so that the migration will not be successufl
unless the destination is able to use the new model.
This means basically, that a missing model name becomes assigned to a
particular value.
> Any help on how to deal with this? I'm clearly not very good
> at the whole migration thing :(
>
> > If the term 'PCI host bridge' is a p-series specific thing, we should
> > make it more obvious in the code since it's too generic sounding without
> > knowledge of the context.
>
> Maybe I can change it to virDomainControllerIsPSeriesPHB()?
> That would require passing in the virDomainDef though.
That looks way better. It's clear at first glance that it's not a
general thing. (At least for the lazy not reading the documentation for
the function)
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: blockcopy: reuse storage driver APIs to pre-create copy target
On Wed, Jul 19, 2017 at 17:44:16 -0400, John Ferlan wrote:
>
>
> On 07/11/2017 11:46 AM, Peter Krempa wrote:
> > Rather than using the local-file only implementation 'qemuOpenFile'
> > switch to the imagelabel aware storage driver implementation.
> > ---
> > src/qemu/qemu_driver.c | 21 ++---
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index d00166f23..48dc5e5cc 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
[...]
> > @@ -16880,8 +16885,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
> > vm->def->name);
> >
> > endjob:
> > -if (need_unlink && unlink(mirror->path))
> > -VIR_WARN("unable to unlink just-created %s", mirror->path);
> > +if (need_unlink && virStorageFileUnlink(mirror))
>
> ...Unlink(mirror) < 0)
>
> > +VIR_WARN("%s", _("unable to remove just-created copy target"));
> > +virStorageFileDeinit(mirror);
> > +virStorageSourceFree(mirror);
>
> Isn't this duplicitous with the same in the cleanup: label
Yes indeed. It would even crash on the error paths if 'mirror' was not
consumed into the disk definition structure. Good catch.
>
> > qemuDomainObjEndJob(driver, vm);
> > if (monitor_error) {
> > virSetError(monitor_error);
> >
>
> with fixups...
>
> Reviewed-by: John Ferlan
Thanks
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/6] qemu: don't pass qemuctime into virQEMUCapsIsValid
On Wed, Jul 19, 2017 at 17:09:50 +0200, Pavel Hrdina wrote: > It's not required and following patches will change the code. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_capabilities.c | 27 +++ > 1 file changed, 11 insertions(+), 16 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 1/3] nodedev: Fix usage of virNodeDeviceObjListGetParentHost
> >> > >> @@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) > >> int ret = -1; > >> virNodeDeviceObjPtr obj = NULL; > >> virNodeDeviceDefPtr def; > >> +char *name = NULL; > >> +char *parent = NULL; > >> +char *parent_wwnn = NULL; > >> +char *parent_wwpn = NULL; > >> +char *parent_fabric_wwn = NULL; > >> char *wwnn = NULL, *wwpn = NULL; > >> int parent_host = -1; > >> > >> @@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) > >> if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) > >> goto cleanup; > >> > >> -/* virNodeDeviceGetParentHost will cause the device object's lock > >> - * to be taken, so grab the object def which will have the various > >> - * fields used to search (name, parent, parent_wwnn, parent_wwpn, > >> - * or parent_fabric_wwn) and drop the object lock. */ > >> +/* Because we're about to release the lock and thus run into a race > >> + * possibility (however improbably) with a udevAddOneDevice change > >> + * event which would essentially free the existing @def (obj->def) and > >> + * replace it with something new, we need to save off and use the > >> + * various fields that virNodeDeviceObjListGetParentHost will use */ > > > > And as I originally suggested I would allocate a new temporary @def > > structure, > > initialize it, pass it to the *GetParentHost method like nothing out of the > > ordinary happened and mentioned in the commentary why we've done it that way > > (which you already do in this patch). > > > > So you'd prefer some sort of virNodeDeviceDefCopy function be created? > Or use VIR_ALLOC(def) and copy in the 5 fields only to then > virNodeDeviceDefFree() it afterwards? Yeah, exactly, I'm aware of those unused extra field, I don't know it just looked more appealing and transparent to me, again, matter of preference, the way I see it, you store/pass it in a much more compact way with the added benefit of other, in this case currently unused, fields, should virNodeDeviceObjListGetParentHost ever need them. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
