[Libvir] [PATCH] Another Report error in virsh.c code.
Hi, I am watching through the virsh code for same type bug check. http://git.et.redhat.com/?p=libvirt.git;a=commit;h=c857ace66df5a5068ed561aad... And I found another point it should report error. Thanks, Shigeki Sakamoto. Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.135 diff -u -p -r1.135 virsh.c --- src/virsh.c 4 Mar 2008 19:59:56 -0000 1.135 +++ src/virsh.c 7 Mar 2008 07:03:12 -0000 @@ -1729,6 +1729,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm } if (!(cpulist = vshCommandOptString(cmd, "cpulist", NULL))) { + vshError(ctl, FALSE, _("vcpupin: Invalid value of cpulist")); virDomainFree(dom); return FALSE; } @@ -1744,6 +1745,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm } if (vcpu >= info.nrVirtCpu) { + vshError(ctl, FALSE, _("vcpupin: Invalid vCPU number.")); virDomainFree(dom); return FALSE; } @@ -4473,6 +4475,7 @@ cmdAttachDevice(vshControl * ctl, vshCmd from = vshCommandOptString(cmd, "file", &found); if (!found) { + vshError(ctl, FALSE, _("attach-device: Invalid value of <file> option")); virDomainFree(dom); return FALSE; } @@ -4529,6 +4532,7 @@ cmdDetachDevice(vshControl * ctl, vshCmd from = vshCommandOptString(cmd, "file", &found); if (!found) { + vshError(ctl, FALSE, _("detach-device: Invalid value of <file> option")); virDomainFree(dom); return FALSE; }
On Fri, Mar 07, 2008 at 04:43:37PM +0900, S.Sakamoto wrote:
Hi,
I am watching through the virsh code for same type bug check. http://git.et.redhat.com/?p=libvirt.git;a=commit;h=c857ace66df5a5068ed561aad... And I found another point it should report error.
Thanks, Shigeki Sakamoto.
Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.135 diff -u -p -r1.135 virsh.c --- src/virsh.c 4 Mar 2008 19:59:56 -0000 1.135 +++ src/virsh.c 7 Mar 2008 07:03:12 -0000 @@ -1729,6 +1729,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm }
if (!(cpulist = vshCommandOptString(cmd, "cpulist", NULL))) { + vshError(ctl, FALSE, _("vcpupin: Invalid value of cpulist")); virDomainFree(dom); return FALSE; }
Is this one necessary? vshCommandOptString prints an error anyway because the cpulist parameter is marked as required, ie: $ virsh vcpupin error: command 'vcpupin' requires <domain> option error: command 'vcpupin' requires <vcpu> option error: command 'vcpupin' requires <cpulist> option
@@ -1744,6 +1745,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm }
if (vcpu >= info.nrVirtCpu) { + vshError(ctl, FALSE, _("vcpupin: Invalid vCPU number.")); virDomainFree(dom); return FALSE; }
+1
@@ -4473,6 +4475,7 @@ cmdAttachDevice(vshControl * ctl, vshCmd
from = vshCommandOptString(cmd, "file", &found); if (!found) { + vshError(ctl, FALSE, _("attach-device: Invalid value of <file> option")); virDomainFree(dom); return FALSE; }
Again, vshCommandOptString prints an error: $ virsh attach-device error: command 'attach-device' requires <domain> option error: command 'attach-device' requires <file> option
@@ -4529,6 +4532,7 @@ cmdDetachDevice(vshControl * ctl, vshCmd
from = vshCommandOptString(cmd, "file", &found); if (!found) { + vshError(ctl, FALSE, _("detach-device: Invalid value of <file> option")); virDomainFree(dom); return FALSE; }
And similarly. Let me know if I'm missing something. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Is this one necessary? vshCommandOptString prints an error anyway because the cpulist parameter is marked as required, ie:
$ virsh vcpupin error: command 'vcpupin' requires <domain> option error: command 'vcpupin' requires <vcpu> option error: command 'vcpupin' requires <cpulist> option
@@ -4473,6 +4475,7 @@ cmdAttachDevice(vshControl * ctl, vshCmd
from = vshCommandOptString(cmd, "file", &found); if (!found) { + vshError(ctl, FALSE, _("attach-device: Invalid value of <file> option")); virDomainFree(dom); return FALSE; }
Again, vshCommandOptString prints an error:
$ virsh attach-device error: command 'attach-device' requires <domain> option error: command 'attach-device' requires <file> option
@@ -4529,6 +4532,7 @@ cmdDetachDevice(vshControl * ctl, vshCmd
from = vshCommandOptString(cmd, "file", &found); if (!found) { + vshError(ctl, FALSE, _("detach-device: Invalid value of <file> option")); virDomainFree(dom); return FALSE; }
And similarly.
Let me know if I'm missing something.
Surely, the message of "Invalid value" is not pertinent since the validity of value is checked at vshCommandCheckOpts and shown the message there. BTW, I think another message is needed here to inform the internal error to user. For example, the migrate command shows the following message when "desturi" is missing: migrate: Missing desturi But it does not show the error-message even though "migrateuri" is missing because "migrateuri" is *not* a necessary option. So, I want to unify "virsh.c" with the following rules, - for necessary option show the message if error occur - for unnecessary option do not show the message even if error occur How do you think ? Regards, Shigeki Sakamoto.
On Tue, Mar 11, 2008 at 05:09:45PM +0900, S.Sakamoto wrote:
BTW, I think another message is needed here to inform the internal error to user. For example, the migrate command shows the following message when "desturi" is missing: migrate: Missing desturi But it does not show the error-message even though "migrateuri" is missing because "migrateuri" is *not* a necessary option.
I'm not sure I follow what is wrong. 'desturi' is required, so if missing we get an error. 'migrateuri' is not required, so there is no error if it is missing. And that's what the code does.
So, I want to unify "virsh.c" with the following rules, - for necessary option show the message if error occur - for unnecessary option do not show the message even if error occur
Do you have a patch or another way to explain this, because I'm afraid I don't follow what you mean. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On Tue, Mar 11, 2008 at 05:09:45PM +0900, S.Sakamoto wrote:
BTW, I think another message is needed here to inform the internal error to user. For example, the migrate command shows the following message when "desturi" is missing: migrate: Missing desturi But it does not show the error-message even though "migrateuri" is missing because "migrateuri" is *not* a necessary option.
I'm not sure I follow what is wrong. 'desturi' is required, so if missing we get an error. 'migrateuri' is not required, so there is no error if it is missing. And that's what the code does.
So, I want to unify "virsh.c" with the following rules, - for necessary option show the message if error occur - for unnecessary option do not show the message even if error occur
Do you have a patch or another way to explain this, because I'm afraid I don't follow what you mean. Sorry, My explanation is not enough...
What I meant to say was that I want to output to output error-message at "vcpupin" as like "migrate". I wish it is unified as follows. ***migrate*** 2225 desturi = vshCommandOptString (cmd, "desturi", &found); 2226 if (!found) { 2227 vshError (ctl, FALSE, "%s", _("migrate: Missing desturi")); 2228 goto done; 2229 } ***vcpupin*** 1731 if (!(cpulist = vshCommandOptString(cmd, "cpulist", NULL))) { + vshError (ctl, FALSE, "%s", _("vcpupin: Missing cpulist")); 1732 virDomainFree(dom); 1733 return FALSE; 1734 } Shigeki Sakamoto.
On Tue, Mar 11, 2008 at 07:19:57PM +0900, S.Sakamoto wrote:
I wish it is unified as follows.
***migrate*** 2225 desturi = vshCommandOptString (cmd, "desturi", &found); 2226 if (!found) { 2227 vshError (ctl, FALSE, "%s", _("migrate: Missing desturi")); 2228 goto done; 2229 }
***vcpupin*** 1731 if (!(cpulist = vshCommandOptString(cmd, "cpulist", NULL))) { + vshError (ctl, FALSE, "%s", _("vcpupin: Missing cpulist")); 1732 virDomainFree(dom); 1733 return FALSE; 1734 }
So the problem is just the text in the error message? ie. Instead of: $ virsh vcpupin error: command 'vcpupin' requires <domain> option error: command 'vcpupin' requires <vcpu> option error: command 'vcpupin' requires <cpulist> option you want it to print a different message, as in: vcpupin: Missing cpulist ? Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
$ virsh vcpupin error: command 'vcpupin' requires <domain> option error: command 'vcpupin' requires <vcpu> option error: command 'vcpupin' requires <cpulist> option These messages that "vshCommandCheckOpts" outputs is no problem.
What I meant to say was that I want to apply following rules of "migrate" to "vcpupin". ***migrate*** - when option <desturi> error It outputs error message in "vshCommandCheckOpts". error: command 'migrate' requires <desturi> option - when internal-error occurs at "vshCommandOptString" It outputs error message after "vshCommandOptString". migrate: Missing desturi ***vcpupin*** - when option <cpulist> error It outputs error message in "vshCommandCheckOpts". error: command 'vcpupin' requires <cpulist> option - when internal-error occurs at "vshCommandOptString" ***no error-message, and return 1*** ->I want to output error-message here, because I want to tell an error to a user. Regards, Shigeki Sakamoto.
Hi, Would you give me a comment on my opinion ? If not, I make a patch that is suitable for my opinion. Shigeki Sakamoto.
$ virsh vcpupin error: command 'vcpupin' requires <domain> option error: command 'vcpupin' requires <vcpu> option error: command 'vcpupin' requires <cpulist> option These messages that "vshCommandCheckOpts" outputs is no problem.
What I meant to say was that I want to apply following rules of "migrate" to "vcpupin".
***migrate*** - when option <desturi> error It outputs error message in "vshCommandCheckOpts". error: command 'migrate' requires <desturi> option
- when internal-error occurs at "vshCommandOptString" It outputs error message after "vshCommandOptString". migrate: Missing desturi
***vcpupin*** - when option <cpulist> error It outputs error message in "vshCommandCheckOpts". error: command 'vcpupin' requires <cpulist> option
- when internal-error occurs at "vshCommandOptString" ***no error-message, and return 1*** ->I want to output error-message here, because I want to tell an error to a user.
Regards, Shigeki Sakamoto.
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Mar 25, 2008 at 03:22:41PM +0900, S.Sakamoto wrote:
Would you give me a comment on my opinion ? If not, I make a patch that is suitable for my opinion. I forgot to attach a patch... I attach it. If not, please apply it.
Shigeki Sakamoto.
OK, I've committed your final version. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
participants (2)
-
Richard W.M. Jones -
S.Sakamoto