1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement

Michal Privoznik posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/bec166d5330978ffc44d8d368068159deb941fc0.1491290681.git.mprivozn@redhat.com
src/libvirt_public.syms | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Michal Privoznik 7 years ago 
The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
and not 3.1.0 release as its current placement in the file
suggests.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/libvirt_public.syms | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 428cf2e..37a8a18 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
 
 LIBVIRT_3.1.0 {
     global:
-        virDomainSetBlockThreshold;
         virDomainSetVcpu;
 } LIBVIRT_3.0.0;
 
+LIBVIRT_3.2.0 {
+     global:
+        virDomainSetBlockThreshold;
+} LIBVIRT_3.1.0;
+
 # .... define new API here using predicted next version number ....
-- 
2.10.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Jiri Denemark 7 years ago 
On Tue, Apr 04, 2017 at 09:24:41 +0200, Michal Privoznik wrote:
> The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
> and not 3.1.0 release as its current placement in the file
> suggests.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/libvirt_public.syms | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 428cf2e..37a8a18 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
>  
>  LIBVIRT_3.1.0 {
>      global:
> -        virDomainSetBlockThreshold;
>          virDomainSetVcpu;
>  } LIBVIRT_3.0.0;
>  
> +LIBVIRT_3.2.0 {
> +     global:
> +        virDomainSetBlockThreshold;
> +} LIBVIRT_3.1.0;
> +
>  # .... define new API here using predicted next version number ....

Oh, this is unfortunate. But doesn't this change break ABI?

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Michal Privoznik 7 years ago 
On 04/04/2017 09:27 AM, Jiri Denemark wrote:
> On Tue, Apr 04, 2017 at 09:24:41 +0200, Michal Privoznik wrote:
>> The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
>> and not 3.1.0 release as its current placement in the file
>> suggests.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/libvirt_public.syms | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
>> index 428cf2e..37a8a18 100644
>> --- a/src/libvirt_public.syms
>> +++ b/src/libvirt_public.syms
>> @@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
>>
>>  LIBVIRT_3.1.0 {
>>      global:
>> -        virDomainSetBlockThreshold;
>>          virDomainSetVcpu;
>>  } LIBVIRT_3.0.0;
>>
>> +LIBVIRT_3.2.0 {
>> +     global:
>> +        virDomainSetBlockThreshold;
>> +} LIBVIRT_3.1.0;
>> +
>>  # .... define new API here using predicted next version number ....
>
> Oh, this is unfortunate. But doesn't this change break ABI?


Well, the change I'm referring to in the commit message broke ABI in the 
first place. This will break ABI only for apps linked with 3.2.0 release 
and un-break apps linked with every other release.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Daniel P. Berrange 7 years ago 
On Tue, Apr 04, 2017 at 09:42:47AM +0200, Michal Privoznik wrote:
> On 04/04/2017 09:27 AM, Jiri Denemark wrote:
> > On Tue, Apr 04, 2017 at 09:24:41 +0200, Michal Privoznik wrote:
> > > The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
> > > and not 3.1.0 release as its current placement in the file
> > > suggests.
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >  src/libvirt_public.syms | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> > > index 428cf2e..37a8a18 100644
> > > --- a/src/libvirt_public.syms
> > > +++ b/src/libvirt_public.syms
> > > @@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
> > > 
> > >  LIBVIRT_3.1.0 {
> > >      global:
> > > -        virDomainSetBlockThreshold;
> > >          virDomainSetVcpu;
> > >  } LIBVIRT_3.0.0;
> > > 
> > > +LIBVIRT_3.2.0 {
> > > +     global:
> > > +        virDomainSetBlockThreshold;
> > > +} LIBVIRT_3.1.0;
> > > +
> > >  # .... define new API here using predicted next version number ....
> > 
> > Oh, this is unfortunate. But doesn't this change break ABI?
> 
> 
> Well, the change I'm referring to in the commit message broke ABI in the
> first place. This will break ABI only for apps linked with 3.2.0 release and
> un-break apps linked with every other release.

No, the original change doesn't break ABI at all. It merely added a new
symbol to the existing ABI. This means that applications which used the
new API would build, link & run just fine - they'll simply not get an
automatic dependancy on libvirt 3.2.0 added to the RPM deps. By contrast,
changing the version now, will definitely break ABI, since any apps which
linked to libvirt 3.2.0 using this API will have unresolveable symbols at
link load time.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Michal Privoznik 7 years ago 
On 04/04/2017 11:56 AM, Daniel P. Berrange wrote:
> On Tue, Apr 04, 2017 at 09:42:47AM +0200, Michal Privoznik wrote:
>> On 04/04/2017 09:27 AM, Jiri Denemark wrote:
>>> On Tue, Apr 04, 2017 at 09:24:41 +0200, Michal Privoznik wrote:
>>>> The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
>>>> and not 3.1.0 release as its current placement in the file
>>>> suggests.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/libvirt_public.syms | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
>>>> index 428cf2e..37a8a18 100644
>>>> --- a/src/libvirt_public.syms
>>>> +++ b/src/libvirt_public.syms
>>>> @@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
>>>>
>>>>  LIBVIRT_3.1.0 {
>>>>      global:
>>>> -        virDomainSetBlockThreshold;
>>>>          virDomainSetVcpu;
>>>>  } LIBVIRT_3.0.0;
>>>>
>>>> +LIBVIRT_3.2.0 {
>>>> +     global:
>>>> +        virDomainSetBlockThreshold;
>>>> +} LIBVIRT_3.1.0;
>>>> +
>>>>  # .... define new API here using predicted next version number ....
>>>
>>> Oh, this is unfortunate. But doesn't this change break ABI?
>>
>>
>> Well, the change I'm referring to in the commit message broke ABI in the
>> first place. This will break ABI only for apps linked with 3.2.0 release and
>> un-break apps linked with every other release.
>
> No, the original change doesn't break ABI at all. It merely added a new
> symbol to the existing ABI. This means that applications which used the
> new API would build, link & run just fine - they'll simply not get an
> automatic dependancy on libvirt 3.2.0 added to the RPM deps. By contrast,
> changing the version now, will definitely break ABI, since any apps which
> linked to libvirt 3.2.0 using this API will have unresolveable symbols at
> link load time.

The apps linked with 3.2.0 and using the SetBlockThreshold API would get 
a false dependency on 3.1.0 so downgrading libvirt from 3.2.0 to 3.1.0 
would break for them: rpm would allow it but in the end the app fails to 
load. But this is much narrower use case. I agree. So lets keep the code 
as is.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Daniel P. Berrange 7 years ago 
On Tue, Apr 04, 2017 at 11:59:20AM +0200, Michal Privoznik wrote:
> On 04/04/2017 11:56 AM, Daniel P. Berrange wrote:
> > On Tue, Apr 04, 2017 at 09:42:47AM +0200, Michal Privoznik wrote:
> > > On 04/04/2017 09:27 AM, Jiri Denemark wrote:
> > > > On Tue, Apr 04, 2017 at 09:24:41 +0200, Michal Privoznik wrote:
> > > > > The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
> > > > > and not 3.1.0 release as its current placement in the file
> > > > > suggests.
> > > > > 
> > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > > > ---
> > > > >  src/libvirt_public.syms | 6 +++++-
> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> > > > > index 428cf2e..37a8a18 100644
> > > > > --- a/src/libvirt_public.syms
> > > > > +++ b/src/libvirt_public.syms
> > > > > @@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
> > > > > 
> > > > >  LIBVIRT_3.1.0 {
> > > > >      global:
> > > > > -        virDomainSetBlockThreshold;
> > > > >          virDomainSetVcpu;
> > > > >  } LIBVIRT_3.0.0;
> > > > > 
> > > > > +LIBVIRT_3.2.0 {
> > > > > +     global:
> > > > > +        virDomainSetBlockThreshold;
> > > > > +} LIBVIRT_3.1.0;
> > > > > +
> > > > >  # .... define new API here using predicted next version number ....
> > > > 
> > > > Oh, this is unfortunate. But doesn't this change break ABI?
> > > 
> > > 
> > > Well, the change I'm referring to in the commit message broke ABI in the
> > > first place. This will break ABI only for apps linked with 3.2.0 release and
> > > un-break apps linked with every other release.
> > 
> > No, the original change doesn't break ABI at all. It merely added a new
> > symbol to the existing ABI. This means that applications which used the
> > new API would build, link & run just fine - they'll simply not get an
> > automatic dependancy on libvirt 3.2.0 added to the RPM deps. By contrast,
> > changing the version now, will definitely break ABI, since any apps which
> > linked to libvirt 3.2.0 using this API will have unresolveable symbols at
> > link load time.
> 
> The apps linked with 3.2.0 and using the SetBlockThreshold API would get a
> false dependency on 3.1.0 so downgrading libvirt from 3.2.0 to 3.1.0 would
> break for them: rpm would allow it but in the end the app fails to load. But
> this is much narrower use case. I agree. So lets keep the code as is.

What we could do is list the symbol in *both* versions. That way next time
an app is rebuilt, the linker should pick the 3.2.0 tagged symbol, ensuring
it gets the stronger RPM dep, but we still heve the 3.1.0 tagged symbol for
compat

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt_public.syms: Fix virDomainSetBlockThreshold placement
Posted by Daniel P. Berrange 7 years ago 
On Tue, Apr 04, 2017 at 09:27:42AM +0200, Jiri Denemark wrote:
> On Tue, Apr 04, 2017 at 09:24:41 +0200, Michal Privoznik wrote:
> > The symbol was introduced in the 3.2.0 release (bb09798fbeb5f)
> > and not 3.1.0 release as its current placement in the file
> > suggests.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  src/libvirt_public.syms | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> > index 428cf2e..37a8a18 100644
> > --- a/src/libvirt_public.syms
> > +++ b/src/libvirt_public.syms
> > @@ -755,8 +755,12 @@ LIBVIRT_3.0.0 {
> >  
> >  LIBVIRT_3.1.0 {
> >      global:
> > -        virDomainSetBlockThreshold;
> >          virDomainSetVcpu;
> >  } LIBVIRT_3.0.0;
> >  
> > +LIBVIRT_3.2.0 {
> > +     global:
> > +        virDomainSetBlockThreshold;
> > +} LIBVIRT_3.1.0;
> > +
> >  # .... define new API here using predicted next version number ....
> 
> Oh, this is unfortunate. But doesn't this change break ABI?

Yes, it'll break ABI for anyone who has linked to libvirt 3.2.0 using
the new symbol.

Nasty as this mistake is, the cure is worse than the problem

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.