[libvirt] [PATCH] Virsh command vol-rename
From: Harry Wei <harryxiyou@gmail.com> Vol-rename is a convenience function, requiring when one wanna rename a volume name. Signed-off-by: Harry Wei <harryxiyou@gmail.com> --- tools/virsh-volume.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 0ca295f..8757d54 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -490,6 +490,103 @@ cleanup: } /* + * "vol-rename" command + */ +static const vshCmdInfo info_vol_rename[] = { + {.name = "help", + .data = N_("rename a volume.") + }, + {.name = "desc", + .data = N_("Rename an existing volume.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_vol_rename[] = { + {.name = "vol", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("orig vol name or key") + }, + {.name = "newname", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("new name") + }, + {.name = "pool", + .type = VSH_OT_STRING, + .help = N_("pool name or uuid") + }, + {.name = NULL} +}; + +static bool +cmdVolRename(vshControl *ctl, const vshCmd *cmd) +{ + virStoragePoolPtr origpool = NULL; + virStorageVolPtr origvol = NULL, newvol = NULL; + const char *origname = NULL; + const char *name = NULL; + char *origxml = NULL; + xmlChar *newxml = NULL; + bool ret = false; + unsigned int flags = 0; + + if (!(origvol = vshCommandOptVol(ctl, cmd, "vol", "pool", &origname))) + goto cleanup; + + origpool = virStoragePoolLookupByVolume(origvol); + if (!origpool) { + vshError(ctl, "%s", _("failed to get parent pool")); + goto cleanup; + } + + if (vshCommandOptStringReq(ctl, cmd, "newname", &name) < 0) + goto cleanup; + + origxml = virStorageVolGetXMLDesc(origvol, 0); + if (!origxml) + goto cleanup; + + newxml = vshMakeCloneXML(origxml, name); + if (!newxml) { + vshPrint(ctl, "%s", _("Failed to allocate XML buffer")); + goto cleanup; + } + + newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, origvol, flags); + + if (newvol != NULL) { + vshPrint(ctl, _("Vol %s cloned from %s\n"), + virStorageVolGetName(newvol), virStorageVolGetName(origvol)); + } else { + vshError(ctl, _("Failed to clone vol from %s"), + virStorageVolGetName(origvol)); + goto cleanup; + } + + if (virStorageVolDelete(origvol, 0) == 0) { + vshPrint(ctl, _("Vol %s deleted\n"), origname); + } else { + vshError(ctl, _("Failed to delete vol %s"), origname); + goto cleanup; + } + + ret = true; + +cleanup: + VIR_FREE(origxml); + xmlFree(newxml); + if (origvol) + virStorageVolFree(origvol); + if (newvol) + virStorageVolFree(newvol); + if (origpool) + virStoragePoolFree(origpool); + return ret; +} + +/* * "vol-clone" command */ static const vshCmdInfo info_vol_clone[] = { @@ -1752,6 +1849,12 @@ cmdVolPath(vshControl *ctl, const vshCmd *cmd) } const vshCmdDef storageVolCmds[] = { + {.name = "vol-rename", + .handler = cmdVolRename, + .opts = opts_vol_rename, + .info = info_vol_rename, + .flags = 0 + }, {.name = "vol-clone", .handler = cmdVolClone, .opts = opts_vol_clone, -- 1.7.9.5
On 18/04/13 16:29, harryxiyou@gmail.com wrote:
From: Harry Wei <harryxiyou@gmail.com>
Vol-rename is a convenience function, requiring when one wanna rename a volume name.
NACK. It should use the upcoming rename APIs for the *-rename virsh commands instead .
Signed-off-by: Harry Wei <harryxiyou@gmail.com> --- tools/virsh-volume.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 0ca295f..8757d54 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -490,6 +490,103 @@ cleanup: }
/* + * "vol-rename" command + */ +static const vshCmdInfo info_vol_rename[] = { + {.name = "help", + .data = N_("rename a volume.") + }, + {.name = "desc", + .data = N_("Rename an existing volume.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_vol_rename[] = { + {.name = "vol", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("orig vol name or key") + }, + {.name = "newname", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("new name") + }, + {.name = "pool", + .type = VSH_OT_STRING, + .help = N_("pool name or uuid") + }, + {.name = NULL} +}; + +static bool +cmdVolRename(vshControl *ctl, const vshCmd *cmd) +{ + virStoragePoolPtr origpool = NULL; + virStorageVolPtr origvol = NULL, newvol = NULL; + const char *origname = NULL; + const char *name = NULL; + char *origxml = NULL; + xmlChar *newxml = NULL; + bool ret = false; + unsigned int flags = 0; + + if (!(origvol = vshCommandOptVol(ctl, cmd, "vol", "pool", &origname))) + goto cleanup; + + origpool = virStoragePoolLookupByVolume(origvol); + if (!origpool) { + vshError(ctl, "%s", _("failed to get parent pool")); + goto cleanup; + } + + if (vshCommandOptStringReq(ctl, cmd, "newname", &name) < 0) + goto cleanup; + + origxml = virStorageVolGetXMLDesc(origvol, 0); + if (!origxml) + goto cleanup; + + newxml = vshMakeCloneXML(origxml, name); + if (!newxml) { + vshPrint(ctl, "%s", _("Failed to allocate XML buffer")); + goto cleanup; + } + + newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, origvol, flags); + + if (newvol != NULL) { + vshPrint(ctl, _("Vol %s cloned from %s\n"), + virStorageVolGetName(newvol), virStorageVolGetName(origvol)); + } else { + vshError(ctl, _("Failed to clone vol from %s"), + virStorageVolGetName(origvol)); + goto cleanup; + } + + if (virStorageVolDelete(origvol, 0) == 0) {
Indention problem..
+ vshPrint(ctl, _("Vol %s deleted\n"), origname); + } else { + vshError(ctl, _("Failed to delete vol %s"), origname); + goto cleanup; + } + + ret = true; + +cleanup: + VIR_FREE(origxml); + xmlFree(newxml); + if (origvol) + virStorageVolFree(origvol); + if (newvol) + virStorageVolFree(newvol); + if (origpool) + virStoragePoolFree(origpool); + return ret; +} + +/* * "vol-clone" command */ static const vshCmdInfo info_vol_clone[] = { @@ -1752,6 +1849,12 @@ cmdVolPath(vshControl *ctl, const vshCmd *cmd) }
const vshCmdDef storageVolCmds[] = { + {.name = "vol-rename", + .handler = cmdVolRename, + .opts = opts_vol_rename, + .info = info_vol_rename, + .flags = 0 + }, {.name = "vol-clone", .handler = cmdVolClone, .opts = opts_vol_clone,
On Thu, Apr 18, 2013 at 5:27 PM, Osier Yang <jyang@redhat.com> wrote:
On 18/04/13 16:29, harryxiyou@gmail.com wrote:
From: Harry Wei <harryxiyou@gmail.com>
Vol-rename is a convenience function, requiring when one wanna rename a volume name.
NACK. It should use the upcoming rename APIs for the *-rename virsh commands instead .
I cannot understand your *upcoming rename APIs* clearly. Would you please explain these stuffs in details. Osier, thanks for your comments very much. -- Thanks Harry Wei
On Thu, Apr 18, 2013 at 04:29:45PM +0800, harryxiyou@gmail.com wrote:
From: Harry Wei <harryxiyou@gmail.com>
Vol-rename is a convenience function, requiring when one wanna rename a volume name.
Signed-off-by: Harry Wei <harryxiyou@gmail.com> --- tools/virsh-volume.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)
NACK copying & deleting volumes is totally not what people want from a "rename" command. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Apr 18, 2013 at 5:30 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Apr 18, 2013 at 04:29:45PM +0800, harryxiyou@gmail.com wrote:
From: Harry Wei <harryxiyou@gmail.com>
Vol-rename is a convenience function, requiring when one wanna rename a volume name.
Signed-off-by: Harry Wei <harryxiyou@gmail.com> --- tools/virsh-volume.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)
NACK copying & deleting volumes is totally not what people want from a "rename" command.
Hmmm..., maybe this is not direct for us. However, i am not sure about what kind of "rename" command we need. Actually, i am investigating what kind of "rename" we need. Let me give another way. Only change the name of volume and need not copying and deleting volume. To back-end storage volume(Offline storage), Libvirt just call relevant storage driver and the storage volume(with a volume name) is created by back-end storage driver. If we wanna change the volume name without copying and deleting, we may need give hook function to let back-end driver to realize relevant driver for rename stuffs. Danpb, thanks for your comments very much. -- Thanks Harry Wei
On 04/18/2013 06:21 AM, harryxiyou wrote:
On Thu, Apr 18, 2013 at 5:30 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Apr 18, 2013 at 04:29:45PM +0800, harryxiyou@gmail.com wrote:
From: Harry Wei <harryxiyou@gmail.com>
Vol-rename is a convenience function, requiring when one wanna rename a volume name.
Signed-off-by: Harry Wei <harryxiyou@gmail.com> --- tools/virsh-volume.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)
NACK copying & deleting volumes is totally not what people want from a "rename" command.
Hmmm..., maybe this is not direct for us. However, i am not sure about what kind of "rename" command we need. Actually, i am investigating what kind of "rename" we need. Let me give another way.
Only change the name of volume and need not copying and deleting volume. To back-end storage volume(Offline storage), Libvirt just call relevant storage driver and the storage volume(with a volume name) is created by back-end storage driver. If we wanna change the volume name without copying and deleting, we may need give hook function to let back-end driver to realize relevant driver for rename stuffs.
Exactly - we need to add a new libvirt API that can support renames at the backend driver level, with instant effect (a directory pool forwarding to rename(2), other pools using pool-specific renaming commands). It might be worth still having virsh vol-rename have an option to do a long-running copy/delete in the case of a backend that doesn't support instant renames, but the default should be instant rename or failure. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Apr 18, 2013 at 11:03 PM, Eric Blake <eblake@redhat.com> wrote: [...]
Exactly - we need to add a new libvirt API that can support renames at the backend driver level, with instant effect (a directory pool forwarding to rename(2), other pools using pool-specific renaming commands).
It might be worth still having virsh vol-rename have an option to do a long-running copy/delete in the case of a backend that doesn't support instant renames, but the default should be instant rename or failure.
Okay, i will give my V2 patch-set for these stuffs. My [1]CheckList for *Rename* jobs have described my schedule for them one by one. I also offer the [2]design plan for these stuffs. [1] http://code.google.com/p/gsocxy/wiki/CheckList [2] http://code.google.com/p/gsocxy/wiki/GeneralDesignForRenameAPIs Eric, very thanks for your comments ;-) -- Thanks Harry Wei
On 19/04/13 11:35, harryxiyou wrote:
On Thu, Apr 18, 2013 at 11:03 PM, Eric Blake <eblake@redhat.com> wrote: [...]
Exactly - we need to add a new libvirt API that can support renames at the backend driver level, with instant effect (a directory pool forwarding to rename(2), other pools using pool-specific renaming commands).
It might be worth still having virsh vol-rename have an option to do a long-running copy/delete in the case of a backend that doesn't support instant renames, but the default should be instant rename or failure.
Okay, i will give my V2 patch-set for these stuffs. My [1]CheckList for *Rename* jobs have described my schedule for them one by one. I also offer the [2]design plan for these stuffs.
[1] http://code.google.com/p/gsocxy/wiki/CheckList [2] http://code.google.com/p/gsocxy/wiki/GeneralDesignForRenameAPIs
Eric, very thanks for your comments ;-)
There is no new things in the wiki. Before writing the code, I'd suggest going through the HACKING and the document for how to implement a new API for libvirt [1], it's a bit out of date though, but still useful. And you can pick up the patches which introduced new API as example. I think with the document and the existing patches for new API, you will known how/what to do next. After that, if you are still not confident about your approach, send RFC to list, instead of putting in your wiki. On one hand, it's not convenient, I believe not everyone likes an external link. On the other hand, it's not good for tracking the history, even you can guarantee it won't be deleted/changed forever. Including draft patches in RFC is not required, but having it is always better. You might be not picked up finally for the project, but it's the generic knowledge worth to learn if you are interested in contributing to libvirt. [1] http://libvirt.org/api_extension.html Osier
On Fri, Apr 19, 2013 at 1:42 PM, Osier Yang <jyang@redhat.com> wrote: [...]
There is no new things in the wiki.
Sorry, i have not put my new design plan for 'Rename volume name'.
Before writing the code, I'd suggest going through the HACKING and the document for how to implement a new API for libvirt [1], it's a bit out of date though, but still useful. And you can pick up the patches which introduced new API as example. I think with the document and the existing patches for new API, you will known how/what to do next.
Yup, these suggestions are really helpful for me. I will read the docs and find the patches that introduced new API.
After that, if you are still not confident about your approach, send RFC to list, instead of putting in your wiki. On one hand, it's not convenient, I believe not everyone likes an external link. On the other hand, it's not good for tracking the history, even you can guarantee it won't be deleted/changed forever. Including draft patches in RFC is not required, but having it is always better.
Ok, i will, thanks.
You might be not picked up finally for the project, but it's the generic knowledge worth to learn if you are interested in contributing to libvirt.
Sure, i think so. Osier, very thanks for your comments ;-). -- Thanks Harry Wei
On Thu, Apr 18, 2013 at 09:03:59AM -0600, Eric Blake wrote:
On 04/18/2013 06:21 AM, harryxiyou wrote:
On Thu, Apr 18, 2013 at 5:30 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Apr 18, 2013 at 04:29:45PM +0800, harryxiyou@gmail.com wrote:
From: Harry Wei <harryxiyou@gmail.com>
Vol-rename is a convenience function, requiring when one wanna rename a volume name.
Signed-off-by: Harry Wei <harryxiyou@gmail.com> --- tools/virsh-volume.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)
NACK copying & deleting volumes is totally not what people want from a "rename" command.
Hmmm..., maybe this is not direct for us. However, i am not sure about what kind of "rename" command we need. Actually, i am investigating what kind of "rename" we need. Let me give another way.
Only change the name of volume and need not copying and deleting volume. To back-end storage volume(Offline storage), Libvirt just call relevant storage driver and the storage volume(with a volume name) is created by back-end storage driver. If we wanna change the volume name without copying and deleting, we may need give hook function to let back-end driver to realize relevant driver for rename stuffs.
Exactly - we need to add a new libvirt API that can support renames at the backend driver level, with instant effect (a directory pool forwarding to rename(2), other pools using pool-specific renaming commands).
It might be worth still having virsh vol-rename have an option to do a long-running copy/delete in the case of a backend that doesn't support instant renames, but the default should be instant rename or failure.
I don't think we should ever do a copy+delete as part of any rename command. It just isn't appropriate for the semantics of 'rename' IMHO Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (5)
-
Daniel P. Berrange -
Eric Blake -
harryxiyou -
harryxiyou＠gmail.com
-
Osier Yang