Re: [libvirt] [jenkins-ci PATCH] guests: List all known guests in inventory
On Wed, Jul 11, 2018 at 12:59:13PM +0200, Andrea Bolognani wrote: > Users will probably want to only work with a subset of > guests but, like any other customization, that should be > handled with local tweaks. > > Signed-off-by: Andrea Bolognani > --- > guests/inventory | 4 > 1 file changed, 4 insertions(+) > > diff --git a/guests/inventory b/guests/inventory > index 6ea43d9..9838d7c 100644 > --- a/guests/inventory > +++ b/guests/inventory > @@ -1,8 +1,12 @@ > libvirt-centos-7 > libvirt-debian-8 > libvirt-debian-9 > +libvirt-debian-sid > libvirt-fedora-27 > libvirt-fedora-28 > libvirt-fedora-rawhide > libvirt-freebsd-10 > libvirt-freebsd-11 > +libvirt-freebsd-current what does ^this map to? > +libvirt-ubuntu-16 > +libvirt-ubuntu-18 shouldn't ubuntu 14 be mentioned here too? Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/9] extend virsh domstate to show additional information
On Wed, Jul 11, 2018 at 01:40:31PM +0200, Bjoern Walk wrote: Jiri Denemark [2018-07-11, 01:17PM +0200]: On Wed, Jul 11, 2018 at 12:49:13 +0200, Bjoern Walk wrote: > This patch series introduces the ability to save additional information > for the domain state and exposes this information in virsh domstate. > > For example in the case of QEMU guest panic events, we can provide additional > information like the crash reason or register state of the domain. This > information usually gets logged in the domain log but for debugging it is > useful to have it accessible from the client. Therefore, let's introduce a new > public API function, virDomainGetStateParams, an extensible version of > virDomainGetState, which returns the complete state of the domain, including > newly introduced additional information. > > Let's also extend virsh domstate and introduce a new parameter --info to show > the domain state, reason and additional information when available. > > virsh # domstate --info guest-1 > crashed (panicked: disabled-wait core='1' psw-mask='0x0010f146' \ > psw-addr='0x000200018000') I was thinking you introduced a new API with typed parameters so that you can return each part of the info string as a separate parameter with a defined semantics. But I was wrong, you're just adding a new opaque string with more details about the reason. In that case typed parameters don't actually bring any additional value, they just complicate the usage of the API. The following prototype would be much better The extensibility for this new API was more regarding for future additions of any state related information. Since libvirt does not have a deprecation model for APIs, whenever we would want to return additional information for the state (like in this case) a new API function has to be created. int virDomainGetState...(virDomainPtr domain, int *state, int *reason, char **info, unsigned int flags) On the other hand, is an opaque string really a good idea? It makes the additional info usable only for being shown to the user rather than being processed by an upper management layer. That's probably fine for crashed state, but perhaps other states would want to return something that is supposed to be processed. Maybe I'm just overthinking this, but I'd like to avoid having to add yet another API later. So the API could have the following prototype Sure, if machine readability is a goal this approach makes certainly more sense. On the other hand, the same information can be queried by a qemu-monitor-command call and retrieved in JSON format. This information here is aimed at human interaction, similar to the log output. It is also unclear which platforms/hypervisors would provide what information and mapping all of them to a virTypedParameter entry could result in a rather large list. Certainly no arguments against your objection, and if the overall concensus is that this is something we want, I can definitely rework the API. I'm not very particularly opinionated on this, but I think APIs should be machine-readable and CLI tools can convert that to human-readable format. You'll never know when a program will like to access that and having to tell anyone in the future that they need to parse a string is ugly IMHO. Also from the monitor you can get that information only if there is any QEMU running. I presume the state you are returning is saved somewhere along with the reason so that it can be provided later. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: schema: Add missing to vsock device
On Thu, Jul 12, 2018 at 02:28:17PM +0800, Han Han wrote: > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1600345 > > Signed-off-by: Han Han > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh.pod: Fix typo of nwfilter-binding-undefine
On Thu, Jul 12, 2018 at 12:40:09PM +0800, Han Han wrote: > Rename nwfilter-binding-undefine to nwfilter-binding-delete. > > Signed-off-by: Han Han > --- > tools/virsh.pod | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/virsh.pod b/tools/virsh.pod > index 4c6e21fc22..771e99591e 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -4828,7 +4828,7 @@ of the network filters directly. > Associate a network port with a network filter. The network filter backend > will immediately attempt to instantiate the filter rules on the port. > > -=item B I > +=item B I > > Disassociate a network port from a network filter. The network filter > backend will immediately tear down the filter rules that exist on the Reviewed-by: Erik Skultety I slightly reworded the commit message and pushed. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 04/11] qemu_monitor: Introduce qemuMonitorCPUModelInfoInit and qemuMonitorCPUModelInfoFreeContents
On Thu, Jul 12, 2018 at 11:35:23 -0500, Chris Venteicher wrote: > Quoting Jiri Denemark (2018-07-12 08:13:07) > > On Mon, Jul 09, 2018 at 22:56:48 -0500, Chris Venteicher wrote: > > > These forms modify contents of a qemuMonitorCPUModelInfo structure but > > > do not allocate or free the actual structure. > > > > > > Init - Initialize model name and empty properties within existing > > > structure > > > FreeContents - Free model name and properties within existing structure > > > > We call such function with "Clear" suffix, i.e., > > qemuMonitorCPUModelInfoClear. > > > > But it is usually used when we have a structure stored somewhere > > directly rather than having a pointer to it. And this was not the case > > so far and I don't think there's any reason to introduce a code which > > would need any of these functions. > > > > NACK to this patch. > > > Hi Jirka. I see what you mean about combining dependent patches... It would > be > helpful if this patch was coupled with the qemuMonitorGetCPUModelExpansion > patch. > > Could I get you're thoughts on the qemuMonitorGetCPUModelExpansion patch to > know > what to do with this one? > > Specifically, I seem to need to send a full CPUModelInfo to QEMU (w/ model > name > + properties) and get a full CPUModelInfo back from QEMU (again w/ model name > + > expanded properties). > > I implemented this by rewriting the contents (property list) of the > CPUModelInfo > structure that is passed in to qemuMonitorGetCPUModelExpansion. > > I do a similar thing in qemuMonitorCPUModelInfoRemovePropByBoolValue... I > rewrite the property list rather than allocating and returning a completely > new > CPUModelInfo for output. > > Is this consistent with other functions or would I be better off allocating > and > returning a completely new CPUModelInfo for the output? Yeah, that's the solution I was thinking about. With it you don't need these fragile FreeContents/Init functions and the function won't touch the input data unless it's finished at which point it will just free the input and replace it with the new structure. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/35] util: alloc: add macros for implementing automatic cleanup functionality
On Thu, Jul 12, 2018 at 11:44:16PM +0530, Sukrit Bhatnagar wrote:
> On Thu, 12 Jul 2018 at 22:09, Erik Skultety wrote:
> >
> > On Sat, Jun 30, 2018 at 02:30:05PM +0530, Sukrit Bhatnagar wrote:
> > > New macros are introduced which help in adding GNU C's cleanup
> > > attribute to variable declarations. Variables declared with these
> > > macros will have their allocated memory freed automatically when
> > > they go out of scope.
> > >
> > > Signed-off-by: Sukrit Bhatnagar
> > > ---
> > > src/util/viralloc.h | 44
> > > 1 file changed, 44 insertions(+)
> > >
> > > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > > index 69d0f90..5c1d0d5 100644
> > > --- a/src/util/viralloc.h
> > > +++ b/src/util/viralloc.h
> > > @@ -596,4 +596,48 @@ void virAllocTestInit(void);
> > > int virAllocTestCount(void);
> > > void virAllocTestOOM(int n, int m);
> > > void virAllocTestHook(void (*func)(int, void*), void *data);
> > > +
> > > +# define VIR_AUTOPTR_TYPE_NAME(type) type##AutoPtr
> > > +# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
> > > +
> > > +/**
> > > + * VIR_DEFINE_AUTOPTR_FUNC:
> > > + * @type: type of the variable to be freed automatically
> > > + * @func: cleanup function to be automatically called
> > > + *
> > > + * This macro defines a function for automatic freeing of
> > > + * resources allocated to a variable of type @type. This newly
> > > + * defined function works as a necessary wrapper around @func.
> > > + */
> > > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > > +typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
> >
> > So, it's not visible at first glance how ^this typedef is used...
> >
> > > +static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > > +{ \
> > > +if (*_ptr) \
> > > +(func)(*_ptr); \
> > > +*_ptr = NULL; \
> > > +} \
> >
> > ...therefore I'd write it explicitly as
> >
> > VIR_AUTOPTR_FUNC_NAME(type)(VIR_AUTOPTR_TYPE_NAME(type) *_ptr)
> >
> > > +
> > > +# define VIR_AUTOPTR(type) \
> > > +__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type
> > > VIR_AUTOPTR_TYPE_NAME(type)
> > > +
> >
> > Also, since we're going to use it like this:
> > VIR_AUTOPTR(virDomainDef) foo
> >
> > instead of this:
> > VIR_AUTOPTR(virDomainDefPtr) foo
> >
> > why do we need VIR_AUTOPTR_TYPE_NAME anyway, since you could just use
> > "type *" in the VIR_AUTOPTR macro definition and that should work for
> > external
> > types as well.
>
> I agree. We don't really need it. Here is how the code will look after
> the changes
> you suggested:
>
> # define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
>
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> if (*_ptr) \
> (func)(*_ptr); \
> *_ptr = NULL; \
> } \
>
> # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
>
> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type *
>
>
> Shall I proceed to send the first series of patches?
Yep, please do, since there weren't any major flaws code-wise, I think it
should be very straightforward.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
