Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-09-02 Thread Kevin Wolf
Am 23.08.2019 um 11:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
> > To get rid of implicit filters related workarounds in future let's
> > deprecate them now.
> 
> Interesting, could we deprecate implicit filter without deprecation of
> unnecessity of parameter? As actually, it's good when this parameter
> is not necessary, in most cases user is not interested in node-name.

A modern client is supposed to be interested in node-names. We basically
expect it to manage the whole block graph at a node level, so it should
certainly make sure to know the node-name of every node. It will need it
to take snapshots, insert filter drivers or anything like this on top of
the implicit node.

So once we make the option mandatory (which means returning an error for
legacy clients that don't do node-level management), I don't see a good
reason to ever make it optional again.

Kevin



Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-29 Thread John Snow



On 8/29/19 11:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Aren't you going to deprecate and drop it at some moment?

Libvirt's going to keep supporting older versions of QEMU in perpetuity.
Apparently, there are still some barriers (SD cards?) to using only
blockdev API for new versions, too:

>> "Note that even with new qemu, if an SD card is used blockdev will be
>> disabled."

It sounds like we need to facilitate libvirt's transfer to an
all-blockdev API for modern QEMU instances. Once we do that, we can
likely add an introspectable feature flag to commands that create
formerly-implicit nodes to tip off libvirt as to what behavior it can
expect.

(In practice, if libvirt sees the new flag, it knows it needs to rely
exclusively on blockdev API and that (likely) it should always provide a
node-name for the filter.)

I think it is reasonably clear that deprecating and re-implementing is
not something that will be compatible with libvirt's feature detection,
so we shouldn't do it.

I'm still not sold that this is worth the effort, but you and Max would
know best right now. I'll leave it to you two to sort out.

--js




Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-29 Thread Christophe de Dinechin


John Snow writes:
[...]
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow commands to return with "Warning" strings,
> or "Deprecated" stings to allow in-band deprecation notices for cases
> like these?
>
> example:
>
> { "return": {},
>   "deprecated": True,
>   "warning": "Omitting filter-node-name parameter is deprecated, it will
> be required in the future"
> }
>
> There's no "error" key, so this should be recognized as success by
> compatible clients, but they'll definitely see the extra information.
>
> Part of my motivation is to facilitate a more aggressive deprecation of
> legacy features by ensuring that we are able to rigorously notify users
> through any means that they need to adjust their scripts.

I like this approach even if there is no consumer today. It does not
hurt, and it is indeed a motivation to develop consumers that care.

I personally find this much easier to swallow than any kind of crash on
deprecation, which already at the BoF seemed like a really big hammer to
kill a fly.

CC'ing Andrea as well, because we discussed recently about how to deal
with error checking in general, and if a new error checking framework is
being put in place, adding deprecation to the thinking could be a good
idea.

--
Cheers,
Christophe de Dinechin (IRC c3d)



Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-29 Thread Vladimir Sementsov-Ogievskiy
29.08.2019 17:44, Peter Krempa wrote:
> On Wed, Aug 28, 2019 at 13:48:10 -0400, John Snow wrote:
>> (Peter: search for "pkrempa" down below.)
>>
>> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> []
> 
> 
>> So that's a bit of a change, but only visually. The "reality" is still
>> the same, we just report it more "accurately." libvirt MIGHT need a
>> heads up here. I'm looping pkrempa back in for comment.
>>
>> 
>> Would libvirt be negatively impacted by the revelation of formerly
>> internal ("implicit") nodes created by mirror and commit via query block
>> commands? At the moment, QEMU hides them from you if you do not name them.
> 
> Currently we would not be able to handle that properly at least
> definitely in the pre-blockdev case. In blockdev case I must make sure
> that it will work.
> 
> The thing is that I didn't really want to touch the pre-blockdev case
> code any more,

Aren't you going to deprecate and drop it at some moment?

  but if you decide that we should do it I'm willing to
> investigate this case also for the old commands.
> 
>> 
>>
>>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>>> I think it's not a problem, just drop special case for implicit fitlers
>>>
>>
>> I'm much less certain about what the impact of this would be and would
>> need to audit it (and don't have the time to, personally.)
>>
>> Do you have a POC or RFC patch that demonstrates dropping these special
>> cases? It might be nice to see as proof that it's safe to deprecate.
>>
>>> So, seems the only real change is query-block and query-blockstats output 
>>> when mirror or commit is started
>>> without specifying filter-node-name (filter would be on top)
>>>
>>> So, how should we deprecate this, or can we just change it?
>>>
>>
>> I'm not sure if it's worth it yet, what does dropping the implicit field
>> buy us? Conceptually I understand that it's simpler without the notion
>> of implicit fields, but I imagine there's some cleanup in particular
>> that motivated this.
>>
>> I'd say to just change the behavior, we should:
>>
>> - Give a standard three-release warning that the behavior will change in
>> an incompatible way
>> - Demonstrate with an RFC patch that special cases around ->implicit in
>> block.c can be removed and do not make the code more complex,
>> - Get blessings from Peter Krempa.
>>
>> As always: Libvirt is not the end-all be-all of QEMU management, but if
>> libvirt is capable of working around design changes then I believe any
>> project out there today also could, so it's a good litmus test.
> 
> For libvirt we really care more whether a node is format/protocol
> related or not rather than whether it's implicit or not.
> 
> In this case we could filter it by the known protocol and format driver
> types and filter out the rest in cases when we e.g. detect the node
> names for the pre-blockdev era cases.
> 
> (Note that even with new qemu, if an SD card is used blockdev will be
> disabled).
> 


-- 
Best regards,
Vladimir



Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-29 Thread Vladimir Sementsov-Ogievskiy
29.08.2019 18:00, Vladimir Sementsov-Ogievskiy wrote:
> 28.08.2019 20:48, John Snow wrote:
>> (Peter: search for "pkrempa" down below.)
>>
>> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.08.2019 23:12, John Snow wrote:


 On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
>> To get rid of implicit filters related workarounds in future let's
>> deprecate them now.
>
> Interesting, could we deprecate implicit filter without deprecation of 
> unnecessity of
> parameter? As actually, it's good when this parameter is not necessary, 
> in most cases
> user is not interested in node-name.
>

 https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
 that this a real word in the language I speak. :)

 I assume you're referring to making the optional argument mandatory.
>>>
>>> exactly, it's my a bit "google-translate-driven" English)
>>>
>>
>> It teaches me some fun words!
>>

> Obviously we can do the following:
>
> 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
> filters
> 2. After some releases in 4.x we can drop deprecated functionality, so we 
> drop it together with
> implicit filters. And, in same release 4.x we return it back (as it's 
> compatible change :)
> but without implicit filters (so, if filter-node-name not specified, we 
> just create
> explicit filter with autogenerated node-name)
>
> So, effectively we just drop "deprecation mark" together with implicit 
> filters, which is nice
> but actually confusing.
>
> Instead, we may do
> 1. In 4.2 deprecate
> 2. In 4.x drop optionality together with implicit filters
> 3. In 4.y (y > x of course) return optionality back
>

 Ah, I see what you're digging at here now...

> It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
> difference..
>
> Or we just write in spec, that implicit filters are deprecated? But we 
> have nothing about implicit
> filters in spec. More over, we directly write that we have filter, and if 
> parameter is omitted
> it's node-name is autogenerated. So actually, the fact the filter is 
> hidden when filter-node-name is
> unspecified is _undocumented_.
>
> So, finally, it looks like nothing to deprecated in specification, we can 
> just drop implicit filters :)
>
> What do you think?
>

 What exactly _IS_ an implicit filter? How does it differ today from an
 explicit filter? I assumed the only difference was if it was named or
 not; but I think I must be mistaken now if you're proposing leaving the
 interface alone entirely.

 Are they instantiated differently?

>>>
>>> As I understand, the only difference is their BlockDriverState.impicit 
>>> field, and several places in code
>>> where we skip implicit filter when trying to find something in a chain 
>>> starting from a device.
>>>
>>
>> Oh, oh, yes. I see.
>>
>>> Hmm, OK, let's see:
>>>
>>> 1. the only implicit filters are commit_top and mirror_top if user don't 
>>> specify filter-node-name.
>>>
>>> Where it make sense, i.e., where implicit field used?
>>>
>>
>> `git grep -E '(->|\.)implicit'` is what I used to find usages.
>>
>>> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when 
>>> called from bdrv_query_info), they'll
>>> report filter as top node if we don't mark it implicit.
>>>
>>
>> So that's a bit of a change, but only visually. The "reality" is still
>> the same, we just report it more "accurately." libvirt MIGHT need a
>> heads up here. I'm looping pkrempa back in for comment.
>>
>> 
>> Would libvirt be negatively impacted by the revelation of formerly
>> internal ("implicit") nodes created by mirror and commit via query block
>> commands? At the moment, QEMU hides them from you if you do not name them.
>> 
>>
>>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>>>     I think it's not a problem, just drop special case for implicit fitlers
>>>
>>
>> I'm much less certain about what the impact of this would be and would
>> need to audit it (and don't have the time to, personally.)
>>
>> Do you have a POC or RFC patch that demonstrates dropping these special
>> cases? It might be nice to see as proof that it's safe to deprecate.

I faced a problem with some iotest (it's not a surprise of course), but I 
think, I'll
come back with and RFC of course.

>>
>>> So, seems the only real change is query-block and query-blockstats output 
>>> when mirror or commit is started
>>> without specifying filter-node-name (filter would be on top)
>>>
>>> So, how should we deprecate this, or can we just change it?
>>>
>>
>> I'm not sure if it's worth it yet, what does dropping the implicit field
>> buy us? Conceptually I understand 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-29 Thread Vladimir Sementsov-Ogievskiy
28.08.2019 20:48, John Snow wrote:
> (Peter: search for "pkrempa" down below.)
> 
> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 27.08.2019 23:12, John Snow wrote:
>>>
>>>
>>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.

 Interesting, could we deprecate implicit filter without deprecation of 
 unnecessity of
 parameter? As actually, it's good when this parameter is not necessary, in 
 most cases
 user is not interested in node-name.

>>>
>>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>>> that this a real word in the language I speak. :)
>>>
>>> I assume you're referring to making the optional argument mandatory.
>>
>> exactly, it's my a bit "google-translate-driven" English)
>>
> 
> It teaches me some fun words!
> 
>>>
 Obviously we can do the following:

 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
 filters
 2. After some releases in 4.x we can drop deprecated functionality, so we 
 drop it together with
 implicit filters. And, in same release 4.x we return it back (as it's 
 compatible change :)
 but without implicit filters (so, if filter-node-name not specified, we 
 just create
 explicit filter with autogenerated node-name)

 So, effectively we just drop "deprecation mark" together with implicit 
 filters, which is nice
 but actually confusing.

 Instead, we may do
 1. In 4.2 deprecate
 2. In 4.x drop optionality together with implicit filters
 3. In 4.y (y > x of course) return optionality back

>>>
>>> Ah, I see what you're digging at here now...
>>>
 It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
 difference..

 Or we just write in spec, that implicit filters are deprecated? But we 
 have nothing about implicit
 filters in spec. More over, we directly write that we have filter, and if 
 parameter is omitted
 it's node-name is autogenerated. So actually, the fact the filter is 
 hidden when filter-node-name is
 unspecified is _undocumented_.

 So, finally, it looks like nothing to deprecated in specification, we can 
 just drop implicit filters :)

 What do you think?

>>>
>>> What exactly _IS_ an implicit filter? How does it differ today from an
>>> explicit filter? I assumed the only difference was if it was named or
>>> not; but I think I must be mistaken now if you're proposing leaving the
>>> interface alone entirely.
>>>
>>> Are they instantiated differently?
>>>
>>
>> As I understand, the only difference is their BlockDriverState.impicit 
>> field, and several places in code
>> where we skip implicit filter when trying to find something in a chain 
>> starting from a device.
>>
> 
> Oh, oh, yes. I see.
> 
>> Hmm, OK, let's see:
>>
>> 1. the only implicit filters are commit_top and mirror_top if user don't 
>> specify filter-node-name.
>>
>> Where it make sense, i.e., where implicit field used?
>>
> 
> `git grep -E '(->|\.)implicit'` is what I used to find usages.
> 
>> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when 
>> called from bdrv_query_info), they'll
>> report filter as top node if we don't mark it implicit.
>>
> 
> So that's a bit of a change, but only visually. The "reality" is still
> the same, we just report it more "accurately." libvirt MIGHT need a
> heads up here. I'm looping pkrempa back in for comment.
> 
> 
> Would libvirt be negatively impacted by the revelation of formerly
> internal ("implicit") nodes created by mirror and commit via query block
> commands? At the moment, QEMU hides them from you if you do not name them.
> 
> 
>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>> I think it's not a problem, just drop special case for implicit fitlers
>>
> 
> I'm much less certain about what the impact of this would be and would
> need to audit it (and don't have the time to, personally.)
> 
> Do you have a POC or RFC patch that demonstrates dropping these special
> cases? It might be nice to see as proof that it's safe to deprecate.
> 
>> So, seems the only real change is query-block and query-blockstats output 
>> when mirror or commit is started
>> without specifying filter-node-name (filter would be on top)
>>
>> So, how should we deprecate this, or can we just change it?
>>
> 
> I'm not sure if it's worth it yet, what does dropping the implicit field
> buy us? Conceptually I understand that it's simpler without the notion
> of implicit fields, but I imagine there's some cleanup in particular
> that motivated this.

Reviewing Max's "block: Deal with filters" series motivated me.

> 
> I'd say to just change the behavior, we should:
> 
> - Give a standard three-release warning 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-29 Thread Peter Krempa
On Wed, Aug 28, 2019 at 13:48:10 -0400, John Snow wrote:
> (Peter: search for "pkrempa" down below.)
> 
> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

[]


> So that's a bit of a change, but only visually. The "reality" is still
> the same, we just report it more "accurately." libvirt MIGHT need a
> heads up here. I'm looping pkrempa back in for comment.
> 
> 
> Would libvirt be negatively impacted by the revelation of formerly
> internal ("implicit") nodes created by mirror and commit via query block
> commands? At the moment, QEMU hides them from you if you do not name them.

Currently we would not be able to handle that properly at least
definitely in the pre-blockdev case. In blockdev case I must make sure
that it will work.

The thing is that I didn't really want to touch the pre-blockdev case
code any more, but if you decide that we should do it I'm willing to
investigate this case also for the old commands.

> 
> 
> > 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
> >I think it's not a problem, just drop special case for implicit fitlers
> >
> 
> I'm much less certain about what the impact of this would be and would
> need to audit it (and don't have the time to, personally.)
> 
> Do you have a POC or RFC patch that demonstrates dropping these special
> cases? It might be nice to see as proof that it's safe to deprecate.
> 
> > So, seems the only real change is query-block and query-blockstats output 
> > when mirror or commit is started
> > without specifying filter-node-name (filter would be on top)
> > 
> > So, how should we deprecate this, or can we just change it?
> > 
> 
> I'm not sure if it's worth it yet, what does dropping the implicit field
> buy us? Conceptually I understand that it's simpler without the notion
> of implicit fields, but I imagine there's some cleanup in particular
> that motivated this.
> 
> I'd say to just change the behavior, we should:
> 
> - Give a standard three-release warning that the behavior will change in
> an incompatible way
> - Demonstrate with an RFC patch that special cases around ->implicit in
> block.c can be removed and do not make the code more complex,
> - Get blessings from Peter Krempa.
> 
> As always: Libvirt is not the end-all be-all of QEMU management, but if
> libvirt is capable of working around design changes then I believe any
> project out there today also could, so it's a good litmus test.

For libvirt we really care more whether a node is format/protocol
related or not rather than whether it's implicit or not.

In this case we could filter it by the known protocol and format driver
types and filter out the rest in cases when we e.g. detect the node
names for the pre-blockdev era cases.

(Note that even with new qemu, if an SD card is used blockdev will be
disabled).



signature.asc
Description: PGP signature


Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-28 Thread John Snow
(Peter: search for "pkrempa" down below.)

On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.08.2019 23:12, John Snow wrote:
>>
>>
>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
 To get rid of implicit filters related workarounds in future let's
 deprecate them now.
>>>
>>> Interesting, could we deprecate implicit filter without deprecation of 
>>> unnecessity of
>>> parameter? As actually, it's good when this parameter is not necessary, in 
>>> most cases
>>> user is not interested in node-name.
>>>
>>
>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>> that this a real word in the language I speak. :)
>>
>> I assume you're referring to making the optional argument mandatory.
> 
> exactly, it's my a bit "google-translate-driven" English)
> 

It teaches me some fun words!

>>
>>> Obviously we can do the following:
>>>
>>> 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
>>> filters
>>> 2. After some releases in 4.x we can drop deprecated functionality, so we 
>>> drop it together with
>>> implicit filters. And, in same release 4.x we return it back (as it's 
>>> compatible change :)
>>> but without implicit filters (so, if filter-node-name not specified, we 
>>> just create
>>> explicit filter with autogenerated node-name)
>>>
>>> So, effectively we just drop "deprecation mark" together with implicit 
>>> filters, which is nice
>>> but actually confusing.
>>>
>>> Instead, we may do
>>> 1. In 4.2 deprecate
>>> 2. In 4.x drop optionality together with implicit filters
>>> 3. In 4.y (y > x of course) return optionality back
>>>
>>
>> Ah, I see what you're digging at here now...
>>
>>> It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
>>> difference..
>>>
>>> Or we just write in spec, that implicit filters are deprecated? But we have 
>>> nothing about implicit
>>> filters in spec. More over, we directly write that we have filter, and if 
>>> parameter is omitted
>>> it's node-name is autogenerated. So actually, the fact the filter is hidden 
>>> when filter-node-name is
>>> unspecified is _undocumented_.
>>>
>>> So, finally, it looks like nothing to deprecated in specification, we can 
>>> just drop implicit filters :)
>>>
>>> What do you think?
>>>
>>
>> What exactly _IS_ an implicit filter? How does it differ today from an
>> explicit filter? I assumed the only difference was if it was named or
>> not; but I think I must be mistaken now if you're proposing leaving the
>> interface alone entirely.
>>
>> Are they instantiated differently?
>>
> 
> As I understand, the only difference is their BlockDriverState.impicit field, 
> and several places in code
> where we skip implicit filter when trying to find something in a chain 
> starting from a device.
> 

Oh, oh, yes. I see.

> Hmm, OK, let's see:
> 
> 1. the only implicit filters are commit_top and mirror_top if user don't 
> specify filter-node-name.
> 
> Where it make sense, i.e., where implicit field used?
> 

`git grep -E '(->|\.)implicit'` is what I used to find usages.

> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when 
> called from bdrv_query_info), they'll
> report filter as top node if we don't mark it implicit.
> 

So that's a bit of a change, but only visually. The "reality" is still
the same, we just report it more "accurately." libvirt MIGHT need a
heads up here. I'm looping pkrempa back in for comment.


Would libvirt be negatively impacted by the revelation of formerly
internal ("implicit") nodes created by mirror and commit via query block
commands? At the moment, QEMU hides them from you if you do not name them.


> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>I think it's not a problem, just drop special case for implicit fitlers
>

I'm much less certain about what the impact of this would be and would
need to audit it (and don't have the time to, personally.)

Do you have a POC or RFC patch that demonstrates dropping these special
cases? It might be nice to see as proof that it's safe to deprecate.

> So, seems the only real change is query-block and query-blockstats output 
> when mirror or commit is started
> without specifying filter-node-name (filter would be on top)
> 
> So, how should we deprecate this, or can we just change it?
> 

I'm not sure if it's worth it yet, what does dropping the implicit field
buy us? Conceptually I understand that it's simpler without the notion
of implicit fields, but I imagine there's some cleanup in particular
that motivated this.

I'd say to just change the behavior, we should:

- Give a standard three-release warning that the behavior will change in
an incompatible way
- Demonstrate with an RFC patch that special cases around ->implicit in
block.c can be removed and do not make the code more complex,
- Get blessings from Peter Krempa.

As always: Libvirt is not the end-all be-all 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-28 Thread Vladimir Sementsov-Ogievskiy
27.08.2019 23:12, John Snow wrote:
> 
> 
> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>> To get rid of implicit filters related workarounds in future let's
>>> deprecate them now.
>>
>> Interesting, could we deprecate implicit filter without deprecation of 
>> unnecessity of
>> parameter? As actually, it's good when this parameter is not necessary, in 
>> most cases
>> user is not interested in node-name.
>>
> 
> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
> that this a real word in the language I speak. :)
> 
> I assume you're referring to making the optional argument mandatory.

exactly, it's my a bit "google-translate-driven" English)

> 
>> Obviously we can do the following:
>>
>> 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
>> filters
>> 2. After some releases in 4.x we can drop deprecated functionality, so we 
>> drop it together with
>> implicit filters. And, in same release 4.x we return it back (as it's 
>> compatible change :)
>> but without implicit filters (so, if filter-node-name not specified, we just 
>> create
>> explicit filter with autogenerated node-name)
>>
>> So, effectively we just drop "deprecation mark" together with implicit 
>> filters, which is nice
>> but actually confusing.
>>
>> Instead, we may do
>> 1. In 4.2 deprecate
>> 2. In 4.x drop optionality together with implicit filters
>> 3. In 4.y (y > x of course) return optionality back
>>
> 
> Ah, I see what you're digging at here now...
> 
>> It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
>> difference..
>>
>> Or we just write in spec, that implicit filters are deprecated? But we have 
>> nothing about implicit
>> filters in spec. More over, we directly write that we have filter, and if 
>> parameter is omitted
>> it's node-name is autogenerated. So actually, the fact the filter is hidden 
>> when filter-node-name is
>> unspecified is _undocumented_.
>>
>> So, finally, it looks like nothing to deprecated in specification, we can 
>> just drop implicit filters :)
>>
>> What do you think?
>>
> 
> What exactly _IS_ an implicit filter? How does it differ today from an
> explicit filter? I assumed the only difference was if it was named or
> not; but I think I must be mistaken now if you're proposing leaving the
> interface alone entirely.
> 
> Are they instantiated differently?
> 

As I understand, the only difference is their BlockDriverState.impicit field, 
and several places in code
where we skip implicit filter when trying to find something in a chain starting 
from a device.

Hmm, OK, let's see:

1. the only implicit filters are commit_top and mirror_top if user don't 
specify filter-node-name.

Where it make sense, i.e., where implicit field used?

2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when 
called from bdrv_query_info), they'll
report filter as top node if we don't mark it implicit.

3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
   I think it's not a problem, just drop special case for implicit fitlers

So, seems the only real change is query-block and query-blockstats output when 
mirror or commit is started
without specifying filter-node-name (filter would be on top)

So, how should we deprecate this, or can we just change it?

-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-27 Thread John Snow



On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
>> To get rid of implicit filters related workarounds in future let's
>> deprecate them now.
> 
> Interesting, could we deprecate implicit filter without deprecation of 
> unnecessity of
> parameter? As actually, it's good when this parameter is not necessary, in 
> most cases
> user is not interested in node-name.
> 

https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
that this a real word in the language I speak. :)

I assume you're referring to making the optional argument mandatory.

> Obviously we can do the following:
> 
> 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
> filters
> 2. After some releases in 4.x we can drop deprecated functionality, so we 
> drop it together with
> implicit filters. And, in same release 4.x we return it back (as it's 
> compatible change :)
> but without implicit filters (so, if filter-node-name not specified, we just 
> create
> explicit filter with autogenerated node-name)
> 
> So, effectively we just drop "deprecation mark" together with implicit 
> filters, which is nice
> but actually confusing.
> 
> Instead, we may do
> 1. In 4.2 deprecate
> 2. In 4.x drop optionality together with implicit filters
> 3. In 4.y (y > x of course) return optionality back
> 

Ah, I see what you're digging at here now...

> It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
> difference..
> 
> Or we just write in spec, that implicit filters are deprecated? But we have 
> nothing about implicit
> filters in spec. More over, we directly write that we have filter, and if 
> parameter is omitted
> it's node-name is autogenerated. So actually, the fact the filter is hidden 
> when filter-node-name is
> unspecified is _undocumented_.
> 
> So, finally, it looks like nothing to deprecated in specification, we can 
> just drop implicit filters :)
> 
> What do you think?
> 

What exactly _IS_ an implicit filter? How does it differ today from an
explicit filter? I assumed the only difference was if it was named or
not; but I think I must be mistaken now if you're proposing leaving the
interface alone entirely.

Are they instantiated differently?

--js



Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-23 Thread Vladimir Sementsov-Ogievskiy
14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.

Interesting, could we deprecate implicit filter without deprecation of 
unnecessity of
parameter? As actually, it's good when this parameter is not necessary, in most 
cases
user is not interested in node-name.

Obviously we can do the following:

1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
filters
2. After some releases in 4.x we can drop deprecated functionality, so we drop 
it together with
implicit filters. And, in same release 4.x we return it back (as it's 
compatible change :)
but without implicit filters (so, if filter-node-name not specified, we just 
create
explicit filter with autogenerated node-name)

So, effectively we just drop "deprecation mark" together with implicit filters, 
which is nice
but actually confusing.

Instead, we may do
1. In 4.2 deprecate
2. In 4.x drop optionality together with implicit filters
3. In 4.y (y > x of course) return optionality back

It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
difference..

Or we just write in spec, that implicit filters are deprecated? But we have 
nothing about implicit
filters in spec. More over, we directly write that we have filter, and if 
parameter is omitted
it's node-name is autogenerated. So actually, the fact the filter is hidden 
when filter-node-name is
unspecified is _undocumented_.

So, finally, it looks like nothing to deprecated in specification, we can just 
drop implicit filters :)

What do you think?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>   qemu-deprecated.texi  |  7 +++
>   qapi/block-core.json  |  6 --
>   include/block/block_int.h | 10 +-
>   blockdev.c| 10 ++
>   4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2753fafd0b..8222440148 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets in 
> server mode
>   
>   Use blockdev-mirror and blockdev-backup instead.
>   
> +@subsection implicit filters (since 4.2)
> +
> +Mirror and commit jobs inserts filters, which becomes implicit if user
> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> +always. Note, that drive-mirror don't have this parameter, so it will
> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> +
>   @section Human Monitor Protocol (HMP) commands
>   
>   @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
> 3.1)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4e35526634..0505ac9d8b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1596,7 +1596,8 @@
>   # @filter-node-name: the node name that should be assigned to the
>   #filter driver that the commit job inserts into the 
> graph
>   #above @top. If this option is not given, a node name is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>   #
>   # @auto-finalize: When false, this job will wait in a PENDING state after 
> it has
>   # finished its work, waiting for @block-job-finalize before
> @@ -2249,7 +2250,8 @@
>   # @filter-node-name: the node name that should be assigned to the
>   #filter driver that the mirror job inserts into the 
> graph
>   #above @device. If this option is not given, a node 
> name is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>   #
>   # @copy-mode: when to copy data to the destination; defaults to 'background'
>   # (Since: 3.0)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..624da0b4a2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -762,7 +762,15 @@ struct BlockDriverState {
>   bool sg;/* if true, the device is a /dev/sg* */
>   bool probed;/* if true, format was probed rather than specified */
>   bool force_share; /* if true, always allow all shared permissions */
> -bool implicit;  /* if true, this filter node was automatically inserted 
> */
> +
> +/*
> + * @implicit field is deprecated, don't set it to true for new filters.
> + * If true, this filter node was automatically inserted and user don't
> + * know about it and unprepared for any effects of it. So, implicit
> + * filters are workarounded and skipped in many places of the block
> + * layer code.
> + */
> +bool implicit;
>   
>   

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-16 Thread Vladimir Sementsov-Ogievskiy
16.08.2019 15:33, Markus Armbruster wrote:
> Kevin Wolf  writes:
> 
>> Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
> [...]
>>> Let's assume all libvirt ever does with deprecation notices is logging
>>> them.  Would that solve the problem of reliably alerting libvirt
>>> developers to deprecation issues?  Nope.  But it could help
>>> occasionally.
>>
>> I'm not saying that deprecation notices would hurt, just that they
>> probably won't solve problem alone.
> 
> No argument.
> 
>> Crashing if --future is given and logging otherwise seems reasonable
>> enough to me. Whether we need to wire up a new deprecation mechanism in
>> QMP for the logging or if we can just keep printing to stderr is
>> debatable. stderr already ends up in a log file, a QMP extension would
>> require new libvirt code. If libvirt would log deprecation notices more
>> prominently, or use the information for tainting or any other kind of
>> processing, a dedicated QMP mechanism could be justified.
> 
> I'd like to start with two tasks:
> 
> * A CLI option to configure what to do on use of a deprecated feature.
> 
>We currently warn.  We want to be able to crash instead.  Silencing
>the warnings might be useful.  Turning them into errors might be
>useful.
> 
>The existing ad hoc warnings need to be replaced by a call of a common
>function that implements the configurable behavior.
> 
> * QAPI feature flag "deprecated", for introspectable deprecation, and
>without ad hoc code.
> 
> Then see whether our users need more.
> 

Crashing is useful for libvirt developers, it's obvious, just enable 
crash-on-deprecated
on all testing environments and most probably we will not miss such a case.

For qapi I doubt is it really needed. Implementing code in libvirt which will 
check for command
(or it's parameter, or it's parameter "optionality" is deprecated) ? It's hard 
and what libvirt
should report to final user? It becomes a kind of synthetic error in libvirt 
code, like

...
log_error("We are going to divide by zero. It's a bug, please report it to 
developers!");
x = a / 0;
...

It's simpler to fix second line than implement special mechanism including 
protocol specification
to report such a case.

I exaggerate of course with this example, but I doubt that implementing a 
special protocol
for it worth doing. And I think notifying libvirt by email (as Peter said) and 
providing option
"crash-on-deprecated" in Qemu are enough for libvirt developers to prevent and 
to fix using
deprecated things.

In other words, I don't see why reporting deprecated feature usage is better in 
libvirt than in
Qemu (by warning, error or crash), and in Qemu it's much more simple and don't 
need QAPI protocol
extension.

(I'm sorry if I'm repeating already written arguments, I've not read the whole 
thread)

-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-16 Thread Markus Armbruster
Kevin Wolf  writes:

> Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
[...]
>> Let's assume all libvirt ever does with deprecation notices is logging
>> them.  Would that solve the problem of reliably alerting libvirt
>> developers to deprecation issues?  Nope.  But it could help
>> occasionally.
>
> I'm not saying that deprecation notices would hurt, just that they
> probably won't solve problem alone.

No argument.

> Crashing if --future is given and logging otherwise seems reasonable
> enough to me. Whether we need to wire up a new deprecation mechanism in
> QMP for the logging or if we can just keep printing to stderr is
> debatable. stderr already ends up in a log file, a QMP extension would
> require new libvirt code. If libvirt would log deprecation notices more
> prominently, or use the information for tainting or any other kind of
> processing, a dedicated QMP mechanism could be justified.

I'd like to start with two tasks:

* A CLI option to configure what to do on use of a deprecated feature.

  We currently warn.  We want to be able to crash instead.  Silencing
  the warnings might be useful.  Turning them into errors might be
  useful.

  The existing ad hoc warnings need to be replaced by a call of a common
  function that implements the configurable behavior.

* QAPI feature flag "deprecated", for introspectable deprecation, and
  without ad hoc code.

Then see whether our users need more.



Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-16 Thread Kevin Wolf
Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 15.08.2019 um 18:07 hat John Snow geschrieben:
> >> 
> >> 
> >> On 8/15/19 6:49 AM, Kevin Wolf wrote:
> >> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
> >> >>
> >> >>
> >> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> >> >>> To get rid of implicit filters related workarounds in future let's
> >> >>> deprecate them now.
> >> >>>
> >> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> >> >>> ---
> >> >>>  qemu-deprecated.texi  |  7 +++
> >> >>>  qapi/block-core.json  |  6 --
> >> >>>  include/block/block_int.h | 10 +-
> >> >>>  blockdev.c| 10 ++
> >> >>>  4 files changed, 30 insertions(+), 3 deletions(-)
> >> >>>
> >> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >> >>> index 2753fafd0b..8222440148 100644
> >> >>> --- a/qemu-deprecated.texi
> >> >>> +++ b/qemu-deprecated.texi
> >> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to 
> >> >>> sockets in server mode
> >> >>>  
> >> >>>  Use blockdev-mirror and blockdev-backup instead.
> >> >>>  
> >> >>> +@subsection implicit filters (since 4.2)
> >> >>> +
> >> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
> >> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set 
> >> >>> it
> >> >>> +always. Note, that drive-mirror don't have this parameter, so it will
> >> >>> +create implicit filter anyway, but drive-mirror is deprecated itself 
> >> >>> too.
> >> >>> +
> >> >>>  @section Human Monitor Protocol (HMP) commands
> >> >>>  
> >> >>>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
> >> >>> (since 3.1)
> >> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >>> index 4e35526634..0505ac9d8b 100644
> >> >>> --- a/qapi/block-core.json
> >> >>> +++ b/qapi/block-core.json
> >> >>> @@ -1596,7 +1596,8 @@
> >> >>>  # @filter-node-name: the node name that should be assigned to the
> >> >>>  #filter driver that the commit job inserts into 
> >> >>> the graph
> >> >>>  #above @top. If this option is not given, a node 
> >> >>> name is
> >> >>> -#autogenerated. (Since: 2.9)
> >> >>> +#autogenerated. Omitting this option is 
> >> >>> deprecated, it will
> >> >>> +#be required in future. (Since: 2.9)
> >> >>>  #
> >> >>>  # @auto-finalize: When false, this job will wait in a PENDING state 
> >> >>> after it has
> >> >>>  # finished its work, waiting for @block-job-finalize 
> >> >>> before
> >> >>> @@ -2249,7 +2250,8 @@
> >> >>>  # @filter-node-name: the node name that should be assigned to the
> >> >>>  #filter driver that the mirror job inserts into 
> >> >>> the graph
> >> >>>  #above @device. If this option is not given, a 
> >> >>> node name is
> >> >>> -#autogenerated. (Since: 2.9)
> >> >>> +#autogenerated. Omitting this option is 
> >> >>> deprecated, it will
> >> >>> +#be required in future. (Since: 2.9)
> >> >>>  #
> >> >>>  # @copy-mode: when to copy data to the destination; defaults to 
> >> >>> 'background'
> >> >>>  # (Since: 3.0)
> >> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> >>> index 3aa1e832a8..624da0b4a2 100644
> >> >>> --- a/include/block/block_int.h
> >> >>> +++ b/include/block/block_int.h
> >> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
> >> >>>  bool sg;/* if true, the device is a /dev/sg* */
> >> >>>  bool probed;/* if true, format was probed rather than 
> >> >>> specified */
> >> >>>  bool force_share; /* if true, always allow all shared permissions 
> >> >>> */
> >> >>> -bool implicit;  /* if true, this filter node was automatically 
> >> >>> inserted */
> >> >>> +
> >> >>> +/*
> >> >>> + * @implicit field is deprecated, don't set it to true for new 
> >> >>> filters.
> >> >>> + * If true, this filter node was automatically inserted and user 
> >> >>> don't
> >> >>> + * know about it and unprepared for any effects of it. So, 
> >> >>> implicit
> >> >>> + * filters are workarounded and skipped in many places of the 
> >> >>> block
> >> >>> + * layer code.
> >> >>> + */
> >> >>> +bool implicit;
> >> >>>  
> >> >>>  BlockDriver *drv; /* NULL means no media */
> >> >>>  void *opaque;
> >> >>> diff --git a/blockdev.c b/blockdev.c
> >> >>> index 36e9368e01..b3cfaccce1 100644
> >> >>> --- a/blockdev.c
> >> >>> +++ b/blockdev.c
> >> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const 
> >> >>> char *job_id, const char *device,
> >> >>>  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> >> >>>  int job_flags = JOB_DEFAULT;
> >> >>>  
> >> >>> +if (!has_filter_node_name) {
> >> >>> +

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-15 Thread Markus Armbruster
Kevin Wolf  writes:

> Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>> 
>> 
>> On 8/15/19 6:49 AM, Kevin Wolf wrote:
>> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>> >>
>> >>
>> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> >>> To get rid of implicit filters related workarounds in future let's
>> >>> deprecate them now.
>> >>>
>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> >>> ---
>> >>>  qemu-deprecated.texi  |  7 +++
>> >>>  qapi/block-core.json  |  6 --
>> >>>  include/block/block_int.h | 10 +-
>> >>>  blockdev.c| 10 ++
>> >>>  4 files changed, 30 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>> >>> index 2753fafd0b..8222440148 100644
>> >>> --- a/qemu-deprecated.texi
>> >>> +++ b/qemu-deprecated.texi
>> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to 
>> >>> sockets in server mode
>> >>>  
>> >>>  Use blockdev-mirror and blockdev-backup instead.
>> >>>  
>> >>> +@subsection implicit filters (since 4.2)
>> >>> +
>> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
>> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
>> >>> +always. Note, that drive-mirror don't have this parameter, so it will
>> >>> +create implicit filter anyway, but drive-mirror is deprecated itself 
>> >>> too.
>> >>> +
>> >>>  @section Human Monitor Protocol (HMP) commands
>> >>>  
>> >>>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
>> >>> (since 3.1)
>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >>> index 4e35526634..0505ac9d8b 100644
>> >>> --- a/qapi/block-core.json
>> >>> +++ b/qapi/block-core.json
>> >>> @@ -1596,7 +1596,8 @@
>> >>>  # @filter-node-name: the node name that should be assigned to the
>> >>>  #filter driver that the commit job inserts into the 
>> >>> graph
>> >>>  #above @top. If this option is not given, a node 
>> >>> name is
>> >>> -#autogenerated. (Since: 2.9)
>> >>> +#autogenerated. Omitting this option is deprecated, 
>> >>> it will
>> >>> +#be required in future. (Since: 2.9)
>> >>>  #
>> >>>  # @auto-finalize: When false, this job will wait in a PENDING state 
>> >>> after it has
>> >>>  # finished its work, waiting for @block-job-finalize 
>> >>> before
>> >>> @@ -2249,7 +2250,8 @@
>> >>>  # @filter-node-name: the node name that should be assigned to the
>> >>>  #filter driver that the mirror job inserts into the 
>> >>> graph
>> >>>  #above @device. If this option is not given, a node 
>> >>> name is
>> >>> -#autogenerated. (Since: 2.9)
>> >>> +#autogenerated. Omitting this option is deprecated, 
>> >>> it will
>> >>> +#be required in future. (Since: 2.9)
>> >>>  #
>> >>>  # @copy-mode: when to copy data to the destination; defaults to 
>> >>> 'background'
>> >>>  # (Since: 3.0)
>> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> >>> index 3aa1e832a8..624da0b4a2 100644
>> >>> --- a/include/block/block_int.h
>> >>> +++ b/include/block/block_int.h
>> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
>> >>>  bool sg;/* if true, the device is a /dev/sg* */
>> >>>  bool probed;/* if true, format was probed rather than specified 
>> >>> */
>> >>>  bool force_share; /* if true, always allow all shared permissions */
>> >>> -bool implicit;  /* if true, this filter node was automatically 
>> >>> inserted */
>> >>> +
>> >>> +/*
>> >>> + * @implicit field is deprecated, don't set it to true for new 
>> >>> filters.
>> >>> + * If true, this filter node was automatically inserted and user 
>> >>> don't
>> >>> + * know about it and unprepared for any effects of it. So, implicit
>> >>> + * filters are workarounded and skipped in many places of the block
>> >>> + * layer code.
>> >>> + */
>> >>> +bool implicit;
>> >>>  
>> >>>  BlockDriver *drv; /* NULL means no media */
>> >>>  void *opaque;
>> >>> diff --git a/blockdev.c b/blockdev.c
>> >>> index 36e9368e01..b3cfaccce1 100644
>> >>> --- a/blockdev.c
>> >>> +++ b/blockdev.c
>> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
>> >>> *job_id, const char *device,
>> >>>  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>> >>>  int job_flags = JOB_DEFAULT;
>> >>>  
>> >>> +if (!has_filter_node_name) {
>> >>> +warn_report("Omitting filter-node-name parameter is deprecated, 
>> >>> it "
>> >>> +"will be required in future");
>> >>> +}
>> >>> +
>> >>>  if (!has_speed) {
>> >>>  speed = 0;
>> >>>  }
>> >>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const 
>> >>> char *job_id,
>> >>> 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-15 Thread John Snow
On 8/15/19 12:48 PM, Kevin Wolf wrote:
> Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>> On 8/15/19 6:49 AM, Kevin Wolf wrote:
>>> Am 14.08.2019 um 21:27 hat John Snow geschrieben:

 This might be OK to do right away, though.

 I asked Markus this not too long ago; do we want to amend the QAPI
 schema specification to allow commands to return with "Warning" strings,
 or "Deprecated" stings to allow in-band deprecation notices for cases
 like these?

 example:

 { "return": {},
   "deprecated": True,
   "warning": "Omitting filter-node-name parameter is deprecated, it will
 be required in the future"
 }

 There's no "error" key, so this should be recognized as success by
 compatible clients, but they'll definitely see the extra information.

 Part of my motivation is to facilitate a more aggressive deprecation of
 legacy features by ensuring that we are able to rigorously notify users
 through any means that they need to adjust their scripts.
>>>
>>> Who would read this, though? In the best case it ends up deep in a
>>> libvirt log that nobody will look at because there was no error. In the
>>> more common case, the debug level is configured so that QMP traffic
>>> isn't even logged.
>>>
>>> Kevin
>>>
>>
>> I believe you are right, but I also can't shake the feeling that this
>> attitude ensures that we'll never find a way to expose this information
>> to the end-user. Is this not too defeatist?
> 
> I think the discussed approach that seemed most likely to me to succeed
> was adding a command line option that makes QEMU just crash if you use a
> deprecated feature, and enable that in libvirt test cases (or possibly
> even any non-release builds, though maybe it's a bit harsh there).
> 
>> I think deprecation notices in the QMP stream has two benefits:
>>
>> 1) Any direct usages via qmp-shell or manual JSON connection are likely
>> to see this message in development or testing. I feel the usage of QEMU
>> directly is more likely to increase with time as other stacks seek to
>> work around libvirt.
>>
>> [Whether or not they should is another question, but I believe the
>> current reality to be that people are trying to.]
> 
> I don't know about other people, but as a human user, I don't care about
> deprecation notices. As long as something works, I use it, and once I
> get an error message back, I'll use something else.
> 
> If I manually enter drive_mirror and get a warning back, that doesn't
> tell me that libvirt still does the same thing and needs to be fixed. It
> just tells me that in the future I might need to change the commands
> that I use manually.
> 

That the message we return needs to be *useful* doesn't sound like a
count against it.

> I guess this would still prevent adding new libvirt features that build
> on deprecated QEMU features because some manual testing will be involved
> there. But was this ever a problem?
> 

No, because until recently we didn't deprecate anything.

>> 2) Programmatic deprecation notices can't be presented to a user at all
>> if we don't send them; at least this way it becomes libvirt's problem
>> over what to do with them. Perhaps even just in testing and regression
>> suites libvirt can assert that it sees no deprecation warnings (or
>> whitelist certain ones it knows about.)
>>
>> In the case of libvirt, it's not even necessarily about making sure the
>> end user sees it, because it isn't even necessarily the user's fault --
>> it's libvirt's. This is a sure-fire programmatic way to communicate
>> compatibility changes to libvirt.
> 
> If libvirt uses this to make test cases fail, it could work.
> 

Yeah, I think there's solid use there. I'll continue along in Markus's
thread.

> Kevin
> 



Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-15 Thread Kevin Wolf
Am 15.08.2019 um 18:07 hat John Snow geschrieben:
> 
> 
> On 8/15/19 6:49 AM, Kevin Wolf wrote:
> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
> >>
> >>
> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>> To get rid of implicit filters related workarounds in future let's
> >>> deprecate them now.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> >>> ---
> >>>  qemu-deprecated.texi  |  7 +++
> >>>  qapi/block-core.json  |  6 --
> >>>  include/block/block_int.h | 10 +-
> >>>  blockdev.c| 10 ++
> >>>  4 files changed, 30 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >>> index 2753fafd0b..8222440148 100644
> >>> --- a/qemu-deprecated.texi
> >>> +++ b/qemu-deprecated.texi
> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to 
> >>> sockets in server mode
> >>>  
> >>>  Use blockdev-mirror and blockdev-backup instead.
> >>>  
> >>> +@subsection implicit filters (since 4.2)
> >>> +
> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> >>> +always. Note, that drive-mirror don't have this parameter, so it will
> >>> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> >>> +
> >>>  @section Human Monitor Protocol (HMP) commands
> >>>  
> >>>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
> >>> (since 3.1)
> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>> index 4e35526634..0505ac9d8b 100644
> >>> --- a/qapi/block-core.json
> >>> +++ b/qapi/block-core.json
> >>> @@ -1596,7 +1596,8 @@
> >>>  # @filter-node-name: the node name that should be assigned to the
> >>>  #filter driver that the commit job inserts into the 
> >>> graph
> >>>  #above @top. If this option is not given, a node 
> >>> name is
> >>> -#autogenerated. (Since: 2.9)
> >>> +#autogenerated. Omitting this option is deprecated, 
> >>> it will
> >>> +#be required in future. (Since: 2.9)
> >>>  #
> >>>  # @auto-finalize: When false, this job will wait in a PENDING state 
> >>> after it has
> >>>  # finished its work, waiting for @block-job-finalize 
> >>> before
> >>> @@ -2249,7 +2250,8 @@
> >>>  # @filter-node-name: the node name that should be assigned to the
> >>>  #filter driver that the mirror job inserts into the 
> >>> graph
> >>>  #above @device. If this option is not given, a node 
> >>> name is
> >>> -#autogenerated. (Since: 2.9)
> >>> +#autogenerated. Omitting this option is deprecated, 
> >>> it will
> >>> +#be required in future. (Since: 2.9)
> >>>  #
> >>>  # @copy-mode: when to copy data to the destination; defaults to 
> >>> 'background'
> >>>  # (Since: 3.0)
> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >>> index 3aa1e832a8..624da0b4a2 100644
> >>> --- a/include/block/block_int.h
> >>> +++ b/include/block/block_int.h
> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
> >>>  bool sg;/* if true, the device is a /dev/sg* */
> >>>  bool probed;/* if true, format was probed rather than specified 
> >>> */
> >>>  bool force_share; /* if true, always allow all shared permissions */
> >>> -bool implicit;  /* if true, this filter node was automatically 
> >>> inserted */
> >>> +
> >>> +/*
> >>> + * @implicit field is deprecated, don't set it to true for new 
> >>> filters.
> >>> + * If true, this filter node was automatically inserted and user 
> >>> don't
> >>> + * know about it and unprepared for any effects of it. So, implicit
> >>> + * filters are workarounded and skipped in many places of the block
> >>> + * layer code.
> >>> + */
> >>> +bool implicit;
> >>>  
> >>>  BlockDriver *drv; /* NULL means no media */
> >>>  void *opaque;
> >>> diff --git a/blockdev.c b/blockdev.c
> >>> index 36e9368e01..b3cfaccce1 100644
> >>> --- a/blockdev.c
> >>> +++ b/blockdev.c
> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
> >>> *job_id, const char *device,
> >>>  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> >>>  int job_flags = JOB_DEFAULT;
> >>>  
> >>> +if (!has_filter_node_name) {
> >>> +warn_report("Omitting filter-node-name parameter is deprecated, 
> >>> it "
> >>> +"will be required in future");
> >>> +}
> >>> +
> >>>  if (!has_speed) {
> >>>  speed = 0;
> >>>  }
> >>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const 
> >>> char *job_id,
> >>>  Error *local_err = NULL;
> >>>  int ret;
> >>>  
> >>> +if (!has_filter_node_name) {
> >>> +warn_report("Omitting filter-node-name 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-15 Thread John Snow



On 8/15/19 6:49 AM, Kevin Wolf wrote:
> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>>
>>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> To get rid of implicit filters related workarounds in future let's
>>> deprecate them now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>  qemu-deprecated.texi  |  7 +++
>>>  qapi/block-core.json  |  6 --
>>>  include/block/block_int.h | 10 +-
>>>  blockdev.c| 10 ++
>>>  4 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>> index 2753fafd0b..8222440148 100644
>>> --- a/qemu-deprecated.texi
>>> +++ b/qemu-deprecated.texi
>>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets 
>>> in server mode
>>>  
>>>  Use blockdev-mirror and blockdev-backup instead.
>>>  
>>> +@subsection implicit filters (since 4.2)
>>> +
>>> +Mirror and commit jobs inserts filters, which becomes implicit if user
>>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
>>> +always. Note, that drive-mirror don't have this parameter, so it will
>>> +create implicit filter anyway, but drive-mirror is deprecated itself too.
>>> +
>>>  @section Human Monitor Protocol (HMP) commands
>>>  
>>>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
>>> (since 3.1)
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 4e35526634..0505ac9d8b 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1596,7 +1596,8 @@
>>>  # @filter-node-name: the node name that should be assigned to the
>>>  #filter driver that the commit job inserts into the 
>>> graph
>>>  #above @top. If this option is not given, a node name 
>>> is
>>> -#autogenerated. (Since: 2.9)
>>> +#autogenerated. Omitting this option is deprecated, it 
>>> will
>>> +#be required in future. (Since: 2.9)
>>>  #
>>>  # @auto-finalize: When false, this job will wait in a PENDING state after 
>>> it has
>>>  # finished its work, waiting for @block-job-finalize before
>>> @@ -2249,7 +2250,8 @@
>>>  # @filter-node-name: the node name that should be assigned to the
>>>  #filter driver that the mirror job inserts into the 
>>> graph
>>>  #above @device. If this option is not given, a node 
>>> name is
>>> -#autogenerated. (Since: 2.9)
>>> +#autogenerated. Omitting this option is deprecated, it 
>>> will
>>> +#be required in future. (Since: 2.9)
>>>  #
>>>  # @copy-mode: when to copy data to the destination; defaults to 
>>> 'background'
>>>  # (Since: 3.0)
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 3aa1e832a8..624da0b4a2 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -762,7 +762,15 @@ struct BlockDriverState {
>>>  bool sg;/* if true, the device is a /dev/sg* */
>>>  bool probed;/* if true, format was probed rather than specified */
>>>  bool force_share; /* if true, always allow all shared permissions */
>>> -bool implicit;  /* if true, this filter node was automatically 
>>> inserted */
>>> +
>>> +/*
>>> + * @implicit field is deprecated, don't set it to true for new filters.
>>> + * If true, this filter node was automatically inserted and user don't
>>> + * know about it and unprepared for any effects of it. So, implicit
>>> + * filters are workarounded and skipped in many places of the block
>>> + * layer code.
>>> + */
>>> +bool implicit;
>>>  
>>>  BlockDriver *drv; /* NULL means no media */
>>>  void *opaque;
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 36e9368e01..b3cfaccce1 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
>>> *job_id, const char *device,
>>>  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>>  int job_flags = JOB_DEFAULT;
>>>  
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it 
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>>  if (!has_speed) {
>>>  speed = 0;
>>>  }
>>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
>>> *job_id,
>>>  Error *local_err = NULL;
>>>  int ret;
>>>  
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it 
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>>  bs = qmp_get_root_bs(device, errp);
>>>  if (!bs) {
>>>  return;
>>>
>>
>> This might be OK to do right away, though.
>>
>> I asked Markus this not too long ago; do we want to amend 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-15 Thread Kevin Wolf
Am 14.08.2019 um 21:27 hat John Snow geschrieben:
> 
> 
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > To get rid of implicit filters related workarounds in future let's
> > deprecate them now.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > ---
> >  qemu-deprecated.texi  |  7 +++
> >  qapi/block-core.json  |  6 --
> >  include/block/block_int.h | 10 +-
> >  blockdev.c| 10 ++
> >  4 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 2753fafd0b..8222440148 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets 
> > in server mode
> >  
> >  Use blockdev-mirror and blockdev-backup instead.
> >  
> > +@subsection implicit filters (since 4.2)
> > +
> > +Mirror and commit jobs inserts filters, which becomes implicit if user
> > +omitted filter-node-name parameter. So omitting it is deprecated, set it
> > +always. Note, that drive-mirror don't have this parameter, so it will
> > +create implicit filter anyway, but drive-mirror is deprecated itself too.
> > +
> >  @section Human Monitor Protocol (HMP) commands
> >  
> >  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
> > (since 3.1)
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 4e35526634..0505ac9d8b 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1596,7 +1596,8 @@
> >  # @filter-node-name: the node name that should be assigned to the
> >  #filter driver that the commit job inserts into the 
> > graph
> >  #above @top. If this option is not given, a node name 
> > is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it 
> > will
> > +#be required in future. (Since: 2.9)
> >  #
> >  # @auto-finalize: When false, this job will wait in a PENDING state after 
> > it has
> >  # finished its work, waiting for @block-job-finalize before
> > @@ -2249,7 +2250,8 @@
> >  # @filter-node-name: the node name that should be assigned to the
> >  #filter driver that the mirror job inserts into the 
> > graph
> >  #above @device. If this option is not given, a node 
> > name is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it 
> > will
> > +#be required in future. (Since: 2.9)
> >  #
> >  # @copy-mode: when to copy data to the destination; defaults to 
> > 'background'
> >  # (Since: 3.0)
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 3aa1e832a8..624da0b4a2 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -762,7 +762,15 @@ struct BlockDriverState {
> >  bool sg;/* if true, the device is a /dev/sg* */
> >  bool probed;/* if true, format was probed rather than specified */
> >  bool force_share; /* if true, always allow all shared permissions */
> > -bool implicit;  /* if true, this filter node was automatically 
> > inserted */
> > +
> > +/*
> > + * @implicit field is deprecated, don't set it to true for new filters.
> > + * If true, this filter node was automatically inserted and user don't
> > + * know about it and unprepared for any effects of it. So, implicit
> > + * filters are workarounded and skipped in many places of the block
> > + * layer code.
> > + */
> > +bool implicit;
> >  
> >  BlockDriver *drv; /* NULL means no media */
> >  void *opaque;
> > diff --git a/blockdev.c b/blockdev.c
> > index 36e9368e01..b3cfaccce1 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
> > *job_id, const char *device,
> >  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> >  int job_flags = JOB_DEFAULT;
> >  
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it 
> > "
> > +"will be required in future");
> > +}
> > +
> >  if (!has_speed) {
> >  speed = 0;
> >  }
> > @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
> > *job_id,
> >  Error *local_err = NULL;
> >  int ret;
> >  
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it 
> > "
> > +"will be required in future");
> > +}
> > +
> >  bs = qmp_get_root_bs(device, errp);
> >  if (!bs) {
> >  return;
> > 
> 
> This might be OK to do right away, though.
> 
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow 

Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

2019-08-14 Thread John Snow



On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qemu-deprecated.texi  |  7 +++
>  qapi/block-core.json  |  6 --
>  include/block/block_int.h | 10 +-
>  blockdev.c| 10 ++
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2753fafd0b..8222440148 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets in 
> server mode
>  
>  Use blockdev-mirror and blockdev-backup instead.
>  
> +@subsection implicit filters (since 4.2)
> +
> +Mirror and commit jobs inserts filters, which becomes implicit if user
> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> +always. Note, that drive-mirror don't have this parameter, so it will
> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> +
>  @section Human Monitor Protocol (HMP) commands
>  
>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
> 3.1)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4e35526634..0505ac9d8b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1596,7 +1596,8 @@
>  # @filter-node-name: the node name that should be assigned to the
>  #filter driver that the commit job inserts into the graph
>  #above @top. If this option is not given, a node name is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>  #
>  # @auto-finalize: When false, this job will wait in a PENDING state after it 
> has
>  # finished its work, waiting for @block-job-finalize before
> @@ -2249,7 +2250,8 @@
>  # @filter-node-name: the node name that should be assigned to the
>  #filter driver that the mirror job inserts into the graph
>  #above @device. If this option is not given, a node name 
> is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>  #
>  # @copy-mode: when to copy data to the destination; defaults to 'background'
>  # (Since: 3.0)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..624da0b4a2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -762,7 +762,15 @@ struct BlockDriverState {
>  bool sg;/* if true, the device is a /dev/sg* */
>  bool probed;/* if true, format was probed rather than specified */
>  bool force_share; /* if true, always allow all shared permissions */
> -bool implicit;  /* if true, this filter node was automatically inserted 
> */
> +
> +/*
> + * @implicit field is deprecated, don't set it to true for new filters.
> + * If true, this filter node was automatically inserted and user don't
> + * know about it and unprepared for any effects of it. So, implicit
> + * filters are workarounded and skipped in many places of the block
> + * layer code.
> + */
> +bool implicit;
>  
>  BlockDriver *drv; /* NULL means no media */
>  void *opaque;
> diff --git a/blockdev.c b/blockdev.c
> index 36e9368e01..b3cfaccce1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>  int job_flags = JOB_DEFAULT;
>  
> +if (!has_filter_node_name) {
> +warn_report("Omitting filter-node-name parameter is deprecated, it "
> +"will be required in future");
> +}
> +
>  if (!has_speed) {
>  speed = 0;
>  }
> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
> *job_id,
>  Error *local_err = NULL;
>  int ret;
>  
> +if (!has_filter_node_name) {
> +warn_report("Omitting filter-node-name parameter is deprecated, it "
> +"will be required in future");
> +}
> +
>  bs = qmp_get_root_bs(device, errp);
>  if (!bs) {
>  return;
> 

This might be OK to do right away, though.

I asked Markus this not too long ago; do we want to amend the QAPI
schema specification to allow commands to return with "Warning" strings,
or "Deprecated" stings to allow in-band deprecation notices for cases
like these?

example:

{ "return": {},
  "deprecated": True,
  "warning": "Omitting filter-node-name parameter is deprecated, it will
be required in the future"
}

There's no "error" key, so this should be