Re: [Libvir] [PATCH] Another Report error in virsh.c code.
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. virsh.patch Description: Binary data -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
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 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
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 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
$ 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
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
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 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
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***
2225desturi = vshCommandOptString (cmd, desturi, found);
2226if (!found) {
2227vshError (ctl, FALSE, %s, _(migrate: Missing desturi));
2228goto done;
2229}
***vcpupin***
1731if (!(cpulist = vshCommandOptString(cmd, cpulist, NULL))) {
+ vshError (ctl, FALSE, %s, _(vcpupin: Missing cpulist));
1732virDomainFree(dom);
1733return FALSE;
1734}
Shigeki Sakamoto.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
On Tue, Mar 11, 2008 at 07:19:57PM +0900, S.Sakamoto wrote:
I wish it is unified as follows.
***migrate***
2225desturi = vshCommandOptString (cmd, desturi, found);
2226if (!found) {
2227vshError (ctl, FALSE, %s, _(migrate: Missing desturi));
2228goto done;
2229}
***vcpupin***
1731if (!(cpulist = vshCommandOptString(cmd, cpulist, NULL))) {
+ vshError (ctl, FALSE, %s, _(vcpupin: Missing cpulist));
1732virDomainFree(dom);
1733return 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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
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=c857ace66df5a5068ed561aad913b29fd36160f9
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 - 1.135
+++ src/virsh.c 7 Mar 2008 07:03:12 -
@@ -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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
