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

[Osier Yang]

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