1:43 a.m.
Hi,
I am watching through the virsh code for same type bug check.
http://git.et.redhat.com/?p=libvirt.git;a=commit;h=c857ace66df5a5068ed561...
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;
}
3:23 a.m.
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=c857ace66df5a5068ed561...
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
3:09 a.m.
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.
4:23 a.m.
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
5:19 a.m.
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.
10:31 a.m.
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
4:48 a.m.
$ 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.
11 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:24 a.m.
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
6085
days inactive
6103
days old
9 comments
2 participants
participants (2)
-
Richard W.M. Jones -
S.Sakamoto