Re: [libvirt] Matching the type of mediated devices in the migration
On Fri, 3 Aug 2018 12:07:58 + "Wang, Zhi A" wrote: > Hi: > > Thanks for unfolding your idea. The picture is clearer to me now. I didn't > realize that you also want to support cross hardware migration. Well, I > thought for a while, the cross hardware migration might be not popular in > vGPU case but could be quite popular in other mdev cases. Exactly, we need to think beyond the implementation for a specific vendor or class of device. > Let me continue my summary: > > Mdev dev type has already included a parent driver name/a group name/physical > device version/configuration type. For example i915-GVTg_V5_4. The driver > name and the group name could already distinguish the vendor and the product > between different mdevs, e.g. between Intel and Nvidia, between vGPU or > vOther. Note that there are only two identifiers here, a vendor driver and a type. We included the vendor driver to avoid namespace collisions between vendors. The type itself should be considered opaque regardless of how a specific vendor makes use of it. > Each device provides a collection of the version of device state of data > stream in a preferred order in a mdev type, as newer version of device state > might contains more information which might help on performances. > > Let's say a new device N and an old device O, they both support mdev_type M. > > For example: > Device N is newer and supports the versions of device state: [ 6.3 6.2 .6.1 > ] in mdev type M > Device O is older and supports the versions of device state: [ 5.3 5.2 5.1 ] > in mdev type M > > - Version scheme of device state in backwards compatibility case: Migrate a > VM from a VM with device O to a VM with device N, the mdev type is M. > > Device N: [ 6.3 6.2 6.1 5.3 ] in M > Device O: [ 5.3 5.2 5.1 ] in M > Version used in migration: 5.3 > The new device directly supports mdev_type M with the preferred version on > Device O. Good, best situation. > > Device N: [ 6.3 6.2 6.1 5.2 ] in M > Device O: [ 5.3 5.2 5.1 ] in M > Version used in migration: 5.2 > The new device supports mdev_type M, but not the preferred version. After the > migration, the vendor driver might have to disable some features which is not > mentioned in 5.2 device state. But this totally depends on the vendor driver. > If user wish to achieve the best experience, he should update the vendor > driver in device N, which supports the preferred version on device O. > > Device N: [ 6.3 6.2 6.1 ] in M > Device O: [ 5.3 5.2 5.1 ] in M > Version used in migration: None > No version is matched. Migration would fail. User should update the vendor > driver on device N and device O. > > - Version scheme of device state in forwards compatibility case: Migrate a VM > from a VM with N to a VM with device O, the mdev type is M. > > Device N: [ 6.3 6.2 .6.1 ] in M > Device O: [ 5.3 5.2 5.1 ] in M, but the user updates the vendor driver on > device O. Now device O could support [ 5.3 5.2 5.1 6.1 ] (As an old device, > the Device O still prefers version 5.3) > Version used in migration: 6.1 > As the new device states is going to migrate to an old device, the vendor > driver on old device might have to specially dealing with the new version of > device state. It depends on the vendor driver. > > - QEMU has to figure out and choose the version of device states before > reading device state from the region. (Perhaps we can put the option of > selection in the control part of the region as well) > - Libvirt will check if there is any match of the version in the collection > in device O and device N before migration. > - Each mdev_type has its own collection of versions. (Device can support > different versions in different types) > - Better the collection is not a range, better they could be a collection of > the version strings. (The vendor driver might drop some versions during the > upgrade since they are not ideal) I believe that QEMU has always avoided trying to negotiate a migration version. We can only negotiate if the target is online and since a save/restore is essentially an offline migration, there's no opportunity for negotiation. Therefore I think we need to assume the source version is fixed. If we need to expose an older migration interface, I think we'd need to consider instantiating the mdev with that specification or configuring it via attributes before usage, just like QEMU does with specifying a machine type version. Providing an explicit list of compatible versions also seems like it could quickly get out of hand, imagine a driver with regular releases that maintains compatibility for years. The list could get unmanageable. To be honest, I'm pretty dubious whether vendors will actually implement cross version migration, or really consider migration compatibility at all, which is why I think we need to impose migration compatibility with this sort of interface. A vendor that doesn't want to support cross version migration can simply
Re: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
On Fri, 3 Aug 2018 at 18:49, Erik Skultety wrote:
>
> On Sat, Jul 28, 2018 at 11:31:31PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > majority of the calls to *Free functions can be dropped, which
> > in turn leads to getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar
> > ---
> > src/util/virnetdevmacvlan.c | 28
> > 1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> > index a2ed65c..d01e5ef 100644
> > --- a/src/util/virnetdevmacvlan.c
> > +++ b/src/util/virnetdevmacvlan.c
> > @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const
> > char *ifname,
> > virNetDevVPortProfilePtr
> > virtPortProfile,
> > virNetDevVPortProfileOp vmOp)
> > {
> > -virNetlinkCallbackDataPtr calld = NULL;
> > +VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> > +virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
> >
> > if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE))
> > {
> > if (VIR_ALLOC(calld) < 0)
> > -goto error;
> > +return -1;
> > if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> > -goto error;
> > +return -1;
> > if (VIR_ALLOC(calld->virtPortProfile) < 0)
> > -goto error;
> > +return -1;
> > memcpy(calld->virtPortProfile, virtPortProfile,
> > sizeof(*virtPortProfile));
> > virMacAddrSet(>macaddress, macaddress);
> > if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> > -goto error;
> > +return -1;
> > memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
> >
> > calld->vmOp = vmOp;
> > @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const
> > char *ifname,
> > if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> >
> > virNetDevMacVLanVPortProfileDestroyCallback,
> > calld, macaddress, NETLINK_ROUTE) < 0)
> > -goto error;
> > +return -1;
> > }
> >
> > +VIR_STEAL_PTR(temp, calld);
> > +
> > return 0;
> > -
> > - error:
> > -virNetlinkCallbackDataFree(calld);
> > -return -1;
> > }
>
> ^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
> here and should be left as is.
But by doing this, are getting rid of goto jumps and error section.
That was one of the goals, right?
>
> >
> >
> > @@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char
> > *ifname,
> > }
> >
> > if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
> > -virMacAddrPtr MAC = NULL;
> > -virMacAddrPtr adminMAC = NULL;
> > -virNetDevVlanPtr vlan = NULL;
> > +VIR_AUTOPTR(virMacAddr) MAC = NULL;
> > +VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
> > +VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
> >
> > if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
> > , , ) == 0) &&
> > @@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char
> > *ifname,
> >
> > ignore_value(virNetDevSetNetConfig(linkdev, -1,
> > adminMAC, vlan, MAC,
> > !!vlan));
> > -VIR_FREE(MAC);
> > -VIR_FREE(adminMAC);
> > -virNetDevVlanFree(vlan);
> > }
>
> To ^this hunk:
> Reviewed-by: Erik Skultety
>
>
> > }
> >
> > --
> > 1.8.3.1
> >
> > --
> > 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] tests: Unify data structure for vircaps2xmltest
On Thu, Aug 02, 2018 at 06:40:56PM +0200, Andrea Bolognani wrote: If all we achieve is reducing the depth by one for a single test case, the additional complexity (not to mention breaking the principle of least surprise) is not worth it: let's use simpler, more predictable code instead. This basically reverts fec6e4c48c9c (with a few adjustments). Yeah, it doesn't make sense. I recall there was yet another reason for that and it should've been used later on in a test that was never written (and me being the only one interested in making that test didn't help). So: Reviewed-by: Martin Kletzander signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] conf: rename Export Callback functions
On Mon, 2018-07-30 at 10:07 -0400, Anya Harter wrote: [...] > static int > -virInterfaceObjListPopulate(void *payload, > +virInterfaceObjListExportCallback(void *payload, > const void *name ATTRIBUTE_UNUSED, > void *opaque) Please make sure the signature remains aligned when you change the function's name. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] conf: rename structs used by Export function
On Mon, 2018-07-30 at 10:07 -0400, Anya Harter wrote:
> conf: rename structs used by Export function
>
> to be the name of the Export function followed by Data
>
> ex. for virInterfaceObjListExport, the struct is named
> virInterfaceObjListExportData
The title and body of the commit message should be able to stand
on their own as complete sentences, so you'll have to rework the
latter.
[...]
> -struct virInterfaceObjListData {
> +struct virInterfaceObjListExportData {
> virConnectPtr conn;
> virInterfacePtr *ifaces;
> virInterfaceObjListFilter filter;
> @@ -274,7 +274,7 @@ virInterfaceObjListPopulate(void *payload,
> const void *name ATTRIBUTE_UNUSED,
> void *opaque)
> {
> -struct virInterfaceObjListData *data = opaque;
> +struct virInterfaceObjListExportData *data = opaque;
This is kind of a weird pattern in the context of libvirt: most
structs are defined as
typedef struct _virSomething virSomething;
typedef virSomething *virSomethingPtr;
struct _virSomething {
...
};
Since you're tweaking both the definitions and the usage sites
anyway, might as well go the extra mile and adopt the most
common style as well.
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 19/32] util: netdevtap: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:34PM +0530, Sukrit Bhatnagar wrote: > By making use of GNU C's cleanup attribute handled by the > VIR_AUTOFREE macro for declaring scalar variables, majority > of the VIR_FREE calls can be dropped, which in turn leads to > getting rid of most of our cleanup sections. > > Signed-off-by: Sukrit Bhatnagar > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 18/32] util: netdevopenvswitch: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:33PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virnetdevopenvswitch.c | 106
> +++-
> 1 file changed, 40 insertions(+), 66 deletions(-)
>
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index a9c5e2a..eae5861 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -76,9 +76,7 @@ virNetDevOpenvswitchAddTimeout(virCommandPtr cmd)
> static int
> virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr
> virtVlan)
> {
> -int ret = -1;
> size_t i = 0;
> -virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> if (!virtVlan || !virtVlan->nTags)
> return 0;
> @@ -98,7 +96,12 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd,
> virNetDevVlanPtr virtVlan)
> }
>
> if (virtVlan->trunk) {
> -virBufferAddLit(, "trunk=");
> +VIR_AUTOPTR(virBuffer) buf = NULL;
> +
> +if (VIR_ALLOC(buf) < 0)
> +return -1;
> +
> +virBufferAddLit(buf, "trunk=");
>
> /*
> * Trunk ports have at least one VLAN. Do the first one
> @@ -106,24 +109,21 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd,
> virNetDevVlanPtr virtVlan)
> * start of the for loop if there are more than one VLANs
> * on this trunk port.
> */
> -virBufferAsprintf(, "%d", virtVlan->tag[i]);
> +virBufferAsprintf(buf, "%d", virtVlan->tag[i]);
>
> for (i = 1; i < virtVlan->nTags; i++) {
> -virBufferAddLit(, ",");
> -virBufferAsprintf(, "%d", virtVlan->tag[i]);
> +virBufferAddLit(buf, ",");
> +virBufferAsprintf(buf, "%d", virtVlan->tag[i]);
> }
>
> -if (virBufferCheckError() < 0)
> -goto cleanup;
> -virCommandAddArg(cmd, virBufferCurrentContent());
> +if (virBufferCheckError(buf) < 0)
> +return -1;
> +virCommandAddArg(cmd, virBufferCurrentContent(buf));
> } else if (virtVlan->nTags) {
> virCommandAddArgFormat(cmd, "tag=%d", virtVlan->tag[0]);
> }
>
> -ret = 0;
> - cleanup:
> -virBufferFreeAndReset();
> -return ret;
> +return 0;
The obvious problem with virBuffer :)...otherwise it's fine, + the ordering
issue of the declarations, that applies to all patches.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 17/32] util: netdevopenvswitch: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:32PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
...
> @@ -424,7 +419,6 @@ int
> virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
> {
> virCommandPtr cmd = NULL;
> -int ret = -1;
> int exitstatus;
>
> *master = NULL;
> @@ -438,7 +432,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char
> *ifname, char **master)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Unable to run command to get OVS master for "
> "interface %s"), ifname);
> -goto cleanup;
> +return -1;
> }
>
> /* non-0 exit code just means that the interface has no master in OVS */
> @@ -454,9 +448,7 @@ virNetDevOpenvswitchInterfaceGetMaster(const char
> *ifname, char **master)
>
> VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master :
> "(none)");
>
> -ret = 0;
> - cleanup:
> -return ret;
> +return 0;
Probably should be a separate patch. The rest is fine.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 14/32] util: netdevmacvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:29PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When a variable of type virNetlinkCallbackDataPtr is declared using
> VIR_AUTOPTR, the function virNetlinkCallbackDataFree will be run
> automatically on it when it goes out of scope.
>
> This commit also adds an intermediate typedef for virNetlinkCallbackData
> type for use with the cleanup macros.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virnetdevmacvlan.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index fb41bf9..91c6244 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -576,7 +576,7 @@ static const uint32_t
> modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = {
> };
>
> /* Struct to hold the state and configuration of a 802.1qbg port */
> -struct virNetlinkCallbackData {
> +struct _virNetlinkCallbackData {
> char *cr_ifname;
> virNetDevVPortProfilePtr virtPortProfile;
> virMacAddr macaddress;
> @@ -587,7 +587,8 @@ struct virNetlinkCallbackData {
> unsigned int linkState;
> };
>
> -typedef struct virNetlinkCallbackData *virNetlinkCallbackDataPtr;
> +typedef struct _virNetlinkCallbackData virNetlinkCallbackData;
> +typedef virNetlinkCallbackData *virNetlinkCallbackDataPtr;
>
> # define INSTANCE_STRLEN 36
>
> @@ -870,6 +871,8 @@ virNetlinkCallbackDataFree(virNetlinkCallbackDataPtr
> calld)
> VIR_FREE(calld);
> }
>
> +VIR_DEFINE_AUTOPTR_FUNC(virNetlinkCallbackData, virNetlinkCallbackDataFree)
I believe we can add this once we have a better use case, right now, we don't,
so this patch can be dropped.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:31PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virnetdevmacvlan.c | 28
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index a2ed65c..d01e5ef 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char
> *ifname,
> virNetDevVPortProfilePtr
> virtPortProfile,
> virNetDevVPortProfileOp vmOp)
> {
> -virNetlinkCallbackDataPtr calld = NULL;
> +VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> +virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
>
> if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
> if (VIR_ALLOC(calld) < 0)
> -goto error;
> +return -1;
> if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> -goto error;
> +return -1;
> if (VIR_ALLOC(calld->virtPortProfile) < 0)
> -goto error;
> +return -1;
> memcpy(calld->virtPortProfile, virtPortProfile,
> sizeof(*virtPortProfile));
> virMacAddrSet(>macaddress, macaddress);
> if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> -goto error;
> +return -1;
> memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
>
> calld->vmOp = vmOp;
> @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char
> *ifname,
> if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
>
> virNetDevMacVLanVPortProfileDestroyCallback,
> calld, macaddress, NETLINK_ROUTE) < 0)
> -goto error;
> +return -1;
> }
>
> +VIR_STEAL_PTR(temp, calld);
> +
> return 0;
> -
> - error:
> -virNetlinkCallbackDataFree(calld);
> -return -1;
> }
^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
here and should be left as is.
>
>
> @@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char
> *ifname,
> }
>
> if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
> -virMacAddrPtr MAC = NULL;
> -virMacAddrPtr adminMAC = NULL;
> -virNetDevVlanPtr vlan = NULL;
> +VIR_AUTOPTR(virMacAddr) MAC = NULL;
> +VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
> +VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
>
> if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
> , , ) == 0) &&
> @@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char
> *ifname,
>
> ignore_value(virNetDevSetNetConfig(linkdev, -1,
> adminMAC, vlan, MAC, !!vlan));
> -VIR_FREE(MAC);
> -VIR_FREE(adminMAC);
> -virNetDevVlanFree(vlan);
> }
To ^this hunk:
Reviewed-by: Erik Skultety
> }
>
> --
> 1.8.3.1
>
> --
> 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 v1 15/32] util: netdevmacvlan: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:30PM +0530, Sukrit Bhatnagar wrote: > By making use of GNU C's cleanup attribute handled by the > VIR_AUTOFREE macro for declaring scalar variables, majority > of the VIR_FREE calls can be dropped, which in turn leads to > getting rid of most of our cleanup sections. > > Signed-off-by: Sukrit Bhatnagar > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 13/32] util: netdevip: use VIR_AUTOPTR for aggregate types
On Fri, Aug 03, 2018 at 06:38:30PM +0530, Sukrit Bhatnagar wrote:
> On Fri, 3 Aug 2018 at 18:32, Erik Skultety wrote:
> >
> > On Sat, Jul 28, 2018 at 11:31:28PM +0530, Sukrit Bhatnagar wrote:
> > > By making use of GNU C's cleanup attribute handled by the
> > > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > > majority of the calls to *Free functions can be dropped, which
> > > in turn leads to getting rid of most of our cleanup sections.
> > >
> > > Signed-off-by: Sukrit Bhatnagar
> > > ---
> > > src/util/virnetdevip.c | 55
> > > +-
> > > 1 file changed, 23 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > > index 8f1081b..ca206e2 100644
> > > --- a/src/util/virnetdevip.c
> > > +++ b/src/util/virnetdevip.c
> > > @@ -634,19 +634,22 @@ virNetDevIPCheckIPv6Forwarding(void)
> > > }
> > >
> > > if (!valid) {
> > > -virBuffer buf = VIR_BUFFER_INITIALIZER;
> > > +VIR_AUTOPTR(virBuffer) buf = NULL;
> > > +
> > > +if (VIR_ALLOC(buf) < 0)
> > > +goto cleanup;
> >
> > Hmm, this will actually leak memory because @buf is never going to be freed,
> > worse, we'll assign NULL to it.
>
> But since @buf is declared as AUTOPTR, virBufferFreeAndReset will be called
> when it exits the scope, right?
> If I were using virBufferContentAndReset, then it might be the case.
How does virBufferFreeAndReset free @buf? It frees buf->content, but keeps
@buf.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 10/32] util: socketaddr: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:25PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virsocketaddr.c | 38 --
> 1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
> index eee725d..1b195cd 100644
> --- a/src/util/virsocketaddr.c
> +++ b/src/util/virsocketaddr.c
> @@ -1193,52 +1193,46 @@ virSocketAddrPTRDomain(const virSocketAddr *addr,
> unsigned int prefix,
> char **ptr)
> {
> -virBuffer buf = VIR_BUFFER_INITIALIZER;
> +VIR_AUTOPTR(virBuffer) buf = NULL;
I'll move this one line down.
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 09/32] util: socketaddr: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:24PM +0530, Sukrit Bhatnagar wrote: > By making use of GNU C's cleanup attribute handled by the > VIR_AUTOFREE macro for declaring scalar variables, majority > of the VIR_FREE calls can be dropped, which in turn leads to > getting rid of most of our cleanup sections. > > Signed-off-by: Sukrit Bhatnagar > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] rpm: simplify applying of patches
The distros we support for RPM builds all have %autosetup support so we
can ditch the convoluted code for running git manually and use the RPM
defaults.
Signed-off-by: Daniel P. Berrangé
---
libvirt.spec.in | 38 +-
1 file changed, 1 insertion(+), 37 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 19ae55cdaf..a2f3112a0b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -993,43 +993,7 @@ Libvirt plugin for NSS for translating domain names into
IP addresses.
%prep
-%setup -q
-
-# Patches have to be stored in a temporary file because RPM has
-# a limit on the length of the result of any macro expansion;
-# if the string is longer, it's silently cropped
-%{lua:
-tmp = os.tmpname();
-f = io.open(tmp, "w+");
-count = 0;
-for i, p in ipairs(patches) do
-f:write(p.."\n");
-count = count + 1;
-end;
-f:close();
-print("PATCHCOUNT="..count.."\n")
-print("PATCHLIST="..tmp.."\n")
-}
-
-git init -q
-git config user.name rpm-build
-git config user.email rpm-build
-git config gc.auto 0
-git add .
-git commit -q -a --author 'rpm-build ' \
- -m '%{name}-%{version} base'
-
-COUNT=$(grep '\.patch$' $PATCHLIST | wc -l)
-if [ $COUNT -ne $PATCHCOUNT ]; then
-echo "Found $COUNT patches in $PATCHLIST, expected $PATCHCOUNT"
-exit 1
-fi
-if [ $COUNT -gt 0 ]; then
-xargs git am <$PATCHLIST || exit 1
-fi
-echo "Applied $COUNT patches"
-rm -f $PATCHLIST
-rm -rf .git
+%autosetup -S git_am
%build
%if ! %{supported_platform}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 07/32] util: netdev: use VIR_AUTOPTR for aggregate types
On Sat, Jul 28, 2018 at 11:31:22PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virnetdev.c | 129
> ++-
> 1 file changed, 55 insertions(+), 74 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 7653f8b..c5871b4 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1855,16 +1855,15 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
> const char *stateDir,
> bool saveVlan)
> {
> -int ret = -1;
> const char *pfDevName = NULL;
> VIR_AUTOFREE(char *) pfDevOrig = NULL;
> VIR_AUTOFREE(char *) vfDevOrig = NULL;
> VIR_AUTOFREE(char *) filePath = NULL;
> VIR_AUTOFREE(char *) fileStr = NULL;
> +VIR_AUTOPTR(virJSONValue) configJSON = NULL;
> virMacAddr oldMAC;
> char MACStr[VIR_MAC_STRING_BUFLEN];
> int oldVlanTag = -1;
> -virJSONValuePtr configJSON = NULL;
>
> if (vf >= 0) {
> /* linkdev is the PF */
> @@ -1872,7 +1871,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
>
> /* linkdev should get the VF's netdev name (or NULL if none) */
> if (virNetDevPFGetVF(pfDevName, vf, ) < 0)
> -goto cleanup;
> +return -1;
>
> linkdev = vfDevOrig;
> saveVlan = true;
> @@ -1884,12 +1883,12 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
> */
>
> if (virNetDevGetPhysicalFunction(linkdev, ) < 0)
> -goto cleanup;
> +return -1;
>
> pfDevName = pfDevOrig;
>
> if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, ) < 0)
> -goto cleanup;
> +return -1;
> }
>
> if (pfDevName) {
> @@ -1907,7 +1906,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
> * explicitly enable the PF in the host system network config.
> */
> if (virNetDevGetOnline(pfDevName, ) < 0)
> -goto cleanup;
> +return -1;
>
> if (!pfIsOnline) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1916,12 +1915,12 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
> "change host network config to put the "
> "PF online."),
> vf, pfDevName);
> -goto cleanup;
> +return -1;
> }
> }
>
> if (!(configJSON = virJSONValueNewObject()))
> -goto cleanup;
> +return -1;
>
> /* if there is a PF, it's now in pfDevName, and linkdev is either
> * the VF's name, or NULL (if the VF isn't bound to a net driver
> @@ -1930,11 +1929,11 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
>
> if (pfDevName && saveVlan) {
> if (virAsprintf(, "%s/%s_vf%d", stateDir, pfDevName, vf) <
> 0)
> -goto cleanup;
> +return -1;
>
> /* get admin MAC and vlan tag */
> if (virNetDevGetVfConfig(pfDevName, vf, , ) < 0)
> -goto cleanup;
> +return -1;
>
> if (virJSONValueObjectAppendString(configJSON,
> VIR_NETDEV_KEYNAME_ADMIN_MAC,
> @@ -1942,39 +1941,36 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
> virJSONValueObjectAppendNumberInt(configJSON,
>VIR_NETDEV_KEYNAME_VLAN_TAG,
>oldVlanTag) < 0) {
> -goto cleanup;
> +return -1;
> }
>
> } else {
> if (virAsprintf(, "%s/%s", stateDir, linkdev) < 0)
> -goto cleanup;
> +return -1;
> }
>
> if (linkdev) {
> if (virNetDevGetMAC(linkdev, ) < 0)
> -goto cleanup;
> +return -1;
>
> /* for interfaces with no pfDevName (i.e. not a VF, this will
> * be the only value in the file.
> */
> if (virJSONValueObjectAppendString(configJSON,
> VIR_NETDEV_KEYNAME_MAC,
> virMacAddrFormat(,
> MACStr)) < 0)
> - goto cleanup;
> +return -1;
> }
>
> if (!(fileStr = virJSONValueToString(configJSON, true)))
> -goto cleanup;
> +return -1;
>
> if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
> virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
>"for device = %s, vf = %d"), linkdev,
> vf);
> -goto cleanup;
> +return -1;
> }
>
> -ret = 0;
> - cleanup:
> -virJSONValueFree(configJSON);
> -
Re: [libvirt] [PATCH v1 06/32] util: netdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Sat, Jul 28, 2018 at 11:31:21PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
...
>
> @@ -1282,10 +1260,11 @@ virNetDevGetVirtualFunctions(const char *pfname,
> {
> int ret = -1;
> size_t i;
> -char *pf_sysfs_device_link = NULL;
> -char *pci_sysfs_device_link = NULL;
> -char *pciConfigAddr = NULL;
> -char *pfPhysPortID = NULL;
> +VIR_AUTOFREE(char *) pf_sysfs_device_link = NULL;
> +VIR_AUTOFREE(char *) pci_sysfs_device_link = NULL;
> +VIR_AUTOFREE(char *) pciConfigAddr = NULL;
> +VIR_AUTOFREE(char *) pfPhysPortID = NULL;
> +VIR_AUTOFREE(char **) tempVfname = NULL;
First of all, this function should return the number of VFs on success or -1 on
error rather than needing the caller to pass an argument by reference to fill
the number of VFs, but that is a refactor for another day and I don't expect
you to spend time on that. Anyhow, tempVFname should be used instead of @vfname
at all spots and only at the end of the function block call VIR_STEAL_PTR.
>
> *virt_fns = NULL;
> *n_vfname = 0;
> @@ -1333,13 +1312,9 @@ virNetDevGetVirtualFunctions(const char *pfname,
>
> cleanup:
> if (ret < 0) {
> -VIR_FREE(*vfname);
> +VIR_STEAL_PTR(tempVfname, *vfname);
^This is not the intended usage of VIR_STEAL_PTR, the intended usage is to
steal the local pointer *into* the caller-provided one once we know we're going
to return success, not vice-versa, so ^this "if (ret < 0)" block should be
eventually dropped - you'd need another VIR_AUTOPTR for the virt_fns.
> VIR_FREE(*virt_fns);
> }
> -VIR_FREE(pfPhysPortID);
> -VIR_FREE(pf_sysfs_device_link);
> -VIR_FREE(pci_sysfs_device_link);
> -VIR_FREE(pciConfigAddr);
> return ret;
> }
>
...
> @@ -1522,13 +1473,14 @@ int
> virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
> int *vf)
> {
> -char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
> -int ret = -1;
> +VIR_AUTOFREE(char *) pf_sysfs_path = NULL;
> +VIR_AUTOFREE(char *) vf_sysfs_path = NULL;
> +VIR_AUTOFREE(char *) tempPfname = NULL;
>
> *pfname = NULL;
>
> if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
> -return ret;
> +return -1;
>
> if (virNetDevSysfsFile(_sysfs_path, *pfname, "device") < 0)
> goto cleanup;
> @@ -1536,16 +1488,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname,
> char **pfname,
> if (virNetDevSysfsFile(_sysfs_path, vfname, "device") < 0)
> goto cleanup;
>
> -ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
> +return virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
>
> cleanup:
> -if (ret < 0)
> -VIR_FREE(*pfname);
> +VIR_STEAL_PTR(tempPfname, *pfname);
Same comment as above.
>
> -VIR_FREE(vf_sysfs_path);
> -VIR_FREE(pf_sysfs_path);
> -
> -return ret;
> +return -1;
> }
...
>
> #else /* !__linux__ */
>
> cleanup:
> nlmsg_free(nl_msg);
As I noted in previous responses, we can turn ^this into VIR_AUTOPTR too.
> -VIR_FREE(resp);
> return family_id;
> }
>
> @@ -3265,13 +3184,13 @@ virNetDevSwitchdevFeature(const char *ifname,
>virBitmapPtr *out)
> {
> struct nl_msg *nl_msg = NULL;
> -struct nlmsghdr *resp = NULL;
> +VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> unsigned int recvbuflen;
> struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, };
> virPCIDevicePtr pci_device_ptr = NULL;
> struct genlmsghdr* gmsgh = NULL;
> const char *pci_name;
> -char *pfname = NULL;
> +VIR_AUTOFREE(char *) pfname = NULL;
> int is_vf = -1;
> int ret = -1;
> uint32_t family_id;
> @@ -,8 +3252,6 @@ virNetDevSwitchdevFeature(const char *ifname,
> cleanup:
> nlmsg_free(nl_msg);
> virPCIDeviceFree(pci_device_ptr);
> -VIR_FREE(resp);
> -VIR_FREE(pfname);
> return ret;
> }
> # else
> @@ -3375,7 +3292,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
> int fd,
> struct ifreq *ifr)
> {
> -struct ethtool_gfeatures *g_cmd;
> +VIR_AUTOFREE(struct ethtool_gfeatures *) g_cmd = NULL;
>
> if (VIR_ALLOC_VAR(g_cmd,
>struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
> @@ -3385,7 +3302,6 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
> g_cmd->size = GFEATURES_SIZE;
> if (virNetDevGFeatureAvailable(fd, ifr, g_cmd))
> ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL));
> -VIR_FREE(g_cmd);
> return 0;
> }
> # else
Otherwise, I don't see any other problems, since this
Re: [libvirt] [PATCH v1 04/32] util: macaddr: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Fri, Aug 03, 2018 at 09:30:22AM +0200, Erik Skultety wrote:
> On Sat, Jul 28, 2018 at 11:31:19PM +0530, Sukrit Bhatnagar wrote:
> > Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> > src/util/viralloc.h, define a new wrapper around an existing
> > cleanup function which will be called when a variable declared
> > with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> > viralloc.h include, since that has moved from the source module into
> > the header.
> >
> > When a variable of type virMacAddrPtr is declared using VIR_AUTOPTR,
> > the function virMacAddrFree will be run automatically on it when it
> > goes out of scope.
> >
> > Signed-off-by: Sukrit Bhatnagar
> > ---
> > src/util/virmacaddr.c | 6 ++
> > src/util/virmacaddr.h | 4
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> > index 7afe032..e739775 100644
> > --- a/src/util/virmacaddr.c
> > +++ b/src/util/virmacaddr.c
> > @@ -252,3 +252,9 @@ virMacAddrIsBroadcastRaw(const unsigned char
> > s[VIR_MAC_BUFLEN])
> > {
> > return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
> > }
> > +
> > +void
> > +virMacAddrFree(virMacAddrPtr addr)
> > +{
> > +VIR_FREE(addr);
> > +}
>
> I understand the reason behind this change, however, I don't feel like this
> will bring any benefits only because we said that VIR_AUTOFREE should be used
> with scalar types only, I'd prefer simply using VIR_AUTOFREE here, CC'ng Pavel
> to share his opinion.
>
> Erik
+Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 05/32] util: netdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
On Sat, Jul 28, 2018 at 11:31:20PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When variables of type virNetDevRxFilterPtr and virNetDevMcastEntryPtr
> are declared using VIR_AUTOPTR, the functions virNetDevRxFilterFree
> and virNetDevMcastEntryFree, respectively, will be run
> automatically on them when they go out of scope.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virnetdev.c | 9 -
> src/util/virnetdev.h | 4
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 0777eca..9eca786 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -29,7 +29,6 @@
> #include "virfile.h"
> #include "virerror.h"
> #include "vircommand.h"
> -#include "viralloc.h"
> #include "virpci.h"
> #include "virlog.h"
> #include "virstring.h"
> @@ -120,6 +119,14 @@ struct _virNetDevMcastEntry {
> virMacAddr macaddr;
> };
>
> +static void
> +virNetDevMcastEntryFree(virNetDevMcastEntryPtr entry)
> +{
> +VIR_FREE(entry);
> +}
> +
> +VIR_DEFINE_AUTOPTR_FUNC(virNetDevMcastEntry, virNetDevMcastEntryFree)
> +
Same as with the previous patch, I'm not convinced by this change.
> typedef struct _virNetDevMcastList virNetDevMcastList;
> typedef virNetDevMcastList *virNetDevMcastListPtr;
> struct _virNetDevMcastList {
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 71eaf45..8860ea1 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -30,6 +30,7 @@
> # include "virmacaddr.h"
> # include "virpci.h"
> # include "virnetdevvlan.h"
> +# include "viralloc.h"
>
> # ifdef HAVE_STRUCT_IFREQ
> typedef struct ifreq virIfreq;
> @@ -313,4 +314,7 @@ int virNetDevSysfsFile(char **pf_sysfs_device_link,
>
> int virNetDevRunEthernetScript(const char *ifname, const char *script)
> ATTRIBUTE_NOINLINE;
> +
> +VIR_DEFINE_AUTOPTR_FUNC(virNetDevRxFilter, virNetDevRxFilterFree)
> +
To ^this hunk though:
Reviewed-by: Erik Skultety
> #endif /* __VIR_NETDEV_H__ */
> --
> 1.8.3.1
>
> --
> 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 v1 03/32] util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar types
On Thu, Aug 02, 2018 at 11:03:16PM +0530, Sukrit Bhatnagar wrote:
> On Thu, 2 Aug 2018 at 19:33, Erik Skultety wrote:
> >
> > On Sat, Jul 28, 2018 at 11:31:18PM +0530, Sukrit Bhatnagar wrote:
> > > By making use of GNU C's cleanup attribute handled by the
> > > VIR_AUTOFREE macro for declaring scalar variables, majority
> > > of the VIR_FREE calls can be dropped, which in turn leads to
> > > getting rid of most of our cleanup sections.
> > >
> > > Signed-off-by: Sukrit Bhatnagar
> > > ---
> > > src/util/virnetdevbridge.c | 45
> > > +++--
> > > 1 file changed, 15 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > > index e46ac35..bf30d7c 100644
> > > --- a/src/util/virnetdevbridge.c
> > > +++ b/src/util/virnetdevbridge.c
> > > @@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname,
> > >int fd, /* control socket
> > > */
> > >struct ifreq *ifr) /* pre-filled
> > > bridge name */
> > > {
> > > -char *path = NULL;
> > > -int ret = -1;
> > > +VIR_AUTOFREE(char *) path = NULL;
> > >
> > > if (virAsprintf(, SYSFS_NET_DIR "%s/bridge/%s", brname,
> > > paramname) < 0)
> > > return -1;
> > > @@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname,
> > > if (virFileWriteStr(path, valuestr, 0) < 0) {
> > > virReportSystemError(errno,
> > > _("Unable to set bridge %s %s"),
> > > brname, paramname);
> > > -goto cleanup;
> > > +return -1;
> > > }
> > > } else {
> > > unsigned long paramid;
> > > @@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname,
> > > } else {
> > > virReportSystemError(EINVAL,
> > > _("Unable to set bridge %s %s"),
> > > brname, paramname);
> > > -goto cleanup;
> > > +return -1;
> > > }
> > > unsigned long args[] = { paramid, value, 0, 0 };
> > > ifr->ifr_data = (char*)
> > > if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> > > virReportSystemError(errno,
> > > _("Unable to set bridge %s %s"),
> > > brname, paramname);
> > > -goto cleanup;
> > > +return -1;
> > > }
> > > }
> > >
> > > -ret = 0;
> > > - cleanup:
> > > -VIR_FREE(path);
> > > -return ret;
> > > +return 0;
> > > }
> > >
> > >
> > > @@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname,
> > >const char *paramname, /* sysfs param
> > > name */
> > >unsigned long *value) /* current value */
> > > {
> > > -char *path = NULL;
> > > +VIR_AUTOFREE(char *) path = NULL;
> >
> > Referring to my response to previous patch, I'll move ^this at the end of
> > the
> > "declare" block (there are a few identical spots across the patch).
> >
> > ...
> >
> > > malformed_resp:
> > > @@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac,
> > > const char *ifname,
> > > unsigned int flags, bool isAdd)
> > > {
> > > int ret = -1;
> > > -struct nlmsghdr *resp = NULL;
> > > +VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> > > struct nlmsgerr *err;
> > > unsigned int recvbuflen;
> > > struct nl_msg *nl_msg;
> >
> > So, I believe ^this external type can easily be turned into an autoclean
> > variant.
> >
> > > @@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac,
> > > const char *ifname,
> > > ret = 0;
> > > cleanup:
> > > nlmsg_free(nl_msg);
> >
> > So that ^this would be done automatically.
>
> We would need to pass the nlmsg_free function in the cleanup attribute
> somehow.
> So, shall we not use the VIR_AUTO macros and declare it with cleanup attribute
> explicitly, or create a new type and a new Free wrapper for it to use
> with out macros?
Why do you need a new Free wrapper? Naturally, you'd need a "single-token" type
name, something like virNetlinkNlMsg which you'd put into src/util/virnetlink.h
along with the VIR_DEFINE_AUTOPTR_FUNC(virNetlinkNlMsg, nlmsg_free) and there
you go, you can use VIR_AUTOPTR with it.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC 0/2] Fix detection of slow guest shutdown
Hi, I was recently looking into a case which essentially looked like this: 1. virsh shutdown guest 2. after <1 second the qemu process was gone from /proc/ 3. but libvirt spun in virProcessKillPainfully because the process was still reachable via signals 4. virProcessKillPainfully eventually fails after 15 seconds and the guest stays in "in shutdown" state forever This is not one of the common cases I've found for virProcessKillPainfully to break: - bad I/O e.g. NFS gets qemu stuck - CPU overload stalls things to death - qemu not being reaped (by init) All of the above would have the process still available in /proc/ as Zombie or in uninterruptible sleep, but that is not true in my case. It turned out that the case was dependent on the amount of hostdev resources passed to the guest. Debugging showed that with 8 and more likely 16 GPUs passed it took ~18 seconds from SIGTERM to "no more be reachable with signal 0". I haven't conducted much more tests but stayed on the 16 GPU case, but I'm rather sure more devices might make it take even longer. Discussion with a few kernel folks revealed that the kill(2) man page on signal 0 has to be taken very literal "check for the existence of a process ID" - you can read this as "the PID exists, but the Process is no more". I'm unsure why the kernel would take that much time to clean up as I thought taking away /proc/ would be almost the last step in the cleanup of a task. patch 2: I happened to find that there seems to be no much better way than signal-0 to check, but finding that this isn't reliable if the kernel can still accept for quite some time even with the pid gone from all other interfaces that I could find - so I wanted to suggest a fallback in virProcessKillPainfully that considers the absence of /proc/ as a valid "the process is gone" as well on top of the ESRCH of signal-0. We could also use the open FDs we have e.g. to the qemu monitor to see if the remote end is dead, but that didn't seem more readable/reliable to me and would have to cross quite some code to know about the FDs. But maybe someone else here has the insight what exactly would take the time in the kernel that I see and that might bring us to totally different solutions (therefore RFC). patch 1: Finally after working through this code for a while I got the feeling that if we are in a bad/non-responsive case after 10 seconds upgrading to SIGKILL we should give it some more time to take effect. We reach this in stressful cases only anyway and only if force is set, so then waiting a bit more helps to resolve some of the other cases that I found on the mailing list about virProcessKillPainfully being stuck. If one has a personal interest in the 15 seconds we had before lets add a VIR_WARN on 15 seconds if that would be better, but overall wait a bit more. P.S. Afer a short discussion with Daniel on IRC I'm also adding Alex explicitly for passthrough experience. P.P.S.: For now this really is only meant as an RFC to kick off the discussion. I got taken away the system that I could trigger this case easily on before I could complete a final verification. But the case is interesting enough to start the discussion now. Christian Ehrhardt (2): process: wait longer 5->30s on hard shutdown process: accept the lack of /proc/ as valid process removal src/util/virprocess.c | 29 +++-- 1 file changed, 27 insertions(+), 2 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC 1/2] process: wait longer 5->30s on hard shutdown
In cases where virProcessKillPainfully already reailizes that
SIGTERM wasn't enough we are partially on a bad path already.
Maybe the system is overloaded or having serious trouble to free and
reap resources in time.
In those case give the SIGKILL that was sent after 10 seconds some more
time to take effect if force was set (only then we are falling back to
SIGKILL anyway).
Signed-off-by: Christian Ehrhardt
---
src/util/virprocess.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index f92b0dce37..10952b0980 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -350,6 +350,7 @@ virProcessKillPainfully(pid_t pid, bool force)
{
size_t i;
int ret = -1;
+int maxwait = (force ? 200 : 75 );
const char *signame = "TERM";
VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
@@ -357,12 +358,12 @@ virProcessKillPainfully(pid_t pid, bool force)
/* This loop sends SIGTERM, then waits a few iterations (10 seconds)
* to see if it dies. If the process still hasn't exited, and
* @force is requested, a SIGKILL will be sent, and this will
- * wait up to 5 seconds more for the process to exit before
+ * wait up to 30 seconds more for the process to exit before
* returning.
*
* Note that setting @force could result in dataloss for the process.
*/
-for (i = 0; i < 75; i++) {
+for (i = 0; i < maxwait; i++) {
int signum;
if (i == 0) {
signum = SIGTERM; /* kindly suggest it should exit */
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
