[PATCH v2 0/1] IDE: Deprecate ide-drive

[PATCH v2 0/1] IDE: Deprecate ide-drive

[John Snow]

V2: Change phrasings and spellings as Markus suggested.

John Snow (1):
  IDE: deprecate ide-drive

 qemu-deprecated.texi          | 5 +++++
 hw/ide/qdev.c                 | 3 +++
 tests/qemu-iotests/051.pc.out | 6 ++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.21.0

[PATCH v2 1/1] IDE: deprecate ide-drive

[John Snow]

It's an old compatibility shim that just delegates to ide-cd or ide-hd.
I'd like to refactor these some day, and getting rid of the super-object
will make that easier.

Either way, we don't need this.

Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-deprecated.texi          | 5 +++++
 hw/ide/qdev.c                 | 3 +++
 tests/qemu-iotests/051.pc.out | 6 ++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 01245e0b1c4..7cd4648df3c 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -247,6 +247,11 @@ quite a bit. It will be removed without replacement unless some users speaks
 up at the @email{qemu-devel@@nongnu.org} mailing list with information about
 their usecases.
 
+@subsection ide-drive (since 4.2)
+
+The 'ide-drive' device is deprecated. Users should use 'ide-hd' or
+'ide-cd' as appropriate to get an IDE hard disk or CD-ROM as needed.
+
 @section System emulator machines
 
 @subsection pc-0.12, pc-0.13, pc-0.14 and pc-0.15 (since 4.0)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6fba6b62b87..3666e597211 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -279,6 +279,9 @@ static void ide_drive_realize(IDEDevice *dev, Error **errp)
 {
     DriveInfo *dinfo = NULL;
 
+    warn_report("'ide-drive' is deprecated, "
+                "please use 'ide-hd' or 'ide-cd' instead");
+
     if (dev->conf.blk) {
         dinfo = blk_legacy_dinfo(dev->conf.blk);
     }
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 000557c7c83..34849dd1720 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -158,7 +158,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=none,id=disk -device ide-drive,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-drive,drive=disk: Device needs media, but drive is empty
+(qemu) QEMU_PROG: -device ide-drive,drive=disk: warning: 'ide-drive' is deprecated, please use 'ide-hd' or 'ide-cd' instead
+QEMU_PROG: -device ide-drive,drive=disk: Device needs media, but drive is empty
 
 Testing: -drive if=none,id=disk -device ide-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -228,7 +229,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device ide-drive,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-drive,drive=disk: Block node is read-only
+(qemu) QEMU_PROG: -device ide-drive,drive=disk: warning: 'ide-drive' is deprecated, please use 'ide-hd' or 'ide-cd' instead
+QEMU_PROG: -device ide-drive,drive=disk: Block node is read-only
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device ide-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.21.0

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Thomas Huth]

On 10/10/2019 00.43, John Snow wrote:
> It's an old compatibility shim that just delegates to ide-cd or ide-hd.
> I'd like to refactor these some day, and getting rid of the super-object
> will make that easier.
> 
> Either way, we don't need this.
> 
> Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qemu-deprecated.texi          | 5 +++++
>  hw/ide/qdev.c                 | 3 +++
>  tests/qemu-iotests/051.pc.out | 6 ++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> It's an old compatibility shim that just delegates to ide-cd or ide-hd.
> I'd like to refactor these some day, and getting rid of the super-object
> will make that easier.
>
> Either way, we don't need this.
>
> Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Philippe Mathieu-Daudé]

On 10/10/19 12:43 AM, John Snow wrote:
> It's an old compatibility shim that just delegates to ide-cd or ide-hd.
> I'd like to refactor these some day, and getting rid of the super-object
> will make that easier.
> 
> Either way, we don't need this.
> 
> Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>

Peter made a comment regarding Laszlo's Regression-tested-by tag:

   [...] nobody else is using
   this convention (there are exactly 0 instances of
   "Regression-tested-by" in the project git log as far as
   I can see), and so in practice people reading the commits
   won't really know what you meant by it. Everybody else
   on the project uses "Tested-by" to mean either of the
   two cases you describe above, without distinction...

It probably applies to 'Libvirt-checked-by' too.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg632705.html

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Peter Krempa]

On Thu, Oct 10, 2019 at 13:22:37 +0200, Philippe Mathieu-Daudé wrote:
> On 10/10/19 12:43 AM, John Snow wrote:
> > It's an old compatibility shim that just delegates to ide-cd or ide-hd.
> > I'd like to refactor these some day, and getting rid of the super-object
> > will make that easier.
> > 
> > Either way, we don't need this.
> > 
> > Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
> 
> Peter made a comment regarding Laszlo's Regression-tested-by tag:
> 
>   [...] nobody else is using
>   this convention (there are exactly 0 instances of
>   "Regression-tested-by" in the project git log as far as
>   I can see), and so in practice people reading the commits
>   won't really know what you meant by it. Everybody else
>   on the project uses "Tested-by" to mean either of the
>   two cases you describe above, without distinction...
> 
> It probably applies to 'Libvirt-checked-by' too.

I certainly didn't test it. So feel free to drop that line altogether.

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Philippe Mathieu-Daudé]

On 10/10/19 1:26 PM, Peter Krempa wrote:
> On Thu, Oct 10, 2019 at 13:22:37 +0200, Philippe Mathieu-Daudé wrote:
>> On 10/10/19 12:43 AM, John Snow wrote:
>>> It's an old compatibility shim that just delegates to ide-cd or ide-hd.
>>> I'd like to refactor these some day, and getting rid of the super-object
>>> will make that easier.
>>>
>>> Either way, we don't need this.
>>>
>>> Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
>>
>> Peter made a comment regarding Laszlo's Regression-tested-by tag:
>>
>>    [...] nobody else is using
>>    this convention (there are exactly 0 instances of
>>    "Regression-tested-by" in the project git log as far as
>>    I can see), and so in practice people reading the commits
>>    won't really know what you meant by it. Everybody else
>>    on the project uses "Tested-by" to mean either of the
>>    two cases you describe above, without distinction...
>>
>> It probably applies to 'Libvirt-checked-by' too.
> 
> I certainly didn't test it. So feel free to drop that line altogether.

But you reviewed it, can we use your 'Reviewed-by' instead?

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Peter Krempa]

On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
> On 10/10/19 1:26 PM, Peter Krempa wrote:
> > On Thu, Oct 10, 2019 at 13:22:37 +0200, Philippe Mathieu-Daudé wrote:
> > > On 10/10/19 12:43 AM, John Snow wrote:
> > > > It's an old compatibility shim that just delegates to ide-cd or ide-hd.
> > > > I'd like to refactor these some day, and getting rid of the super-object
> > > > will make that easier.
> > > > 
> > > > Either way, we don't need this.
> > > > 
> > > > Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
> > > 
> > > Peter made a comment regarding Laszlo's Regression-tested-by tag:
> > > 
> > >    [...] nobody else is using
> > >    this convention (there are exactly 0 instances of
> > >    "Regression-tested-by" in the project git log as far as
> > >    I can see), and so in practice people reading the commits
> > >    won't really know what you meant by it. Everybody else
> > >    on the project uses "Tested-by" to mean either of the
> > >    two cases you describe above, without distinction...
> > > 
> > > It probably applies to 'Libvirt-checked-by' too.
> > 
> > I certainly didn't test it. So feel free to drop that line altogether.
> 
> But you reviewed it, can we use your 'Reviewed-by' instead?

To be honest, I didn't really review the code nor the documentation.
I actually reviewed only the idea itself in the context of integration
with libvirt and that's why I didn't go for 'Reviewed-by:'.

The gist of the citation above is that we should stick to well known
tags with their well known meanings and I think that considering this a
'review' would be a stretch of the definiton.

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Kevin Wolf]

Am 10.10.2019 um 13:54 hat Peter Krempa geschrieben:
> On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
> > On 10/10/19 1:26 PM, Peter Krempa wrote:
> > > On Thu, Oct 10, 2019 at 13:22:37 +0200, Philippe Mathieu-Daudé wrote:
> > > > On 10/10/19 12:43 AM, John Snow wrote:
> > > > > It's an old compatibility shim that just delegates to ide-cd or ide-hd.
> > > > > I'd like to refactor these some day, and getting rid of the super-object
> > > > > will make that easier.
> > > > > 
> > > > > Either way, we don't need this.
> > > > > 
> > > > > Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
> > > > 
> > > > Peter made a comment regarding Laszlo's Regression-tested-by tag:
> > > > 
> > > >    [...] nobody else is using
> > > >    this convention (there are exactly 0 instances of
> > > >    "Regression-tested-by" in the project git log as far as
> > > >    I can see), and so in practice people reading the commits
> > > >    won't really know what you meant by it. Everybody else
> > > >    on the project uses "Tested-by" to mean either of the
> > > >    two cases you describe above, without distinction...
> > > > 
> > > > It probably applies to 'Libvirt-checked-by' too.
> > > 
> > > I certainly didn't test it. So feel free to drop that line altogether.
> > 
> > But you reviewed it, can we use your 'Reviewed-by' instead?
> 
> To be honest, I didn't really review the code nor the documentation.
> I actually reviewed only the idea itself in the context of integration
> with libvirt and that's why I didn't go for 'Reviewed-by:'.
> 
> The gist of the citation above is that we should stick to well known
> tags with their well known meanings and I think that considering this a
> 'review' would be a stretch of the definiton.

I think Acked-by works well for instances like this (i.e. you're just
saying that you agree and the intended change is fine from libvirt's
POV, not that you made any effort to check that the patches are correct
or anything).

Kevin

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[John Snow]

On 10/10/19 7:54 AM, Peter Krempa wrote:
> On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
>> On 10/10/19 1:26 PM, Peter Krempa wrote:
>>> On Thu, Oct 10, 2019 at 13:22:37 +0200, Philippe Mathieu-Daudé wrote:
>>>> On 10/10/19 12:43 AM, John Snow wrote:
>>>>> It's an old compatibility shim that just delegates to ide-cd or ide-hd.
>>>>> I'd like to refactor these some day, and getting rid of the super-object
>>>>> will make that easier.
>>>>>
>>>>> Either way, we don't need this.
>>>>>
>>>>> Libvirt-checked-by: Peter Krempa <pkrempa@redhat.com>
>>>>
>>>> Peter made a comment regarding Laszlo's Regression-tested-by tag:
>>>>
>>>>    [...] nobody else is using
>>>>    this convention (there are exactly 0 instances of
>>>>    "Regression-tested-by" in the project git log as far as
>>>>    I can see), and so in practice people reading the commits
>>>>    won't really know what you meant by it. Everybody else
>>>>    on the project uses "Tested-by" to mean either of the
>>>>    two cases you describe above, without distinction...
>>>>
>>>> It probably applies to 'Libvirt-checked-by' too.
>>>
>>> I certainly didn't test it. So feel free to drop that line altogether.
>>
>> But you reviewed it, can we use your 'Reviewed-by' instead?
> 
> To be honest, I didn't really review the code nor the documentation.
> I actually reviewed only the idea itself in the context of integration
> with libvirt and that's why I didn't go for 'Reviewed-by:'.
> 
> The gist of the citation above is that we should stick to well known
> tags with their well known meanings and I think that considering this a
> 'review' would be a stretch of the definiton.
> 

I wasn't aware that PMM wanted to avoid non-standard tags; I consider
them to be for human use, but I can change that behavior.

Peter, I'll change it to an ACK (as suggested by Kevin) is that's OK by you.

--js

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

On Thu, Oct 10, 2019 at 14:08:12 -0400, John Snow wrote:
> On 10/10/19 7:54 AM, Peter Krempa wrote:
> > On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
> >> On 10/10/19 1:26 PM, Peter Krempa wrote:

[...]

> > To be honest, I didn't really review the code nor the documentation.
> > I actually reviewed only the idea itself in the context of integration
> > with libvirt and that's why I didn't go for 'Reviewed-by:'.
> > 
> > The gist of the citation above is that we should stick to well known
> > tags with their well known meanings and I think that considering this a
> > 'review' would be a stretch of the definiton.
> > 
> 
> I wasn't aware that PMM wanted to avoid non-standard tags; I consider
> them to be for human use, but I can change that behavior.
> 
> Peter, I'll change it to an ACK (as suggested by Kevin) is that's OK by you.

Sure! I'll spell it out even for compliance:

ACKed-by: Peter Krempa <pkrempa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[John Snow]

On 10/11/19 5:12 AM, Peter Krempa wrote:
> On Thu, Oct 10, 2019 at 14:08:12 -0400, John Snow wrote:
>> On 10/10/19 7:54 AM, Peter Krempa wrote:
>>> On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
>>>> On 10/10/19 1:26 PM, Peter Krempa wrote:
> 
> [...]
> 
>>> To be honest, I didn't really review the code nor the documentation.
>>> I actually reviewed only the idea itself in the context of integration
>>> with libvirt and that's why I didn't go for 'Reviewed-by:'.
>>>
>>> The gist of the citation above is that we should stick to well known
>>> tags with their well known meanings and I think that considering this a
>>> 'review' would be a stretch of the definiton.
>>>
>>
>> I wasn't aware that PMM wanted to avoid non-standard tags; I consider
>> them to be for human use, but I can change that behavior.
>>
>> Peter, I'll change it to an ACK (as suggested by Kevin) is that's OK by you.
> 
> Sure! I'll spell it out even for compliance:
> 
> ACKed-by: Peter Krempa <pkrempa@redhat.com>
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js