[libvirt] [PATCH] virsh: A bit smarter attach-disk
Detects the file type of source path if no "--sourcetype" and "driver" is specified, instead of always set the disk type as "block". And previous "virCommandOptString" ensures "source" is not NULL, remove the conditional checking. --- tools/virsh.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d45a4c9..3b845ac 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14428,6 +14428,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) const char *stype = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; char *xml; + struct stat st; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -14458,8 +14459,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) } if (!stype) { - if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) + if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) { isFile = true; + } else { + if (!stat(source, &st)) + isFile = S_ISREG(st.st_mode) ? true : false; + } } else if (STREQ(stype, "file")) { isFile = true; } else if (STRNEQ(stype, "block")) { @@ -14497,10 +14502,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (driver || subdriver || cache) virBufferAddLit(&buf, "/>\n"); - if (source) - virBufferAsprintf(&buf, " <source %s='%s'/>\n", - (isFile) ? "file" : "dev", - source); + virBufferAsprintf(&buf, " <source %s='%s'/>\n", + (isFile) ? "file" : "dev", + source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode); -- 1.7.1
On 15.03.2012 10:13, Osier Yang wrote:
Detects the file type of source path if no "--sourcetype" and "driver" is specified, instead of always set the disk type as "block".
And previous "virCommandOptString" ensures "source" is not NULL, remove the conditional checking. --- tools/virsh.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d45a4c9..3b845ac 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14428,6 +14428,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) const char *stype = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; char *xml; + struct stat st;
if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -14458,8 +14459,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) }
if (!stype) { - if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) + if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) { isFile = true; + } else { + if (!stat(source, &st)) + isFile = S_ISREG(st.st_mode) ? true : false;
I think S_ISREG() would be sufficient here. But that's just cosmetic.
+ } } else if (STREQ(stype, "file")) { isFile = true; } else if (STRNEQ(stype, "block")) { @@ -14497,10 +14502,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (driver || subdriver || cache) virBufferAddLit(&buf, "/>\n");
- if (source) - virBufferAsprintf(&buf, " <source %s='%s'/>\n", - (isFile) ? "file" : "dev", - source); + virBufferAsprintf(&buf, " <source %s='%s'/>\n", + (isFile) ? "file" : "dev", + source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode);
However this looks bad. As written in commend just below virCommandOptString(, source): /* Allow empty string as a placeholder that implies no source, for * use in adding a cdrom drive with no disk. */ if (!*source) source = NULL; That means: virsh attach-disk <domain> "" dummy make source NULL. Therefore you want to check source != NULL in the first chunk too (okay, not as strict as here, since passing NULL to stat() makes it fail, but it's clean coding style what matters too). Michal
On 03/15/2012 05:12 PM, Michal Privoznik wrote:
On 15.03.2012 10:13, Osier Yang wrote:
Detects the file type of source path if no "--sourcetype" and "driver" is specified, instead of always set the disk type as "block".
And previous "virCommandOptString" ensures "source" is not NULL, remove the conditional checking. --- tools/virsh.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d45a4c9..3b845ac 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14428,6 +14428,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) const char *stype = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; char *xml; + struct stat st;
if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -14458,8 +14459,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) }
if (!stype) { - if (driver&& (STREQ(driver, "file") || STREQ(driver, "tap"))) + if (driver&& (STREQ(driver, "file") || STREQ(driver, "tap"))) { isFile = true; + } else { + if (!stat(source,&st)) + isFile = S_ISREG(st.st_mode) ? true : false;
I think S_ISREG() would be sufficient here. But that's just cosmetic.
+ } } else if (STREQ(stype, "file")) { isFile = true; } else if (STRNEQ(stype, "block")) { @@ -14497,10 +14502,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (driver || subdriver || cache) virBufferAddLit(&buf, "/>\n");
- if (source) - virBufferAsprintf(&buf, "<source %s='%s'/>\n", - (isFile) ? "file" : "dev", - source); + virBufferAsprintf(&buf, "<source %s='%s'/>\n", + (isFile) ? "file" : "dev", + source); virBufferAsprintf(&buf, "<target dev='%s'/>\n", target); if (mode) virBufferAsprintf(&buf, "<%s/>\n", mode);
However this looks bad. As written in commend just below virCommandOptString(, source):
/* Allow empty string as a placeholder that implies no source, for * use in adding a cdrom drive with no disk. */ if (!*source) source = NULL;
Oh, yes, I didn't notice the comments. Osier
Detects the file type of source path if no "--sourcetype" and "driver" is specified, instead of always set the disk type as "block". --- tools/virsh.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d45a4c9..19f9bbe 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14428,6 +14428,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) const char *stype = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; char *xml; + struct stat st; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -14458,8 +14459,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) } if (!stype) { - if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) + if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) { isFile = true; + } else { + if (source && !stat(source, &st)) + isFile = S_ISREG(st.st_mode) ? true : false; + } } else if (STREQ(stype, "file")) { isFile = true; } else if (STRNEQ(stype, "block")) { -- 1.7.1
On 15.03.2012 11:17, Osier Yang wrote:
Detects the file type of source path if no "--sourcetype" and "driver" is specified, instead of always set the disk type as "block". --- tools/virsh.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
ACK Michal
On 03/15/2012 05:32 PM, Michal Privoznik wrote:
On 15.03.2012 11:17, Osier Yang wrote:
Detects the file type of source path if no "--sourcetype" and "driver" is specified, instead of always set the disk type as "block". --- tools/virsh.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
ACK
Thanks, pushed. Osier
participants (2)
-
Michal Privoznik -
Osier Yang