On 7/21/23 14:27, Daniel P. Berrangé wrote:
> On Wed, Jul 12, 2023 at 04:49:53PM +0200, Michal Privoznik wrote:
>> A <channel/> device is basically an UNIX socket into guest.
>> Whatever is sent from the host, appears in the guest and vice
>> versa. But because of that, the length of the path to the socket
>> is important (underscored by fact that we derive the path from
>> domain short name). But there are still cases where we might not
>> fit into UNIX_PATH_MAX limit (usually 108 characters), because
>> the path is derived also from other variables, e.g.
>> XDG_CONFIG_HOME for session domains.
>>
>> There is one component though, that's needless: "/target/". Drop
>> it. This is safe to do, because running domains have their path
>> saved in status XML and even though paths are dropped on
>> migration, they are not part of guest ABI and thus we are free to
>> change them.
>
> This mentions dropping '/target/' which makes sense, but...
>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 94587638c3..b4844351ba 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1780,7 +1780,7 @@ qemuDomainSetPrivatePaths(virQEMUDriver *driver,
>> priv->libDir = g_strdup_printf("%s/domain-%s", cfg->libDir,
>> domname);
>>
>> if (!priv->channelTargetDir)
>> - priv->channelTargetDir = g_strdup_printf("%s/domain-%s",
>> + priv->channelTargetDir = g_strdup_printf("%s/%s",
>> cfg->channelTargetDir,
>> domname);
>
> ...this is also dropping 'domain-' which also makes sense, but
> is not mentioned in the commit message.
>
>> @@ -5352,9 +5352,12 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
>> * libvirt 1.2.19 - 1.3.2:
>> * {cfg->channelTargetDir}/domain-{dom-name}/{target-name}
>> *
>> - * libvirt 1.3.3 and newer:
>> + * libvirt 1.3.3 - 9.4.0:
>> *
>> {cfg->channelTargetDir}/domain-{dom-id}-{short-dom-name}/{target-name}
>> *
>> + * libvirt 9.6.0 and newer:
>> + * {cfg->channelTargetDir}/{dom-id}-{short-dom-name}/{target-name}
>> + *
>
> This doesn't make it clear that 'cfg->channelTargetDir' is actually
> different in those two examples. Should we change th old ones to
>
> libvirt 1.2.19 - 1.3.2:
> {cfg->channelTargetDir}/target/domain-{dom-name}/{target-name}
>
> libvirt 1.3.3 - 9.4.0:
>
> {cfg->channelTargetDir}/target/domain-{dom-id}-{short-dom-name}/{target-name}
>
>
Good point, let me fix that.
>
>> * The unix socket path was stored in config XML until libvirt 1.3.0.
>> * If someone specifies the same path as we generate, they shouldn't do it.
>> *
>> @@ -5379,7 +5382,7 @@ qemuDomainChrDefDropDefaultPath(virDomainChrDef *chr,
>> cfg = virQEMUDriverGetConfig(driver);
>>
>> virBufferEscapeRegex(&buf, "^%s", cfg->channelTargetDir);
>> - virBufferAddLit(&buf, "/([^/]+\\.)|(domain-[^/]+/)");
>> + virBufferAddLit(&buf,
>> "/(target/)?([^/]+\\.)|(domain-[^/]+/)|([0-9]+-[^/]+/)");
>> virBufferEscapeRegex(&buf, "%s$", chr->target.name);
>
>
>> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> b/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> index 0c3c70a78e..2e5cbfe09c 100644
>> --- a/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> +++ b/tests/qemuhotplugtestdevices/qemuhotplug-qemu-agent-detach.xml
>> @@ -1,5 +1,5 @@
>> <channel type='unix'>
>> - <source mode='bind'
>> path='/var/lib/libvirt/qemu/channel/target/domain-7-hotplug/org.qemu.guest_agent.0'/>
>> + <source mode='bind'
>> path='/var/lib/libvirt/qemu/channel/target/7-hotplug/org.qemu.guest_agent.0'/>
>
> You dropped 'domain-' but didn't drop '/target'. Same in a few other cases
> below
Yeah, I need to squash in the next patch that handles this. Let me fix
that in v2.
Michal
