[libvirt] [PATCH 1/3] qemu: Allow to specify the sysfs root for qemuBuildCommandLine

[Osier Yang]

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.
>