Re: [PATCH v2] Conf: Move validation of virDomainGraphicsListenDef out of Parser
On 4/5/23 19:56, Shiva wrote:
Greetings Sir
My apologies for the incorrect formats as I am a beginner to git. Thank
you very much for your corrections.
That's okay and GSoC was invented specifically for this reason. To
attract students into Open Source and its development. But at the same
time, review bandwidth is precious and therefore it's important to
listen to feedback. I'm glad to see v2 worked out better.
I have a few questions:
Can Questions and doubts be posted within the same mail that consists a
patch file? Or should they be put in a separate email (as I am
attempting to do now)?
It's okay to send it as two separate emails even. I mean, usually, when
we work on something and for instance can't make a decision (e.g. on a
design or something), we often just write an e-mail to the list,
explaining what it is we're working on and that we face this decision,
what are the options, etc. And other developers reply with their thoughts.
As per your instruction to make the functions static, I was able to make
the below function (created by me) static by calling it from
virDomainGraphicsDefListensValidate() within domain_validate.c
static int
virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
const virDomainGraphicsDef *graphics)
Yep, that's correct.
Similarly,
In this patch: Link
<https://listman.redhat.com/archives/libvir-list/2023-April/239205.html>
I had created a function named;
int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
const char *transport);
that removes the validation checks of socket in
virDomainStorageNetworkParseHost().
While I found a function to call Graphics Validation within validate.c,
I am unable to find a function that can be used to call
NetworkHostSocketValidate() from within domain_validate.c. Could you
please provide a pointer on how it could be done?
Yeah, you may need to write new code. As I've said, we're only gradually
moving code into domain_validate.c. But let me see:
So virDomainStorageNetworkParseHost() is called from exactly two places
(identified by 'git grep -npC10 virDomainStorageNetworkParseHost'):
virDomainBackupDefParseXML
virDomainStorageNetworkParseHosts
Let's focus on the latter for now, we'll get back to the former soon.
virDomainStorageNetworkParseHosts() is then called from the following
places:
virDomainHostdevSubsysSCSIiSCSIDefParseXML
virDomainDiskSourceNetworkParse
after repeating this process more times, we can find what device type
might end up calling this function and what conditions lead to it. For
instance, in this case it's a hostdev that has scsi subdev, iscsi
protocol, etc.
Now, looking at domain_validate.c there's
virDomainDeviceDefValidateInternal() which calls
virDomainHostdevDefValidate() for hostdevs, and then has a block of
checks for scsi subsys, but no further code for iscsi protocol. So that
looks like a place to add the check.
Basically, we're trying to reconstruct the path from domain_conf.c that
lead to the check in domain_validate.c.
And now let's return back to the other caller of the function:
virDomainBackupDefParseXML(). Unfortunately, the XML that's parsed there
is not validated - there's no vir*Validate() function called; so if this
check is removed and moved into domain_validate.c then this path would
be left buggy.
After all, it's probably okay to leave the check there. Introducing
validation to backup XMLs is solvable, but definitely not a bite sized
task for newcomer.
Michal