On 16/05/13 01:09, Laine Stump wrote:
On 05/15/2013 07:23 AM, Osier Yang wrote:
On 15/05/13 19:09, Daniel P. Berrange wrote:
On Wed, May 15, 2013 at 12:07:42PM +0100, Daniel P. Berrange wrote:
On Wed, May 15, 2013 at 06:52:44PM +0800, Osier Yang wrote:
Since 0d70656afded, it starts to access the sysfs files to build
the qemu command line (by virSCSIDeviceGetSgName, which is find
out the scsi generic device name by adpater:bus:target:unit), there
is no way to work around, qemu wants to see the scsi generic device
like "/dev/sg6" anyway.
And I think it's not only the place which needs to access sysfs
files when building qemu command line in future.
As a solution, this introduces a new argument "sysfs_root" for
qemuBuildCommandLine, and thus the tests can feed the fake sysfs
root to it.
IMHO it would be nicer to detach the command line generator
code from any use of sysfs in this case.
Instead of having QEMU call virSCSIDeviceGetSgName() directly,
pass in a callback for it to use to resolve the device path.
eg
typedef char *(*qemuGetSCSIDevice)(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit);
Nice idea.
the qemuProcessStart code can pass in an impl of that callback
I guess you meant qemuBuildCommandLine or so, tests won't
call qemuProcessStart. to start the guest.
But qemuProcessStart() calls qemuBuildCommandLine().
Yeah, but tests will just involke qemuBuildCommandLine.
which calls virSCSIDeviceGetSgName(), while the test suite
can pass in an impl which returns a hard-coded device string.
In fact there are probably other cases where we should pass in
callbacks like this to the QEMU command line builder.
In that same category, the calls to qemuNetworkIfaceConnect,
qemuPhysIfaceConnect, and qemuOpenVhostNet (and the "vmop" param to
qemuBuildCommandLine()) have been bothering me for awhile, but instead
yeah, I noticed them when making the patches. And I intended
to clean up them using callbacks, fortunately, I didn't do it before
the agreement.
of a callback sent to qemuBuildCommandLine(), I was considering the more
direct idea of opening all the sockets before even calling
qemuBuildCommandLine() and sending them in the arglist "somehow"[*]
(rather than sending in a pointer to a callback that opens the fd's and
passes them back, which gets a bit confusing if you're not intimately
familiar with the code).
[*] I *think* a reasonable way to do this would be to add alist of tap
fd's and a hidden vhost-net fd) to the virActualNetDef that is part of
every virNetDef, then populate it as a part of
networkAllocateActualDevice() (which could probably do with a name
change). This would have the side effect of consolidating all network
IMHO the "def" struct should *only* keep configuration related stuffs,
and it doesn't resolve the build failure, as the required command line
string should be got in run-time anyway.
device setup into the network driver, thus making it simpler to split it
off into its own service.
Instead of
adding many parameters to the API, we could provide a struct
Agreed, many params is always not good. .
containing all the various callbacks needed in one go.
Daniel
Maybe each device object could have a "hidden" part (i.e. not
necessarily visible in the XML) that can be setup before calling
qemuBuildCommandLine(), and qemuBuildCommandline could use that info if
it's there, otherwise fills in "null" args on the commandline.
Oh, you have a solution (the "null"). Putting /dev/null in the
scsi-generic args
files works (basicly it's similar with hard-code a "/dev/sg*" in the
qemuxml2argvtest.c),
but looks strange.
I'm not against using a callback when it's appropriate, but I think they
hinder proper understanding of code by newcomers, and should be used
sparingly unless necessary.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list