| Message ID | 20191009224303.10232-2-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IDE: Deprecate ide-drive | expand |
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>
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>
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> > ---
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.
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?
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.
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
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
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>
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
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
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(-)