[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Fix the error message for scsi host device's shareable checking



On 30/01/14 18:22, Jiri Denemark wrote:
On Thu, Jan 30, 2014 at 16:58:18 +0800, Osier Yang wrote:
This fixes the wrong argument order.

---
Pushed under trivial rule.
---
  src/qemu/qemu_hostdev.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 2b11d6c..1b16386 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -1135,8 +1135,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
                  virReportError(VIR_ERR_OPERATION_INVALID,
                                 _("SCSI device %s is already in use by "
                                   "other domain(s) as '%s'"),
-                               tmp_shareable ? "shareable" : "non-shareable",
-                               virSCSIDeviceGetName(tmp));
+                               virSCSIDeviceGetName(tmp),
+                               tmp_shareable ? "shareable" : "non-shareable");
                  goto error;
              }
While this fixes wrong argument order, it is still wrong because it's
untranslatable. The code should be rewritten as

     if (tmp_shareable) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        _("SCSI device shareable is already in use by "
                          "other domain(s) as '%s'"),
                        virSCSIDeviceGetName(tmp));
     } else {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        _("SCSI device non-shareable is already in use by "
                          "other domain(s) as '%s'"),
                        virSCSIDeviceGetName(tmp));
     }

I thought whether to translate the two strings ("shareable" and "non-shareable") is trivial, so choosed the conditional operator instead of if...else. But if you keep
thinking it should be changed, I will do.


Not to mention that the error message itself doesn't make a lot of sense
to me... Did you wanted to say something else, e.g.:

     if (scsi_shareable && !tmp_shareable) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        _("Shareable SCSI device '%s' is already in "
                          "use by other domain(s) as non-shareable "
                          "device '%s'"),
                        virSCSIDeviceGetName(scsi),
                        virSCSIDeviceGetName(tmp));
     } else if (!scsi_shareable) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        _("Non-shareable SCSI device '%s' is already in "
                          "use by other domain(s) as device '%s'"),
                        virSCSIDeviceGetName(scsi),
                        virSCSIDeviceGetName(tmp));
     }



With this, the error message will be like:

"Shareable SCSI device '1:0:0:0' is already in use by other domain(s) as non-shareable device '1:0:0:0'"

IMO it's a bit redundant. And I'm not sure if in some cases the SCSI device itself just cannot be shared, so personaly I'd like not use words like "Shareable SCSI device",
which may introduce confusion.

Thanks.

Osier


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]